Messages in this thread | | | Date | Sun, 17 Aug 2008 01:04:39 +0400 | From | Alexey Dobriyan <> | Subject | Re: [PATCH] De-macro spin_trylock_irq, spin_trylock_irqsave, write_trylock_irqsave |
| |
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?
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"!
| |