[PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c

[PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c

[Monam Agarwal]

This patch replaces rcu_assign_pointer(x, NULL) with RCU_INIT_POINTER(x, NULL)

The rcu_assign_pointer() ensures that the initialization of a structure       
is carried out before storing a pointer to that structure. 
And in the case of the NULL pointer, there is no structure to initialize. 
So, rcu_assign_pointer(p, NULL) can be safely converted to RCU_INIT_POINTER(p, NULL)

Signed-off-by: Monam Agarwal <monamagarwal123@gmail.com>
---
 drivers/net/tun.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 26f8635..ee328ba 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -452,7 +452,7 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
 
 		--tun->numqueues;
 		if (clean) {
-			rcu_assign_pointer(tfile->tun, NULL);
+			RCU_INIT_POINTER(tfile->tun, NULL);
 			sock_put(&tfile->sk);
 		} else
 			tun_disable_queue(tun, tfile);
@@ -499,12 +499,12 @@ static void tun_detach_all(struct net_device *dev)
 		tfile = rtnl_dereference(tun->tfiles[i]);
 		BUG_ON(!tfile);
 		wake_up_all(&tfile->wq.wait);
-		rcu_assign_pointer(tfile->tun, NULL);
+		RCU_INIT_POINTER(tfile->tun, NULL);
 		--tun->numqueues;
 	}
 	list_for_each_entry(tfile, &tun->disabled, next) {
 		wake_up_all(&tfile->wq.wait);
-		rcu_assign_pointer(tfile->tun, NULL);
+		RCU_INIT_POINTER(tfile->tun, NULL);
 	}
 	BUG_ON(tun->numqueues != 0);
 
@@ -2194,7 +2194,7 @@ static int tun_chr_open(struct inode *inode, struct file * file)
 					    &tun_proto);
 	if (!tfile)
 		return -ENOMEM;
-	rcu_assign_pointer(tfile->tun, NULL);
+	RCU_INIT_POINTER(tfile->tun, NULL);
 	tfile->net = get_net(current->nsproxy->net_ns);
 	tfile->flags = 0;
 	tfile->ifindex = 0;
-- 
1.7.9.5

Re: [PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c

[Michael S. Tsirkin]

On Mon, Mar 24, 2014 at 12:02:32AM +0530, Monam Agarwal wrote:
> This patch replaces rcu_assign_pointer(x, NULL) with RCU_INIT_POINTER(x, NULL)
> 
> The rcu_assign_pointer() ensures that the initialization of a structure       
> is carried out before storing a pointer to that structure. 
> And in the case of the NULL pointer, there is no structure to initialize. 
> So, rcu_assign_pointer(p, NULL) can be safely converted to RCU_INIT_POINTER(p, NULL)
> 
> Signed-off-by: Monam Agarwal <monamagarwal123@gmail.com>

Sounds right ... but doing this all over the place seems
fragile, and error prone. Can't we make this kind of
optimization automatic? See below:


> ---
>  drivers/net/tun.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 26f8635..ee328ba 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -452,7 +452,7 @@ static void __tun_detach(struct tun_file *tfile, bool clean)
>  
>  		--tun->numqueues;
>  		if (clean) {
> -			rcu_assign_pointer(tfile->tun, NULL);
> +			RCU_INIT_POINTER(tfile->tun, NULL);
>  			sock_put(&tfile->sk);
>  		} else
>  			tun_disable_queue(tun, tfile);
> @@ -499,12 +499,12 @@ static void tun_detach_all(struct net_device *dev)
>  		tfile = rtnl_dereference(tun->tfiles[i]);
>  		BUG_ON(!tfile);
>  		wake_up_all(&tfile->wq.wait);
> -		rcu_assign_pointer(tfile->tun, NULL);
> +		RCU_INIT_POINTER(tfile->tun, NULL);
>  		--tun->numqueues;
>  	}
>  	list_for_each_entry(tfile, &tun->disabled, next) {
>  		wake_up_all(&tfile->wq.wait);
> -		rcu_assign_pointer(tfile->tun, NULL);
> +		RCU_INIT_POINTER(tfile->tun, NULL);
>  	}
>  	BUG_ON(tun->numqueues != 0);
>  
> @@ -2194,7 +2194,7 @@ static int tun_chr_open(struct inode *inode, struct file * file)
>  					    &tun_proto);
>  	if (!tfile)
>  		return -ENOMEM;
> -	rcu_assign_pointer(tfile->tun, NULL);
> +	RCU_INIT_POINTER(tfile->tun, NULL);
>  	tfile->net = get_net(current->nsproxy->net_ns);
>  	tfile->flags = 0;
>  	tfile->ifindex = 0;
> -- 
> 1.7.9.5

--->

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 <mst@redhat.com>

--

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)
 

Re: [PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c

[Eric Dumazet]

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 <mst@redhat.com>
> 
> --
> 
> 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 ;)

Re: [PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c

[Michael S. Tsirkin]

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 <mst@redhat.com>
> > 
> > --
> > 
> > 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 <mst@redhat.com>

---

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)
 

Re: [PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c

[Paul E. McKenney]

On Sun, Mar 23, 2014 at 11:33:49PM +0200, Michael S. Tsirkin wrote:
> 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 <mst@redhat.com>
> > > 
> > > --
> > > 
> > > 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.

This would at the very least need to be tested under a wide variety
of compilers.  And we need to keep 

> 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 <mst@redhat.com>
> 
> ---
> 
> 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)) \

You lost me on this one.  If "v" is not a built-in constant, we want
the smp_wmb(), right?

							Thanx, Paul

> +			smp_wmb(); \
>  		ACCESS_ONCE(p) = RCU_INITIALIZER(v); \
>  	} while (0)
> 
> 

Re: [PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c

[Michael S. Tsirkin]

On Sun, Mar 23, 2014 at 03:12:56PM -0700, Paul E. McKenney wrote:
> On Sun, Mar 23, 2014 at 11:33:49PM +0200, Michael S. Tsirkin wrote:
> > 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 <mst@redhat.com>
> > > > 
> > > > --
> > > > 
> > > > 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.
> 
> This would at the very least need to be tested under a wide variety
> of compilers.

Seems an incredibly strict requirement for something that just
silences a warning.
What exactly should I test?
I intended to just verify this produces same code as before
d322f45ceed525daa under a recent gcc.

>  And we need to keep 
> 
> > 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 <mst@redhat.com>
> > 
> > ---
> > 
> > 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)) \
> 
> You lost me on this one.  If "v" is not a built-in constant, we want
> the smp_wmb(), right?

If "v" is not a built-in constant, then !__builtin_constant_p(v)
is true so (__builtin_constant_p(v) ? (v) : NULL))
is never evaluated.

Basically if (

	1.   !A ||
	2.   A ? B : C

If A  is false, only 1 is evaluated and the expression evaluates to true
If A  is true, then 2 evaluates to B.
C is never evaluated.

Makes sense? Did I miss anything?

But the effect as far as I can tell is that instead of converting v to
integer type we convert an expression involving v, so even though it's
able to figure out the value, gcc understands it's not a typo
and does not warn.


	

> 							Thanx, Paul
> 
> > +			smp_wmb(); \
> >  		ACCESS_ONCE(p) = RCU_INITIALIZER(v); \
> >  	} while (0)
> > 
> > 

Re: [PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c

[Eric Dumazet]

On Mon, 2014-03-24 at 07:09 +0200, Michael S. Tsirkin wrote:

> Seems an incredibly strict requirement for something that just
> silences a warning.
> What exactly should I test?
> I intended to just verify this produces same code as before
> d322f45ceed525daa under a recent gcc.

Thats because many rcu_assign_pointer(X, NULL) were already converted to
RCU_INIT_POINTER(X, NULL)

Quite frankly I don't know why you bother at all.

Adding back the lazy test in rcu_assign_pointer() doesn't help to make
the API cleaner and easier to understand.

People are usually using RCU API without really understanding
all the issues. They tend to add superfluous barriers because they feel
better. 

Having separate RCU_INIT_POINTER() and rcu_assign_pointer() serve as
better documentation of the code, I find it more easier to immediately
check what is going on while reviewing stuff.

Presumably, checkpatch.pl could be augmented to suggest to use
RCU_INIT_POINTER(X, NULL) instead of rcu_assign_pointer(X, NULL)

Re: [PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c

[Michael S. Tsirkin]

On Sun, Mar 23, 2014 at 10:25:27PM -0700, Eric Dumazet wrote:
> On Mon, 2014-03-24 at 07:09 +0200, Michael S. Tsirkin wrote:
> 
> > Seems an incredibly strict requirement for something that just
> > silences a warning.
> > What exactly should I test?
> > I intended to just verify this produces same code as before
> > d322f45ceed525daa under a recent gcc.
> 
> Thats because many rcu_assign_pointer(X, NULL) were already converted to
> RCU_INIT_POINTER(X, NULL)
> 
> Quite frankly I don't know why you bother at all.
> 
> Adding back the lazy test in rcu_assign_pointer() doesn't help to make
> the API cleaner and easier to understand.
> 
> People are usually using RCU API without really understanding
> all the issues. They tend to add superfluous barriers because they feel
> better. 

Cute.  This is exactly what d322f45ceed525daa did actually -
made the barrier unconditional even when not needed.

> Having separate RCU_INIT_POINTER() and rcu_assign_pointer() serve as
> better documentation of the code, I find it more easier to immediately
> check what is going on while reviewing stuff.
> 
> Presumably, checkpatch.pl could be augmented to suggest to use
> RCU_INIT_POINTER(X, NULL) instead of rcu_assign_pointer(X, NULL)
> 
> 


What happens if someone then changes that NULL to something else?
Things will start to break in subtle way, won't they?

To me RCU_INIT_POINTER seems to say "safe to use when initializing
pointer field when no one can access the structure".
The patch that started it all changed a path that clearly
does not satisfy this: it is mutating a field not initializing
it before use. After looking at the implementation, it does
seem safe.  So if some people actually like this API, I don't mind.
A matter of taste I guess.

If someone still wants to make rcu_assign_pointer more optimal, without
a warning, I see a cleaner way to do this now, below.
Lightly tested - if someone sees value in this but requires more testing, let me know,
if no one responds I'll just drop the whole thing.

--->

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
using an inline wrapper, without adding code.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 72bf3a0..0d45b6d 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -585,9 +585,18 @@ 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 inline wrapper is here to prevent gcc from emitting a warning when
+ * passed a pointer to a variable.
+ */
+static inline _rcu_safe_smp_wmb_unless_null(const void *v)
+{
+	if (!__builtin_constant_p(v) || ((v) != NULL))
+		smp_wmb();
+}
+
 #define rcu_assign_pointer(p, v) \
 	do { \
-		smp_wmb(); \
+		_rcu_safe_smp_wmb_unless_null((__force const void *)(v)); \
 		ACCESS_ONCE(p) = RCU_INITIALIZER(v); \
 	} while (0)
 

Re: [PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c

[Lai Jiangshan]

On 03/24/2014 01:25 PM, Eric Dumazet wrote:
> On Mon, 2014-03-24 at 07:09 +0200, Michael S. Tsirkin wrote:
> 
>> Seems an incredibly strict requirement for something that just
>> silences a warning.
>> What exactly should I test?
>> I intended to just verify this produces same code as before
>> d322f45ceed525daa under a recent gcc.
> 
> Thats because many rcu_assign_pointer(X, NULL) were already converted to
> RCU_INIT_POINTER(X, NULL)
> 
> Quite frankly I don't know why you bother at all.
> 
> Adding back the lazy test in rcu_assign_pointer() doesn't help to make
> the API cleaner and easier to understand.
> 
> People are usually using RCU API without really understanding
> all the issues. They tend to add superfluous barriers because they feel
> better. 
> 
> Having separate RCU_INIT_POINTER() and rcu_assign_pointer() serve as
> better documentation of the code, I find it more easier to immediately
> check what is going on while reviewing stuff.
> 
> Presumably, checkpatch.pl could be augmented to suggest to use
> RCU_INIT_POINTER(X, NULL) instead of rcu_assign_pointer(X, NULL)


I prefer rcu_assign_pointer(X, NULL) than RCU_INIT_POINTER(X, NULL),
NULL should not be a special pointer value to the users of RCU.

the RCU implements should hide the difference if RCU implements
differentiate the values for optimization.

RCU_INIT_POINTER() sounds as an initialization-stage API. If we need
something different for NULL pointer, I prefer
rcu_assign_*null*_pointer().

rcu_assign_pointer(X, NULL) implies compiler barrier(), but
RCU_INIT_POINTER(X, NULL) doesn't.

> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c

[Michael S. Tsirkin]

On Mon, Mar 24, 2014 at 08:22:25AM +0200, Michael S. Tsirkin wrote:
> On Sun, Mar 23, 2014 at 10:25:27PM -0700, Eric Dumazet wrote:
> > On Mon, 2014-03-24 at 07:09 +0200, Michael S. Tsirkin wrote:
> > 
> > > Seems an incredibly strict requirement for something that just
> > > silences a warning.
> > > What exactly should I test?
> > > I intended to just verify this produces same code as before
> > > d322f45ceed525daa under a recent gcc.
> > 
> > Thats because many rcu_assign_pointer(X, NULL) were already converted to
> > RCU_INIT_POINTER(X, NULL)
> > 
> > Quite frankly I don't know why you bother at all.
> > 
> > Adding back the lazy test in rcu_assign_pointer() doesn't help to make
> > the API cleaner and easier to understand.
> > 
> > People are usually using RCU API without really understanding
> > all the issues. They tend to add superfluous barriers because they feel
> > better. 
> 
> Cute.  This is exactly what d322f45ceed525daa did actually -
> made the barrier unconditional even when not needed.
> 
> > Having separate RCU_INIT_POINTER() and rcu_assign_pointer() serve as
> > better documentation of the code, I find it more easier to immediately
> > check what is going on while reviewing stuff.
> > 
> > Presumably, checkpatch.pl could be augmented to suggest to use
> > RCU_INIT_POINTER(X, NULL) instead of rcu_assign_pointer(X, NULL)
> > 
> > 
> 
> 
> What happens if someone then changes that NULL to something else?
> Things will start to break in subtle way, won't they?
> 
> To me RCU_INIT_POINTER seems to say "safe to use when initializing
> pointer field when no one can access the structure".
> The patch that started it all changed a path that clearly
> does not satisfy this: it is mutating a field not initializing
> it before use. After looking at the implementation, it does
> seem safe.  So if some people actually like this API, I don't mind.
> A matter of taste I guess.
> 
> If someone still wants to make rcu_assign_pointer more optimal, without
> a warning, I see a cleaner way to do this now, below.
> Lightly tested - if someone sees value in this but requires more testing, let me know,
> if no one responds I'll just drop the whole thing.
> 
> --->
> 
> 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
> using an inline wrapper, without adding code.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ---
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 72bf3a0..0d45b6d 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -585,9 +585,18 @@ 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 inline wrapper is here to prevent gcc from emitting a warning when
> + * passed a pointer to a variable.
> + */
> +static inline _rcu_safe_smp_wmb_unless_null(const void *v)

doh should be static inline void


anyway, I'll go away now

> +{
> +	if (!__builtin_constant_p(v) || ((v) != NULL))
> +		smp_wmb();
> +}
> +
>  #define rcu_assign_pointer(p, v) \
>  	do { \
> -		smp_wmb(); \
> +		_rcu_safe_smp_wmb_unless_null((__force const void *)(v)); \
>  		ACCESS_ONCE(p) = RCU_INITIALIZER(v); \
>  	} while (0)
>  

Re: [PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c

[Eric Dumazet]

On Mon, 2014-03-24 at 08:22 +0200, Michael S. Tsirkin wrote:

> What happens if someone then changes that NULL to something else?
> Things will start to break in subtle way, won't they?

Sure. As anything else can break when/if using wrong API or in any
change. Particularly in RCU protected code. Even a kzalloc() can be
buggy.

Note that smp_wmb() is hardly expensive in all these paths,
so thats why I do not care anymore asking guys using RCU_INIT_POINTER(x,
NULL) instead of rcu_assign_pointer() as I used to.

When doing tree wide change (like in commit 2cfa5a0471fef43), I usually
not do a dumb one, I take the time to read the code and check it for
defects.

I do not remember what kind of compiler warnings we had 3 years ago,
But I do not want to spend more cycles on this subject...

Re: [PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c

[Paul E. McKenney]

On Mon, Mar 24, 2014 at 04:47:32PM +0800, Lai Jiangshan wrote:
> On 03/24/2014 01:25 PM, Eric Dumazet wrote:
> > On Mon, 2014-03-24 at 07:09 +0200, Michael S. Tsirkin wrote:
> > 
> >> Seems an incredibly strict requirement for something that just
> >> silences a warning.
> >> What exactly should I test?
> >> I intended to just verify this produces same code as before
> >> d322f45ceed525daa under a recent gcc.
> > 
> > Thats because many rcu_assign_pointer(X, NULL) were already converted to
> > RCU_INIT_POINTER(X, NULL)
> > 
> > Quite frankly I don't know why you bother at all.
> > 
> > Adding back the lazy test in rcu_assign_pointer() doesn't help to make
> > the API cleaner and easier to understand.
> > 
> > People are usually using RCU API without really understanding
> > all the issues. They tend to add superfluous barriers because they feel
> > better. 
> > 
> > Having separate RCU_INIT_POINTER() and rcu_assign_pointer() serve as
> > better documentation of the code, I find it more easier to immediately
> > check what is going on while reviewing stuff.
> > 
> > Presumably, checkpatch.pl could be augmented to suggest to use
> > RCU_INIT_POINTER(X, NULL) instead of rcu_assign_pointer(X, NULL)
> 
> 
> I prefer rcu_assign_pointer(X, NULL) than RCU_INIT_POINTER(X, NULL),
> NULL should not be a special pointer value to the users of RCU.
> 
> the RCU implements should hide the difference if RCU implements
> differentiate the values for optimization.
> 
> RCU_INIT_POINTER() sounds as an initialization-stage API. If we need
> something different for NULL pointer, I prefer
> rcu_assign_*null*_pointer().

Let's keep what we have for a year or so, and then see how things look at
that point.  A really easy Coccinelle script will make the needed changes,
so we aren't losing anything by waiting.  And who knows, perhaps someone
will come up with a clever idea in that time.

> rcu_assign_pointer(X, NULL) implies compiler barrier(), but
> RCU_INIT_POINTER(X, NULL) doesn't.

Good point!  I don't believe that the current docbook mentions this,
will fix.  And you are right, this is a good argument for maintaining
a separate API for NULL-pointer assignment rather than making
rcu_assign_pointer() sometimes do the smp_wmb() and sometimes not.
With the current approach, you can count on rcu_assign_pointer()
always implying a memory barrier.

Also, one thing I forgot earlier, rcu_assign_pointer() now uses
smp_store_release() rather than smp_wmb().

							Thanx, Paul

> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> > 
> 

Re: [PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c

[David Miller]

From: Monam Agarwal <monamagarwal123@gmail.com>
Date: Mon, 24 Mar 2014 00:02:32 +0530

> This patch replaces rcu_assign_pointer(x, NULL) with RCU_INIT_POINTER(x, NULL)
> 
> The rcu_assign_pointer() ensures that the initialization of a structure       
> is carried out before storing a pointer to that structure. 
> And in the case of the NULL pointer, there is no structure to initialize. 
> So, rcu_assign_pointer(p, NULL) can be safely converted to RCU_INIT_POINTER(p, NULL)
> 
> Signed-off-by: Monam Agarwal <monamagarwal123@gmail.com>

Applied.