* [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing.
@ 2016-05-01 12:52 Muhammad Falak R Wani
2016-05-01 12:52 ` [PATCH 1/2] " Muhammad Falak R Wani
2016-05-01 17:01 ` [PATCH 2/2] " Christoph Hellwig
0 siblings, 2 replies; 10+ messages in thread
From: Muhammad Falak R Wani @ 2016-05-01 12:52 UTC (permalink / raw)
To: Nicholas A. Bellinger; +Cc: target-devel, linux-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 1/2] target: use RCU_INIT_POINTER() when NULLing.
2016-05-01 12:52 [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing Muhammad Falak R Wani
@ 2016-05-01 12:52 ` Muhammad Falak R Wani
2016-05-01 17:01 ` [PATCH 2/2] " Christoph Hellwig
1 sibling, 0 replies; 10+ messages in thread
From: Muhammad Falak R Wani @ 2016-05-01 12:52 UTC (permalink / raw)
To: Nicholas A. Bellinger; +Cc: target-devel, linux-kernel
It is safe to use RCU_INIT_POINTER() to NULL, instead of
rcu_assign_pointer().
This results in slightly smaller/faster code.
The follwoing semantic patch was used:
<smpl>
@@
@@
- rcu_assign_pointer
+ RCU_INIT_POINTER
(..., NULL)
</smpl>
Signed-off-by: Muhammad Falak R Wani <falakreyaz@gmail.com>
---
drivers/target/target_core_device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index a4046ca..6f81e3b 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -432,8 +432,8 @@ void core_disable_device_list_for_node(
kref_put(&orig->pr_kref, target_pr_kref_release);
wait_for_completion(&orig->pr_comp);
- rcu_assign_pointer(orig->se_lun, NULL);
- rcu_assign_pointer(orig->se_lun_acl, NULL);
+ RCU_INIT_POINTER(orig->se_lun, NULL);
+ RCU_INIT_POINTER(orig->se_lun_acl, NULL);
kfree_rcu(orig, rcu_head);
--
1.9.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing.
2016-05-01 12:52 [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing Muhammad Falak R Wani
2016-05-01 12:52 ` [PATCH 1/2] " Muhammad Falak R Wani
@ 2016-05-01 17:01 ` Christoph Hellwig
2016-05-01 19:53 ` Paul E. McKenney
1 sibling, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2016-05-01 17:01 UTC (permalink / raw)
To: Muhammad Falak R Wani, paulmck
Cc: Nicholas A. Bellinger, target-devel, linux-kernel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing.
2016-05-01 17:01 ` [PATCH 2/2] " Christoph Hellwig
@ 2016-05-01 19:53 ` Paul E. McKenney
2016-05-02 14:10 ` Paul E. McKenney
0 siblings, 1 reply; 10+ messages in thread
From: Paul E. McKenney @ 2016-05-01 19:53 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Muhammad Falak R Wani, Nicholas A. Bellinger, target-devel, linux-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing.
2016-05-01 19:53 ` Paul E. McKenney
@ 2016-05-02 14:10 ` Paul E. McKenney
2016-05-02 17:57 ` Paul E. McKenney
0 siblings, 1 reply; 10+ messages in thread
From: Paul E. McKenney @ 2016-05-02 14:10 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Muhammad Falak R Wani, Nicholas A. Bellinger, target-devel, linux-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing.
2016-05-02 14:10 ` Paul E. McKenney
@ 2016-05-02 17:57 ` Paul E. McKenney
2016-05-02 17:59 ` Christoph Hellwig
0 siblings, 1 reply; 10+ messages in thread
From: Paul E. McKenney @ 2016-05-02 17:57 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Muhammad Falak R Wani, Nicholas A. Bellinger, target-devel, linux-kernel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing.
2016-05-02 17:57 ` Paul E. McKenney
@ 2016-05-02 17:59 ` Christoph Hellwig
2016-05-02 18:06 ` Paul E. McKenney
0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2016-05-02 17:59 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Christoph Hellwig, Muhammad Falak R Wani, Nicholas A. Bellinger,
target-devel, linux-kernel
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).
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing.
2016-05-02 17:59 ` Christoph Hellwig
@ 2016-05-02 18:06 ` Paul E. McKenney
2016-05-03 8:45 ` Christoph Hellwig
0 siblings, 1 reply; 10+ messages in thread
From: Paul E. McKenney @ 2016-05-02 18:06 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Muhammad Falak R Wani, Nicholas A. Bellinger, target-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing.
2016-05-02 18:06 ` Paul E. McKenney
@ 2016-05-03 8:45 ` Christoph Hellwig
2016-05-03 13:07 ` Paul E. McKenney
0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2016-05-03 8:45 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Christoph Hellwig, Muhammad Falak R Wani, Nicholas A. Bellinger,
target-devel, linux-kernel
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..
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing.
2016-05-03 8:45 ` Christoph Hellwig
@ 2016-05-03 13:07 ` Paul E. McKenney
0 siblings, 0 replies; 10+ messages in thread
From: Paul E. McKenney @ 2016-05-03 13:07 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Muhammad Falak R Wani, Nicholas A. Bellinger, target-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-05-03 13:07 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-01 12:52 [PATCH 2/2] target: use RCU_INIT_POINTER() when NULLing Muhammad Falak R Wani
2016-05-01 12:52 ` [PATCH 1/2] " Muhammad Falak R Wani
2016-05-01 17:01 ` [PATCH 2/2] " Christoph Hellwig
2016-05-01 19:53 ` Paul E. McKenney
2016-05-02 14:10 ` Paul E. McKenney
2016-05-02 17:57 ` Paul E. McKenney
2016-05-02 17:59 ` Christoph Hellwig
2016-05-02 18:06 ` Paul E. McKenney
2016-05-03 8:45 ` Christoph Hellwig
2016-05-03 13:07 ` Paul E. McKenney
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).