diff mbox

net: remove superfluous call to synchronize_net()

Message ID 49E6C4C7.3050105@cosmosbay.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet April 16, 2009, 5:40 a.m. UTC 
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>


--
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 16, 2009, 3:52 p.m. UTC | #1 
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:
> 
--
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
Eric Dumazet April 16, 2009, 4:03 p.m. UTC | #2 
Paul E. McKenney a écrit :
> 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.  ;-)
> 

OK :)

I suggest applying patch as is, and consider adding a paragraph in Documentation
eventually, if you feel a clarification is needed on the subject ?

Thank you

--
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
Paul E. McKenney April 16, 2009, 6:02 p.m. UTC | #3 
On Thu, Apr 16, 2009 at 06:03:55PM +0200, Eric Dumazet wrote:
> Paul E. McKenney a écrit :
> > 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.  ;-)
> > 
> 
> OK :)
> 
> I suggest applying patch as is, and consider adding a paragraph in Documentation
> eventually, if you feel a clarification is needed on the subject ?

Please add a comment where the synchronize_rcu() used to be explaining why
it is not needed.  The poor slob who copies your code isn't going to read
theh Documentation/RCU, he is just going to expect it to magically work.

With the synchronize_rcu(), it does just magically work.  Without the
synchronize_rcu(), you have to be careful.  Therefore, please add the
comment saying that care is required.

							Thanx, Paul
--
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
Eric Dumazet April 16, 2009, 6:43 p.m. UTC | #4 
Paul E. McKenney a écrit :
> 
> Please add a comment where the synchronize_rcu() used to be explaining why
> it is not needed.  The poor slob who copies your code isn't going to read
> theh Documentation/RCU, he is just going to expect it to magically work.
> 
> With the synchronize_rcu(), it does just magically work.  Without the
> synchronize_rcu(), you have to be careful.  Therefore, please add the
> comment saying that care is required.
> 

Sorry Paul, I dont understand why I should put a comment to say :

/*
 * Dont need to use synchronize_net() or call_rcu() or msleep(100) or
 * whatever function here because bla bla ...
 */

We could add this comment in about 99% of all functions in linux kernel ;)

I checked inet6_register_protosw(struct inet_protosw *p)
and it doesnt have this synchronize_rcu() neither the comment you advise...

Following construct is obvious and should not be commented in code itself.

spin_lock_bh(&somelock);
list_for_each(..., ...) {
	if (some_condition) {
		list_add_rcu(..., ...)
or		rcu_assign_pointer(...) 
		break;
	}
}
spin_unlock_bh(&somelock);

If it is not obvious, then it should be documented once in Documentation/RCU, since
we find hundred of similar code in kernel.

On the contrary, places where we *use* synchronize_{rcu|net}() should get a comment
to explain why this is really necessary since this function can be a real problem.

Thanks

--
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
David Miller April 17, 2009, 11:56 a.m. UTC | #5 
From: Eric Dumazet <dada1@cosmosbay.com>
Date: Thu, 16 Apr 2009 07:40:23 +0200

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

I think this change is fine, so I'm adding it to net-next-2.6

If you guys want to continue discussing the merits of putting
comments in every spot where we lack a RCU sync call, that's
your call. :-)

--
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
Paul E. McKenney April 17, 2009, 7:25 p.m. UTC | #6 
On Fri, Apr 17, 2009 at 04:56:31AM -0700, David Miller wrote:
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Thu, 16 Apr 2009 07:40:23 +0200
> 
> > [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>
> 
> I think this change is fine, so I'm adding it to net-next-2.6
> 
> If you guys want to continue discussing the merits of putting
> comments in every spot where we lack a RCU sync call, that's
> your call. :-)

Your Honor, I plead guilty to charges as read.  ;-)

							Thanx, Paul
--
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: