Messages in this thread | | | Subject | Re: [RFC][PATCH] irq_work | From | Huang Ying <> | Date | Fri, 25 Jun 2010 10:12:43 +0800 |
| |
On Thu, 2010-06-24 at 14:35 +0800, Peter Zijlstra wrote: > Something like this, but filled out with some arch code that does the > self-ipi and calls irq_work_run() should do. > > No need to molest the softirq code, no need for limited vectors of any > kind.
Now, as far as my understanding goes, hard IRQ based solution is acceptable for everyone.
Ingo and Andi,
Do you agree?
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl> > --- > include/linux/irq_callback.h | 13 ++++++++ > kernel/irq_callback.c | 66 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 79 insertions(+) > > Index: linux-2.6/include/linux/irq_callback.h > =================================================================== > --- /dev/null > +++ linux-2.6/include/linux/irq_callback.h > @@ -0,0 +1,13 @@ > +#ifndef _LINUX_IRQ_CALLBACK_H > +#define _LINUX_IRQ_CALLBACK_H > + > +struct irq_work { > + struct irq_work *next; > + void (*func)(struct irq_work *); > +};
It is better to add "void *data" field in this struct to allow same function can be used for multiple struct irq_work.
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?
> +int irq_work_queue(struct irq_work *entry, void (*func)(struct irq_work *)); > +void irq_work_run(void); > +void irq_work_sync(struct irq_work *entry); > + > +#endif /* _LINUX_IRQ_CALLBACK_H */ > Index: linux-2.6/kernel/irq_callback.c > =================================================================== > --- /dev/null > +++ linux-2.6/kernel/irq_callback.c > @@ -0,0 +1,66 @@ > + > +#include <linux/irq_callback.h> > + > +#define CALLBACK_TAIL ((struct irq_work *)-1UL) > + > +static DEFINE_PER_CPU(struct irq_work *, irq_work_list) = { > + CALLBACK_TAIL, > +}; > + > +int irq_work_queue(struct irq_work *entry, void (*func)(struct irq_work *)) > +{ > + struct irq_work **head; > + > + if (cmpxchg(&entry->next, NULL, CALLBACK_TAIL) != NULL) > + return 0; > + > + entry->func = func; > + > + head = &get_cpu_var(irq_work_list); > + > + do { > + entry->next = *head; > + } while (cmpxchg(head, entry->next, entry) != entry->next); > + > + if (entry->next == CALLBACK_TAIL) > + arch_self_ipi(); > + > + put_cpu_var(irq_work_list); > + return 1; > +} > + > +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.
> + /* > + * 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.
> +static int irq_work_pending(struct irq_work *entry) > +{ > + /* > + * matches the wmb in irq_work_run > + */ > + smp_rmb(); > + return entry->next != NULL; > +} > + > +void irq_work_sync(struct irq_work *entry) > +{ > + WARN_ON_ONCE(irqs_disabled()); > + > + while (irq_work_pending(entry)) > + cpu_relax(); > +}
If we move entry->next = NULL earlier in irq_work_run(), we need another flag to signify the entry->func is running here.
Best Regards, Huang Ying
| |