Hi,

Alexey Dobriyan <adobriyan@gmail.com> writes:

quoted text
> 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.

quoted text
> 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
--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/