Re: [RFC][PATCH 2/6] x86, nmi: create new NMI handler routines

[Don Zickus]

On Mon, Aug 22, 2011 at 04:16:20PM +0200, Peter Zijlstra wrote:
> On Fri, 2011-08-19 at 16:37 -0400, Don Zickus wrote:
> > +static struct nmiaction *__free_nmi(unsigned int type, const char *name)
> > +{
> > +       struct nmi_desc *desc = nmi_to_desc(type);
> > +       struct nmiaction *n, **np = &(desc->head);
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&desc->lock, flags);
> > +
> ...
> > +
> > +       spin_unlock_irqrestore(&desc->lock, flags);
> > +       synchronize_rcu();
> > +       return *np;
> > +}
> 
> > +void unregister_nmi_handler(unsigned int type, const char *name)
> > +{
> > +       kfree(__free_nmi(type, name));
> > +}
> 
> This code is weird.. why not have the kfree() in __free_nmi(), also why
> use sync_rcu() and not use kfree_rcu()?

I was looking at trying to use kfree_rcu and noticed I would have to add
another element to the nmiaction struct and another function as a callback
to kfree the memory from the device name.  The overhead didn't seem worth
it.  For some reason it just seems simpler to keep it the way it is and
just have

	struct nmiaction *a;

	a = __free_nmi(type, name);
	if (a) {
		kfree(a->name);
		kfree(a);
	}

(side note:  I was keeping the kfree()s in here for symmetry with the
registration handler).

Maybe I don't understand kfree_rcu, but what advantage do I have by
placing rcu_head into nmiaction that never gets used except on unregister
(which rarely happens to begin with)?

Cheers,
Don
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/