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    [W:0.032 / U:29.124 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site