Re: [PATCH] De-macro spin_trylock_irq, spin_trylock_irqsave, write_trylock_irqsave

[Johannes Weiner <hannes@saeurebad.de>]

Hi,

Alexey Dobriyan <adobriyan@gmail.com> writes:

> On Sat, Aug 16, 2008 at 03:46:00PM +0200, Ingo Molnar wrote:
>> 
>> * Alexey Dobriyan <adobriyan@gmail.com> wrote:
>> 
>> > 1) de-macro, remove ({ usages as side-effect,
>> > 2) change calling convention to not accept "flags" by value -- trylock
>> >    functions can modify them, so by-value is misleading, and number of users
>> >    is relatively low.
>> > 3) de-macro spin_trylock_irq() for a change.
>> 
>> > +++ b/kernel/sched.c
>> > @@ -1174,7 +1174,7 @@ static void resched_cpu(int cpu)
>> >  	struct rq *rq = cpu_rq(cpu);
>> >  	unsigned long flags;
>> >  
>> > -	if (!spin_trylock_irqsave(&rq->lock, flags))
>> > +	if (!spin_trylock_irqsave(&rq->lock, &flags))
>> >  		return;
>> 
>> hm, i dont really like this assymetric calling convention to other 
>> locking primitives that all take 'flags' as a value. 
>> [spin_lock_irqsave(), etc.]
>> 
>> so what's the point really? It sure does not make actual usage more 
>> readable.
>
> Only slightly, reader is hinted that flags can be changed, otherwise
> they will be passed by value.
>
>> If we switched _all_ primitives to use flags as a pointer, 
>> that might make sense, in theory.
>
> We can't really, and I don't propose that: ~8700 usages of
> spin_lock_irqsave, ~1300 usages of local_irq_save. However for code
> which has small number of users, why not?

I would also prefer to maintain symmetry here.  Your argument is moot,
why diverge a small part of one API just because it is not used much?

Everyone using the spin_lock functions learns the weird interface pretty
fast.  If you are in a rare situation where you have to use the trylock
versions, you would really expect them to be used equivalently.

It is weird but diverging it doesn't make it any better.

> The prehistory of this patch is that I'm deeply in spinlock and
> irqflags.h headers for clean irq_flags_t conversion and overall
> implession is that they're horrible.
>
> Just the joke with local_irq_enable() defined via raw_local_irq_enable()
> and several lines below in the opposite order.
>
> The patch is about slightly cleaner code close to C. ;-)
>
>> (but it would also be hugely invasive, 
>> with not much upside with tons of downside like years of migration 
>> fallout and having to rewrite hundreds of kernel hacking books ;-) )
>
> I want my money back for scheduler chapter from "Understanding the
> Linux Kernel"!

I agree that this argument of Ingo's is not a very good one... ;)

	Hannes