Re: [PATCH -mm 1/2] irq_work, Use llist in irq_work

From: Huang Ying
Date: Wed Aug 31 2011 - 21:46:55 EST


Hi, Peter,

Thanks for your comments.

On 08/31/2011 06:10 PM, Peter Zijlstra wrote:
> On Tue, 2011-08-30 at 13:16 +0800, Huang Ying wrote:
>> Use llist in irq_work instead of the lock-less linked list
>> implementation in irq_work to avoid the code duplication.
>
> Except you make code horrid as well.. both this and xlist don't have
> additional function calls, whereas you do.
>
> Also, WTFH do you have unconditinoal cpu_relax() calls inside the
> cmpxchg() loops, that's just bloody insane.

You mean we should not use cpu_relax before the first cmpxchg? You
suggest something as follow?

void llist_add(struct llist_node *new, struct llist_head *head)
{
struct llist_node *entry, *old_entry;

#ifndef CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG
BUG_ON(in_nmi());
#endif

entry = head->first;
for (;;) {
old_entry = entry;
new->next = entry;
entry = cmpxchg(&head->first, old_entry, new);
if (entry == old_entry)
break;
cpu_relax();
}
}

> Move all of lib/llist.c inline, create a new macro for the
>
> #ifndef CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG
> BUG_ON(in_nmi());
> #endif
>
> blurb and loose the LLIST Kconfig.

OK. I will do that.

>> Signed-off-by: Huang Ying <ying.huang@xxxxxxxxx>
>> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>> ---
>> include/linux/irq_work.h | 15 ++++---
>> init/Kconfig | 1
>> kernel/irq_work.c | 92 ++++++++++++++++++-----------------------------
>> 3 files changed, 47 insertions(+), 61 deletions(-)
>>
>> --- a/include/linux/irq_work.h
>> +++ b/include/linux/irq_work.h
>> @@ -1,20 +1,23 @@
>> #ifndef _LINUX_IRQ_WORK_H
>> #define _LINUX_IRQ_WORK_H
>>
>> +#include <linux/llist.h>
>> +
>> struct irq_work {
>> - struct irq_work *next;
>> + unsigned long flags;
>> + struct llist_node llnode;
>> void (*func)(struct irq_work *);
>> };
>
> Separating out the flags is unfortunate, but ok.
>
>
>> +#define LIST_NONEMPTY_BIT 0
>
> This is just sad, see below.
>
>> -static inline struct irq_work *next_flags(struct irq_work *entry, int flags)
>> -{
>> - unsigned long next = (unsigned long)entry;
>> - next |= flags;
>> - return (struct irq_work *)next;
>> -}
>> +struct irq_work_list {
>> + unsigned long flags;
>> + struct llist_head llist;
>> +};
>
> which is superfluous
>
>
>> @@ -77,23 +62,19 @@ void __weak arch_irq_work_raise(void)
>> /*
>> * Queue the entry and raise the IPI if needed.
>> */
>> -static void __irq_work_queue(struct irq_work *entry)
>> +static void __irq_work_queue(struct irq_work *work)
>> {
>> - struct irq_work *next;
>> + struct irq_work_list *irq_work_list;
>>
>> - preempt_disable();
>> + irq_work_list = &get_cpu_var(irq_work_lists);
>>
>> - do {
>> - next = __this_cpu_read(irq_work_list);
>> - /* Can assign non-atomic because we keep the flags set. */
>> - entry->next = next_flags(next, IRQ_WORK_FLAGS);
>> - } while (this_cpu_cmpxchg(irq_work_list, next, entry) != next);
>> + llist_add(&work->llnode, &irq_work_list->llist);
>>
>> /* The list was empty, raise self-interrupt to start processing. */
>> - if (!irq_work_next(entry))
>> + if (!test_and_set_bit(LIST_NONEMPTY_BIT, &irq_work_list->flags))
>> arch_irq_work_raise();
>
> So why can't you simply test work->llnode->next?

Yes. That is better. Even if there may be a small race window, it is
not a big issue to raise one extra self interrupt seldom.

> and loose the get/put
> cpu muck? The existing preempt_disable/enable() are already superfluous
> and could be removed, you just made all this way more horrid than need
> be.

Will it cause race condition to remove preempt_disable/enable?
Considering something as follow:

- get irq_work_list of CPU A
- queue irq_work into irq_work_list of CPU A
- preempted and resumed execution on CPU B
- arch_irq_work_raise on CPU B

irq_work_run on CPU B will do nothing. While irq_work need to wait for
next timer interrupt. Isn't it an issue?

Best Regards,
Huang Ying
--
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/