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

[Eric Dumazet <dada1@cosmosbay.com>]

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.

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

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: