lkml.org 
[lkml]   [2008]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] De-macro spin_trylock_irq, spin_trylock_irqsave, write_trylock_irqsave
Date
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


\
 
 \ /
  Last update: 2008-08-16 23:21    [from the cache]
©2003-2011 Jasper Spaans