Re: [PATCH] Allow lockless traversal of notifier lists

[Andi Kleen <ak@suse.de>]

On Mon, Nov 28, 2005 at 09:31:29PM +0530, Dipankar Sarma wrote:
> On Mon, Nov 28, 2005 at 02:37:57PM +0100, Andi Kleen wrote:
> > 
> >   *
> >   *	Currently always returns zero.
> >   */
> > @@ -116,6 +119,7 @@
> >  		list= &((*list)->next);
> >  	}
> >  	n->next = *list;
> > +	wmb();
> >  	*list=n;
> >  	write_unlock(&notifier_lock);
> 
> Shouldn't this be smp_wmb() ?

Yes.

> 
> Also, not all archs have strong ordering for data dependent reads.

Ah you mean the Alpha exception? Right Alpha would need that.

Thanks for the review.

-Andi

Revised patch with Alphaextrawurst.

----
As discussed in other thread.
 
Just needed an additional write barrier, so that a parallel
running lockup can never see inconsistent state. As long as there
is no unregistration or the unregistration is done using
locking or RCU in the caller they should be ok now.

This only makes a difference on non i386/x86-64 architectures.
x86 was already ok because it never reorders writes.


diff -u linux-2.6.15rc2-work/kernel/sys.c-o linux-2.6.15rc2-work/kernel/sys.c
--- linux-2.6.15rc2-work/kernel/sys.c-o	2005-11-16 00:34:33.000000000 +0100
+++ linux-2.6.15rc2-work/kernel/sys.c	2005-11-28 17:03:22.000000000 +0100
@@ -102,6 +102,9 @@
  *	@n: New entry in notifier chain
  *
  *	Adds a notifier to a notifier chain.
+ *	As long as unregister is not used this is safe against parallel
+ *	lockless notifier lookups. If unregister is used then unregister
+ *	needs to do additional locking or use RCU.
  *
  *	Currently always returns zero.
  */
@@ -116,6 +119,7 @@
 		list= &((*list)->next);
 	}
 	n->next = *list;
+	smp_wmb();
 	*list=n;
 	write_unlock(&notifier_lock);
 	return 0;
@@ -129,6 +133,8 @@
  *	@n: New entry in notifier chain
  *
  *	Removes a notifier from a notifier chain.
+ *	Note this needs additional locking or RCU in the caller to be safe
+ * 	against parallel traversals.
  *
  *	Returns zero on success, or %-ENOENT on failure.
  */
@@ -175,6 +181,7 @@
 
 	while(nb)
 	{
+		smp_read_barrier_depends();
 		ret=nb->notifier_call(nb,val,v);
 		if(ret&NOTIFY_STOP_MASK)
 		{