diff mbox

net: remove superfluous call to synchronize_net()

Message ID 49E5FF5E.50409@cosmosbay.com
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet April 15, 2009, 3:38 p.m. UTC
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.

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)

But more seriously, we should audit all synchronize_{rcu|net}() calls
to make sure we dont waste time and hide some bugs because of artificial
delays.

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Paul E. McKenney April 15, 2009, 9:54 p.m. UTC | #1
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.

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.

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.

> 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)
> 
> But more seriously, we should audit all synchronize_{rcu|net}() calls
> to make sure we dont waste time and hide some bugs because of artificial
> delays.

Good point!

							Thanx, Paul

> 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:
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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: