Re: [RFC][PATCH] irq_work

[Huang Ying <ying.huang@intel.com>]

On Fri, 2010-06-25 at 15:48 +0800, Peter Zijlstra wrote:
> On Fri, 2010-06-25 at 10:12 +0800, Huang Ying wrote:
> > 
> > It is better to add "void *data" field in this struct to allow same
> > function can be used for multiple struct irq_work. 
> 
> No, simply do:
> 
> struct my_foo {
>   struct irq_work work;
>   /* my extra data */
> }
> 
> void my_func(struct irq_work *work)
> {
>   struct my_foo *foo = container_of(work, struct my_foo, work);
> 
>   /* tada! */
> }

Yes. This works too. But Adding "void *data" field is helpful if you do
not embed struct irq_work into another struct.

> > And I think IRQ is the implementation detail here, so irq_work is
> > probably not a good name. nmi_return_notifier or nmi_callback is better?
> 
> Well, its ran in hard-irq context, so its an irq work. There's nothing
> that says it can only be used from NMI context.

It may be run in other contexts on some system (without APIC). And I
don't think it is useful to others except NMI handler. I think this is a
choice between naming after implementation and purpose.

> > > +void irq_work_run(void)
> > > +{
> > > +     struct irq_work *list;
> > > +
> > > +     list = xchg(&__get_cpu_var(irq_work_list), CALLBACK_TAIL);
> > > +     while (list != CALLBACK_TAIL) {
> > > +             struct irq_work *entry = list;
> > > +
> > > +             list = list->next;
> > > +             entry->func(entry);
> > > +
> > > +             entry->next = NULL;
> > 
> > entry->next = NULL should be put before entry->func(entry), so that we
> > will not lose a notification from NMI. And maybe check irq_work_list for
> > several times to make sure nothing is lost.
> 
> But then _sync() will return before its done executing.

We can use another flag to signify whether it is executing. For example
the bit 0 of entry->next.

> I think clearing after the function is done executing is the only sane
> semantics (and yes, I should fix the current perf code).
> 
> You can always miss an NMI since it can always happen before the
> callback gets done, and allowing another enqueue before the callback is
> complete is asking for trouble.

If we move entry->next = NULL before entry->func(entry), we will not
miss the NMI. Can you show how to miss it in this way?

> > > +             /*
> > > +              * matches the mb in cmpxchg() in irq_work_queue()
> > > +              */
> > > +             smp_wmb();
> > > +     }
> > > +}
> > 
> > I don't know why we need smp_wmb() here and smp_rmb() in
> > irq_work_pending(). The smp_<x>mb() in original perf_pending_xxx code is
> > not necessary too. Because smp_<x>mb is invoked in wake_up_process() and
> > __wait_event() already.
> 
> The smp_wmb() wants to be before ->next = NULL; so that all writes are
> completed before we release the entry. To that same effect _sync() and
> _queue need the (r)mb.

It is reasonable in this way.

Best Regards,
Huang Ying