Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1391808 > unrolled thread

[PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing.

Back to article view | Back to linux.kernel

Contents

  [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing. Muhammad Falak R Wani <falakreyaz@gmail.com> - 2016-05-01 15:00 +0200
    Re: [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing. Christoph Hellwig <hch@infradead.org> - 2016-05-01 19:10 +0200
      Re: [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing. "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-05-01 22:00 +0200
        Re: [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing. "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-05-02 16:20 +0200
          Re: [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing. "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-05-02 20:00 +0200
            Re: [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing. Christoph Hellwig <hch@infradead.org> - 2016-05-02 20:10 +0200
              Re: [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing. "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-05-02 20:10 +0200
                Re: [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing. Christoph Hellwig <hch@infradead.org> - 2016-05-03 10:50 +0200
                  Re: [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing. "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-05-03 15:10 +0200

#1391808 — [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing.

It is safe to use RCU_INIT_POINTER() to NULL, instead of
rcu_assign_pointer().
This results in slightly smaller/faster code.

Signed-off-by: Muhammad Falak R Wani <falakreyaz@gmail.com>
---
 drivers/target/target_core_tpg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/target_core_tpg.c b/drivers/target/target_core_tpg.c
index ddf0460..7ae0f08 100644
--- a/drivers/target/target_core_tpg.c
+++ b/drivers/target/target_core_tpg.c
@@ -684,7 +684,7 @@ void core_tpg_remove_lun(
 		spin_lock(&dev->se_port_lock);
 		list_del(&lun->lun_dev_link);
 		dev->export_count--;
-		rcu_assign_pointer(lun->lun_se_dev, NULL);
+		RCU_INIT_POINTER(lun->lun_se_dev, NULL);
 		spin_unlock(&dev->se_port_lock);
 	}
 	if (!(dev->se_hba->hba_flags & HBA_FLAGS_INTERNAL_USE))
-- 
1.9.1

[toc] | [next] | [standalone]

#1391852

On Sun, May 01, 2016 at 06:22:01PM +0530, Muhammad Falak R Wani wrote:
> It is safe to use RCU_INIT_POINTER() to NULL, instead of
> rcu_assign_pointer().
> This results in slightly smaller/faster code.

If this is indeed the case, rcu_assign_pointer should simply check
for NULL using __builtin_constant_p and do the right thing transparently
instead of burdening it on every user.

[toc] | [prev] | [next] | [standalone]

#1391861

On Sun, May 01, 2016 at 10:01:20AM -0700, Christoph Hellwig wrote:
> On Sun, May 01, 2016 at 06:22:01PM +0530, Muhammad Falak R Wani wrote:
> > It is safe to use RCU_INIT_POINTER() to NULL, instead of
> > rcu_assign_pointer().
> > This results in slightly smaller/faster code.
> 
> If this is indeed the case, rcu_assign_pointer should simply check
> for NULL using __builtin_constant_p and do the right thing transparently
> instead of burdening it on every user.

Last time around, there was a compiler bug that prevented this from
working correctly.  But it could well be time to look at it again.
How does the following (untested) patch look?

							Thanx, Paul

------------------------------------------------------------------------

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index c61b6b9506e7..3a4dbfe63c1a 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -650,7 +650,16 @@ 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.
  */
-#define rcu_assign_pointer(p, v) smp_store_release(&p, RCU_INITIALIZER(v))
+#define rcu_assign_pointer(p, v) \
+({ \
+	typeof(v) _r_a_p__v = (v); \
+	\
+	if (__builtin_constant_p(v) && (_r_a_p__v) == NULL) \
+		WRITE_ONCE((p), (_r_a_p__v)); \
+	else \
+		smp_store_release(&p, RCU_INITIALIZER(_r_a_p__v)); \
+	_r_a_p__v; \
+})
 
 /**
  * rcu_access_pointer() - fetch RCU pointer with no dereferencing

[toc] | [prev] | [next] | [standalone]

#1392233

On Sun, May 01, 2016 at 12:53:13PM -0700, Paul E. McKenney wrote:
> On Sun, May 01, 2016 at 10:01:20AM -0700, Christoph Hellwig wrote:
> > On Sun, May 01, 2016 at 06:22:01PM +0530, Muhammad Falak R Wani wrote:
> > > It is safe to use RCU_INIT_POINTER() to NULL, instead of
> > > rcu_assign_pointer().
> > > This results in slightly smaller/faster code.
> > 
> > If this is indeed the case, rcu_assign_pointer should simply check
> > for NULL using __builtin_constant_p and do the right thing transparently
> > instead of burdening it on every user.
> 
> Last time around, there was a compiler bug that prevented this from
> working correctly.  But it could well be time to look at it again.
> How does the following (untested) patch look?

Pretty bad, actually...

People use rcu_assign_pointer() for pointers to functions, which gets
interesting compiler warnings for some configurations.  Please see below
for attempt #2.

							Thanx, Paul

------------------------------------------------------------------------

commit f55c2ffb4e105fa0450fe495788765dffc4b752e
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Sun May 1 18:46:54 2016 -0700

    rcu: No ordering for rcu_assign_pointer() of NULL
    
    This commit does a compile-time check for rcu_assign_pointer() of NULL,
    and uses WRITE_ONCE() rather than smp_store_release() in that case.
    
    Reported-by: Christoph Hellwig <hch@infradead.org>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index c61b6b9506e7..9b8828c5a9c2 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -650,7 +650,16 @@ 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.
  */
-#define rcu_assign_pointer(p, v) smp_store_release(&p, RCU_INITIALIZER(v))
+#define rcu_assign_pointer(p, v) \
+({ \
+	uintptr_t _r_a_p__v = (uintptr_t)(v); \
+	\
+	if (__builtin_constant_p(v) && (_r_a_p__v) == (uintptr_t)NULL) \
+		WRITE_ONCE((p), (typeof(v))(_r_a_p__v)); \
+	else \
+		smp_store_release(&p, RCU_INITIALIZER((typeof(v))_r_a_p__v)); \
+	_r_a_p__v; \
+})
 
 /**
  * rcu_access_pointer() - fetch RCU pointer with no dereferencing

[toc] | [prev] | [next] | [standalone]

#1392418

On Mon, May 02, 2016 at 07:10:23AM -0700, Paul E. McKenney wrote:
> On Sun, May 01, 2016 at 12:53:13PM -0700, Paul E. McKenney wrote:
> > On Sun, May 01, 2016 at 10:01:20AM -0700, Christoph Hellwig wrote:
> > > On Sun, May 01, 2016 at 06:22:01PM +0530, Muhammad Falak R Wani wrote:
> > > > It is safe to use RCU_INIT_POINTER() to NULL, instead of
> > > > rcu_assign_pointer().
> > > > This results in slightly smaller/faster code.
> > > 
> > > If this is indeed the case, rcu_assign_pointer should simply check
> > > for NULL using __builtin_constant_p and do the right thing transparently
> > > instead of burdening it on every user.
> > 
> > Last time around, there was a compiler bug that prevented this from
> > working correctly.  But it could well be time to look at it again.
> > How does the following (untested) patch look?
> 
> Pretty bad, actually...
> 
> People use rcu_assign_pointer() for pointers to functions, which gets
> interesting compiler warnings for some configurations.  Please see below
> for attempt #2.

Perhaps third time is the charm?

							Thanx, Paul

------------------------------------------------------------------------

commit 72a616bf7f99b2ef4f211f73c6def7fa884d6ca4
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Sun May 1 18:46:54 2016 -0700

    rcu: No ordering for rcu_assign_pointer() of NULL
    
    This commit does a compile-time check for rcu_assign_pointer() of NULL,
    and uses WRITE_ONCE() rather than smp_store_release() in that case.
    
    Reported-by: Christoph Hellwig <hch@infradead.org>
    Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index c61b6b9506e7..9be61e47badc 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -650,7 +650,16 @@ 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.
  */
-#define rcu_assign_pointer(p, v) smp_store_release(&p, RCU_INITIALIZER(v))
+#define rcu_assign_pointer(p, v) \
+({ \
+	uintptr_t _r_a_p__v = (uintptr_t)(v); \
+	\
+	if (__builtin_constant_p(v) && (_r_a_p__v) == (uintptr_t)NULL) \
+		WRITE_ONCE((p), (typeof(p))(_r_a_p__v)); \
+	else \
+		smp_store_release(&p, RCU_INITIALIZER((typeof(p))_r_a_p__v)); \
+	_r_a_p__v; \
+})
 
 /**
  * rcu_access_pointer() - fetch RCU pointer with no dereferencing

[toc] | [prev] | [next] | [standalone]

#1392421

On Mon, May 02, 2016 at 10:57:38AM -0700, Paul E. McKenney wrote:
> -#define rcu_assign_pointer(p, v) smp_store_release(&p, RCU_INITIALIZER(v))
> +#define rcu_assign_pointer(p, v) \
> +({ \
> +	uintptr_t _r_a_p__v = (uintptr_t)(v); \
> +	\
> +	if (__builtin_constant_p(v) && (_r_a_p__v) == (uintptr_t)NULL) \
> +		WRITE_ONCE((p), (typeof(p))(_r_a_p__v)); \
> +	else \
> +		smp_store_release(&p, RCU_INITIALIZER((typeof(p))_r_a_p__v)); \
> +	_r_a_p__v; \
> +})

Can't we turn it into an inline (would need different calling
conventions for p, though).

[toc] | [prev] | [next] | [standalone]

#1392426

On Mon, May 02, 2016 at 10:59:50AM -0700, Christoph Hellwig wrote:
> On Mon, May 02, 2016 at 10:57:38AM -0700, Paul E. McKenney wrote:
> > -#define rcu_assign_pointer(p, v) smp_store_release(&p, RCU_INITIALIZER(v))
> > +#define rcu_assign_pointer(p, v) \
> > +({ \
> > +	uintptr_t _r_a_p__v = (uintptr_t)(v); \
> > +	\
> > +	if (__builtin_constant_p(v) && (_r_a_p__v) == (uintptr_t)NULL) \
> > +		WRITE_ONCE((p), (typeof(p))(_r_a_p__v)); \
> > +	else \
> > +		smp_store_release(&p, RCU_INITIALIZER((typeof(p))_r_a_p__v)); \
> > +	_r_a_p__v; \
> > +})
> 
> Can't we turn it into an inline (would need different calling
> conventions for p, though).

And for v.  But how do I do that without C++ templates?

Also, does __builtin_constant_p() work reliably on a parameter?
Especially when the compiler decides not to do the inlining?

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1393186

On Mon, May 02, 2016 at 11:06:44AM -0700, Paul E. McKenney wrote:
> And for v.  But how do I do that without C++ templates?
> 
> Also, does __builtin_constant_p() work reliably on a parameter?
> Especially when the compiler decides not to do the inlining?

Yeah, it's going to be a pain indeed, guess that's why it hasn't been
done before..

[toc] | [prev] | [next] | [standalone]

#1393409

On Tue, May 03, 2016 at 01:45:20AM -0700, Christoph Hellwig wrote:
> On Mon, May 02, 2016 at 11:06:44AM -0700, Paul E. McKenney wrote:
> > And for v.  But how do I do that without C++ templates?
> > 
> > Also, does __builtin_constant_p() work reliably on a parameter?
> > Especially when the compiler decides not to do the inlining?
> 
> Yeah, it's going to be a pain indeed, guess that's why it hasn't been
> done before..

Speaking of pain and things that haven't been done before, would you
be interested in being in the first group to review an attempt to
formalize memory-barriers.txt?

							Thanx, Paul

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web