From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751223AbaCWVeF (ORCPT ); Sun, 23 Mar 2014 17:34:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:21215 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750798AbaCWVeD (ORCPT ); Sun, 23 Mar 2014 17:34:03 -0400 Date: Sun, 23 Mar 2014 23:33:49 +0200 From: "Michael S. Tsirkin" To: Eric Dumazet Cc: Monam Agarwal , davem@davemloft.net, jasowang@redhat.com, xemul@parallels.com, wuzhy@linux.vnet.ibm.com, therbert@google.com, yamato@redhat.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, paulmck@linux.vnet.ibm.com Subject: Re: [PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c Message-ID: <20140323213349.GA8832@redhat.com> References: <1395599552-7486-1-git-send-email-monamagarwal123@gmail.com> <20140323194113.GA13633@redhat.com> <1395604457.9117.21.camel@edumazet-glaptop2.roam.corp.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1395604457.9117.21.camel@edumazet-glaptop2.roam.corp.google.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Mar 23, 2014 at 12:54:17PM -0700, Eric Dumazet wrote: > On Sun, 2014-03-23 at 21:41 +0200, Michael S. Tsirkin wrote: > > > The rcu_assign_pointer() ensures that the initialization of a structure > > is carried out before storing a pointer to that structure. > > In the case of the NULL pointer, there is no structure to initialize, > > so we can safely drop smp_wmb in this case. > > > > Signed-off-by: Michael S. Tsirkin > > > > -- > > > > Lightly tested. > > v is evaluated twice here but that should be ok since this > > only happens when v is a constant, so evaluating it should > > have no side effects. > > Paul, what do you think? > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > > index 72bf3a0..d33c9ec 100644 > > --- a/include/linux/rcupdate.h > > +++ b/include/linux/rcupdate.h > > @@ -587,7 +587,8 @@ static inline void rcu_preempt_sleep_check(void) > > */ > > #define rcu_assign_pointer(p, v) \ > > do { \ > > - smp_wmb(); \ > > + if (!__builtin_constant_p(v) || (v)) \ > > + smp_wmb(); \ > > ACCESS_ONCE(p) = RCU_INITIALIZER(v); \ > > } while (0) > > > > Yeah, I suggest you read d322f45ceed525daa changelog ;) > Oh I see. It does not seem hard to silence that warning though. See below. Alternatively apply these patches everywhere though it does look like too much work for too little gain to me. --> rcu: optimize rcu_assign_pointer with NULL The rcu_assign_pointer() dropped __builtin_constant_p check to avoid a compiler warning, but we can actually work around it without adding code. Signed-off-by: Michael S. Tsirkin --- Untested, too late here, sorry. diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h index 72bf3a0..9111d40 100644 --- a/include/linux/rcupdate.h +++ b/include/linux/rcupdate.h @@ -585,9 +585,14 @@ static inline void rcu_preempt_sleep_check(void) * please be careful when making changes to rcu_assign_pointer() and the * other macros that it invokes. */ +/* The convoluted __builtin_constant_p logic is here to prevent + * gcc from emitting a warning when passed a pointer to a variable. + */ #define rcu_assign_pointer(p, v) \ do { \ - smp_wmb(); \ + if (!__builtin_constant_p(v) || \ + (__builtin_constant_p(v) ? (v) : NULL)) \ + smp_wmb(); \ ACCESS_ONCE(p) = RCU_INITIALIZER(v); \ } while (0)