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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 01 Sep 2011 09:46:49 +0800
From:	Huang Ying <ying.huang@...el.com>
To:	Peter Zijlstra <peterz@...radead.org>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH -mm 1/2] irq_work, Use llist in irq_work

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@...el.com>
>> Cc: Peter Zijlstra <peterz@...radead.org>
>> ---
>>  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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/