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

[Eric Dumazet <dada1@cosmosbay.com>]

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