Re: [git pull] core kernel fixes

[Ingo Molnar <mingo@elte.hu>]

* Vegard Nossum <vegard.nossum@gmail.com> wrote:

> On Mon, Jun 30, 2008 at 5:32 PM, Ingo Molnar <mingo@elte.hu> wrote:
> > diff --git a/lib/debugobjects.c b/lib/debugobjects.c
> > index a76a5e1..85b18d7 100644
> > --- a/lib/debugobjects.c
> > +++ b/lib/debugobjects.c
> > @@ -68,6 +68,7 @@ static int fill_pool(void)
> >  {
> >        gfp_t gfp = GFP_ATOMIC | __GFP_NORETRY | __GFP_NOWARN;
> >        struct debug_obj *new;
> > +       unsigned long flags;
> >
> >        if (likely(obj_pool_free >= ODEBUG_POOL_MIN_LEVEL))
> >                return obj_pool_free;
> > @@ -81,10 +82,10 @@ static int fill_pool(void)
> >                if (!new)
> >                        return obj_pool_free;
> >
> > -               spin_lock(&pool_lock);
> > +               spin_lock_irqsave(&pool_lock, flags);
> >                hlist_add_head(&new->node, &obj_pool);
> >                obj_pool_free++;
> > -               spin_unlock(&pool_lock);
> > +               spin_unlock_irqrestore(&pool_lock, flags);
> >        }
> >        return obj_pool_free;
> >  }
> > @@ -110,16 +111,13 @@ static struct debug_obj *lookup_object(void *addr, struct debug_bucket *b)
> >  }
> >
> >  /*
> > - * Allocate a new object. If the pool is empty and no refill possible,
> > - * switch off the debugger.
> > + * Allocate a new object. If the pool is empty, switch off the debugger.
> >  */
> >  static struct debug_obj *
> >  alloc_object(void *addr, struct debug_bucket *b, struct debug_obj_descr *descr)
> >  {
> >        struct debug_obj *obj = NULL;
> > -       int retry = 0;
> >
> > -repeat:
> >        spin_lock(&pool_lock);
> >        if (obj_pool.first) {
> >                obj         = hlist_entry(obj_pool.first, typeof(*obj), node);
> > @@ -141,9 +139,6 @@ repeat:
> >        }
> >        spin_unlock(&pool_lock);
> >
> > -       if (fill_pool() && !obj && !retry++)
> > -               goto repeat;
> > -
> >        return obj;
> >  }
> >
> > @@ -261,6 +256,8 @@ __debug_object_init(void *addr, struct debug_obj_descr *descr, int onstack)
> >        struct debug_obj *obj;
> >        unsigned long flags;
> >
> > +       fill_pool();
> > +
> >        db = get_bucket((unsigned long) addr);
> >
> >        spin_lock_irqsave(&db->lock, flags);
> > --
> 
> Hm. I have to wonder where this patch came from.
> 
> This was my (faulty) patch: http://lkml.org/lkml/2008/6/14/193
> and Daniel J Blueman followed up with this: http://lkml.org/lkml/2008/6/15/27
> 
> ..but this one looks different from both. I am guessing the last bits 
> were added (or removed?) by Thomas?
> 
> I am wondering if the final patch was tested with the reproducible 
> test case (if so, by whom?) and whether should be credited to Daniel 
> (or Thomas?) instead...

You can use "git log -1 -p --pretty=fuller 50db04dd9c" to see the exact 
details of the commit:

----------
| commit 50db04dd9c74178e68a981a7127c37252ffb3242
| Author:     Vegard Nossum <vegard.nossum@gmail.com>
| AuthorDate: Sun Jun 15 00:47:36 2008 +0200
| Commit:     Thomas Gleixner <tglx@linutronix.de>
| CommitDate: Wed Jun 18 11:09:54 2008 +0200
|
| [...]
|
|   [ daniel.blueman@gmail.com: pool_lock needs to be taken irq safe in fill_pool ]
|
|   Reported-by: Daniel J Blueman <daniel.blueman@gmail.com>
|   Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>
|   Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
----------

As you can see it from the Commit line, it was committed by Thomas.

The "[ daniel.blueman: pool_lock ... ]" line shows that Thomas - instead 
of creating two commits - merged the two fixes into a single commit and 
credited Daniel for the irq-safe fix. This is the standard technique to 
squash small patches and to make fixes multi-authored.

The patch was tested with our standard tests so it's certainly good in 
practice - but i havent specifically tried your testcase (maybe Thomas 
has). Can you see any problem with the fix?

	Ingo