From patchwork Thu Apr 16 05:40:23 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Eric Dumazet X-Patchwork-Id: 26047 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by ozlabs.org (Postfix) with ESMTP id 9C043DE230 for ; Thu, 16 Apr 2009 15:40:41 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752095AbZDPFkg (ORCPT ); Thu, 16 Apr 2009 01:40:36 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751916AbZDPFkg (ORCPT ); Thu, 16 Apr 2009 01:40:36 -0400 Received: from gw1.cosmosbay.com ([212.99.114.194]:45271 "EHLO gw1.cosmosbay.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751821AbZDPFkf convert rfc822-to-8bit (ORCPT ); Thu, 16 Apr 2009 01:40:35 -0400 Received: from [127.0.0.1] (localhost [127.0.0.1]) by gw1.cosmosbay.com (8.13.7/8.13.7) with ESMTP id n3G5eOBH014094; Thu, 16 Apr 2009 07:40:24 +0200 Message-ID: <49E6C4C7.3050105@cosmosbay.com> Date: Thu, 16 Apr 2009 07:40:23 +0200 From: Eric Dumazet User-Agent: Thunderbird 2.0.0.21 (Windows/20090302) MIME-Version: 1.0 To: paulmck@linux.vnet.ibm.com CC: "David S. Miller" , Linux Netdev List Subject: Re: [PATCH] net: remove superfluous call to synchronize_net() References: <49E5FF5E.50409@cosmosbay.com> <20090415215454.GU6766@linux.vnet.ibm.com> In-Reply-To: <20090415215454.GU6766@linux.vnet.ibm.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-1.6 (gw1.cosmosbay.com [0.0.0.0]); Thu, 16 Apr 2009 07:40:25 +0200 (CEST) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org 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 --- 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 --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: