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

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

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