Re: [PATCH] net: remove superfluous call to synchronize_net()

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Thu, Apr 16, 2009 at 07:40:23AM +0200, Eric Dumazet wrote:
> Paul E. McKenney a écrit :
> > On Wed, Apr 15, 2009 at 05:38:06PM +0200, Eric Dumazet wrote:
> >> inet_register_protosw() is adding inet_protosw to inetsw[] with appropriate
> >> locking section and rcu variant. No need to call synchronize_net() to wait
> >> for a RCU grace period. Changes are immediatly visible to other cpus anyway.
> > 
> > I agree with the conclusion (that this change is safe), but not with
> > the reasoning process.  ;-)
> > 
> > The reason that this change is safe is that any inter-process
> > communication mechanism used to tell other CPUs that this protocol has
> > been registered must contain relevant memory barriers, otherwise, that
> > mechanism won't be reliable.
> 
> But my patch is not fixing some unreliable algo. It is already reliable,
> but pessimistic since containing a superflous call to not-related function.
> 
> > If an unreliable mechanism was to be used, the other CPU might not yet see
> > the protocol.  For example, if the caller did a simple non-atomic store
> > to a variable that the other CPU accessed with a simple non-atomic load,
> > then that other CPU could potentially see the inetsw[] without the new
> > protocol, given that inet_create() is lockless.  Unlikely, but possible.
> 
> Well, this reasoning process is a litle it wrong too ;)
> store or loads of the pointer are always atomic.
> You probably meant to say that the store had to be done when memory state
> is stable and committed by the processor doing the _register() thing.

They are indeed atomic, but not necessarily ordered.  So if you did
something like:

	if (flag)
		operation_needing_protocol();

Then it is possible for things to get re-ordered so that the
operation_needing_protocol() doesn't see the newly registered protocol.

> > But if a proper inter-process communication mechanism is used to inform
> > the other CPU, then the first CPU's memory operations will be seen.
> > 
> > So I suggest a comment to this effect.
> 
> Yes, I should really take special attention to ChangeLogs :)

;-)

> Thanks a lot Patrick
> 
> [PATCH] net: remove superfluous call to synchronize_net()
> 
> inet_register_protosw() function is responsible for adding a new
> inet protocol into a global table (inetsw[]) that is used with RCU rules.
> 
> As soon as the store of the pointer is done, other cpus might see
> this new protocol in inetsw[], so we have to make sure new protocol
> is ready for use. All pending memory updates should thus be committed
> to memory before setting the pointer.
> This is correctly done using rcu_assign_pointer()
> 
> synchronize_net() is typically used at unregister time, after
> unsetting the pointer, to make sure no other cpu is still using
> the object we want to dismantle. Using it at register time
> is only adding an artificial delay that could hide a real bug,
> and this bug could popup if/when synchronize_rcu() can proceed
> faster than now.

Actually, if you make a change, then do a synchronize_rcu(), then use
-any- interprocess communications mechanism, safe or not, that causes
an RCU read-side critical section to execute, then that RCU read-side
critical section is guaranteed to see the change.

But if you restrict yourself to safe communication mechanisms that
maintain ordering (locking, atomic operations that return values, POSIX
primitives, ...), then you don't need the synchronize_rcu().

Yes, I am being pedantic, but then again, I am the guy who would have
to straighten out any later confusion.  ;-)

						Thanx, Paul

> This saves about 13 ms on boot time on a HZ=1000 8 cpus machine  ;) 
> (4 calls to inet_register_protosw(), and about 3200 us per call)
> 
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>
> 
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 7f03373..1706896 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1003,8 +1003,6 @@ void inet_register_protosw(struct inet_protosw *p)
>  out:
>  	spin_unlock_bh(&inetsw_lock);
>  
> -	synchronize_net();
> -
>  	return;
>  
>  out_permanent:
>