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]

Message-ID: <1314863829.7945.9.camel@twins>
Date:	Thu, 01 Sep 2011 09:57:08 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Huang Ying <ying.huang@...el.com>
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

On Thu, 2011-09-01 at 09:46 +0800, Huang Ying wrote:

> You mean we should not use cpu_relax before the first cmpxchg? 

Yeah, that's just wasting time for no reason..

>  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();
>         }
> }

If you insist on having cpu_relax(), then yes that's lots better. Also
avoids the assignment in your conditional. Thing with cpu_relax() is
that its only beneficial in the highly contended case and degrade
light/un-contended loads.

Also, just noticed, why do you have different list_head/list_node
structures? They're the same, a single pointer.

> > 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?

Yes that's unfortunate, the current version would work just fine without
preempt but that's because of the this_cpu_* ops foo. 

Not sure it would make sense to add a special this_cpu_llist_add() or
so.. esp seeing that this_cpu_* is basically x86-only.
--
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/