Re: [ANNOUNCE] Kernel Janitor's TODO list

[Manfred Spraul]

Alan Cox wrote:
> 
> > And one more point for the Janitor's list:
> > Get rid of superflous irqsave()/irqrestore()'s - in 90% of the cases
> > either spin_lock_irq() or spin_lock() is sufficient. That's both faster
> > and better readable.
> 
> Expect me to drop any submissions that do this. I'd rather take the two
> clock hit in most cases because the effect of spin_lock_irq() being used
> and people then changing which functions call each other and producing
> impossible to debug irq mishandling cases is unacceptable.
>

IMHO the main problem of spin_lock_irqsave is not the lost cpu cycles,
but readability:

void public_function()
{
	spin_lock_irqsave();
	if(rare_event)
		internal_function()
	spin_unlock_irqrestore();
}

static void internal_function()
{
	...
	spin_unlock_irq();
	kmalloc(GFP_KERNEL);
	spin_lock_irq();
}

IMHO functions that are not irq safe somewhere hidden in internal
functions should never use spin_lock_irqsave().
make_request() in 2.2 falls into that category, and the irqsave() was
removed.

Obviously spin_lock_irq() instead of spin_lock_irqsave() should only be
done if the implementation doesn't support disabled interrupts, not if
currently noone calls a function with disabled interrupts.

(make_request(), down(), smp_call_function()...)

> The original Linux network code did this with sti() not save/restore flags.
> I've been there before, I am not going to allow a rerun of that disaster for
> a few cycles

I hope that during 2.5 we can add debugging into spin_lock_irq():
BUG() if it's called with disabled interrupts.
It's not yet possible due to schedule() with disabled interrupts (I
tried it a few months ago)

--
	Manfred