Messages in this thread | | | Subject | Re: [PATCH -mm 1/2] irq_work, Use llist in irq_work | From | Peter Zijlstra <> | Date | Thu, 01 Sep 2011 09:57:08 +0200 |
| |
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.
| |