* [PATCH] kernel: events: core: Replace rcu_assign_pointer() with RCU_INIT_POINTER() @ 2014-08-22 13:26 Andreea-Cristina Bernat 2014-09-09 9:42 ` Peter Zijlstra 2014-09-09 14:53 ` [tip:perf/core] perf/core: " tip-bot for Andreea-Cristina Bernat 0 siblings, 2 replies; 8+ messages in thread From: Andreea-Cristina Bernat @ 2014-08-22 13:26 UTC (permalink / raw) To: a.p.zijlstra, paulus, mingo, acme, linux-kernel; +Cc: paulmck The use of "rcu_assign_pointer()" is NULLing out the pointer. According to RCU_INIT_POINTER()'s block comment: "1. This use of RCU_INIT_POINTER() is NULLing out the pointer" it is better to use it instead of rcu_assign_pointer() because it has a smaller overhead. The following Coccinelle semantic patch was used: @@ @@ - rcu_assign_pointer + RCU_INIT_POINTER (..., NULL) Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com> --- kernel/events/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/events/core.c b/kernel/events/core.c index 0d735be..4b84dd5 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -5744,7 +5744,7 @@ static void swevent_hlist_release(struct swevent_htable *swhash) if (!hlist) return; - rcu_assign_pointer(swhash->swevent_hlist, NULL); + RCU_INIT_POINTER(swhash->swevent_hlist, NULL); kfree_rcu(hlist, rcu_head); } -- 1.9.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] kernel: events: core: Replace rcu_assign_pointer() with RCU_INIT_POINTER() 2014-08-22 13:26 [PATCH] kernel: events: core: Replace rcu_assign_pointer() with RCU_INIT_POINTER() Andreea-Cristina Bernat @ 2014-09-09 9:42 ` Peter Zijlstra 2014-09-09 16:16 ` Paul E. McKenney 2014-09-09 14:53 ` [tip:perf/core] perf/core: " tip-bot for Andreea-Cristina Bernat 1 sibling, 1 reply; 8+ messages in thread From: Peter Zijlstra @ 2014-09-09 9:42 UTC (permalink / raw) To: Andreea-Cristina Bernat; +Cc: paulus, mingo, acme, linux-kernel, paulmck [-- Attachment #1: Type: text/plain, Size: 1991 bytes --] On Fri, Aug 22, 2014 at 04:26:05PM +0300, Andreea-Cristina Bernat wrote: > The use of "rcu_assign_pointer()" is NULLing out the pointer. > According to RCU_INIT_POINTER()'s block comment: > "1. This use of RCU_INIT_POINTER() is NULLing out the pointer" > it is better to use it instead of rcu_assign_pointer() because it has a > smaller overhead. > > The following Coccinelle semantic patch was used: > @@ > @@ > > - rcu_assign_pointer > + RCU_INIT_POINTER > (..., NULL) > > Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com> > --- > kernel/events/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 0d735be..4b84dd5 100644 > --- a/kernel/events/core.c > +++ b/kernel/events/core.c > @@ -5744,7 +5744,7 @@ static void swevent_hlist_release(struct swevent_htable *swhash) > if (!hlist) > return; > > - rcu_assign_pointer(swhash->swevent_hlist, NULL); > + RCU_INIT_POINTER(swhash->swevent_hlist, NULL); > kfree_rcu(hlist, rcu_head); > } Paul, why not do something like the below and do away with all this nonsense? --- include/linux/rcupdate.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h index d231aa17b1d7..38c3e629ae70 100644 --- a/include/linux/rcupdate.h +++ b/include/linux/rcupdate.h @@ -623,7 +623,13 @@ 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) \ +do { \ + if (__builtin_constant_p(v) && v == NULL) \ + p = RCU_INITIALIZER(v); \ + else \ + smp_store_release(&p, RCU_INITIALIZER(v)); \ +} while (0) /** * rcu_access_pointer() - fetch RCU pointer with no dereferencing [-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] kernel: events: core: Replace rcu_assign_pointer() with RCU_INIT_POINTER() 2014-09-09 9:42 ` Peter Zijlstra @ 2014-09-09 16:16 ` Paul E. McKenney 2014-09-10 13:12 ` Peter Zijlstra 0 siblings, 1 reply; 8+ messages in thread From: Paul E. McKenney @ 2014-09-09 16:16 UTC (permalink / raw) To: Peter Zijlstra; +Cc: Andreea-Cristina Bernat, paulus, mingo, acme, linux-kernel On Tue, Sep 09, 2014 at 11:42:35AM +0200, Peter Zijlstra wrote: > On Fri, Aug 22, 2014 at 04:26:05PM +0300, Andreea-Cristina Bernat wrote: > > The use of "rcu_assign_pointer()" is NULLing out the pointer. > > According to RCU_INIT_POINTER()'s block comment: > > "1. This use of RCU_INIT_POINTER() is NULLing out the pointer" > > it is better to use it instead of rcu_assign_pointer() because it has a > > smaller overhead. > > > > The following Coccinelle semantic patch was used: > > @@ > > @@ > > > > - rcu_assign_pointer > > + RCU_INIT_POINTER > > (..., NULL) > > > > Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com> > > --- > > kernel/events/core.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/events/core.c b/kernel/events/core.c > > index 0d735be..4b84dd5 100644 > > --- a/kernel/events/core.c > > +++ b/kernel/events/core.c > > @@ -5744,7 +5744,7 @@ static void swevent_hlist_release(struct swevent_htable *swhash) > > if (!hlist) > > return; > > > > - rcu_assign_pointer(swhash->swevent_hlist, NULL); > > + RCU_INIT_POINTER(swhash->swevent_hlist, NULL); > > kfree_rcu(hlist, rcu_head); > > } > > Paul, why not do something like the below and do away with all this > nonsense? We used to do that, but the compilers became uncooperative, despite Stephen Hemminger's best efforts. Thanx, Paul > --- > include/linux/rcupdate.h | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > index d231aa17b1d7..38c3e629ae70 100644 > --- a/include/linux/rcupdate.h > +++ b/include/linux/rcupdate.h > @@ -623,7 +623,13 @@ 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) \ > +do { \ > + if (__builtin_constant_p(v) && v == NULL) \ > + p = RCU_INITIALIZER(v); \ > + else \ > + smp_store_release(&p, RCU_INITIALIZER(v)); \ > +} while (0) > > /** > * rcu_access_pointer() - fetch RCU pointer with no dereferencing ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kernel: events: core: Replace rcu_assign_pointer() with RCU_INIT_POINTER() 2014-09-09 16:16 ` Paul E. McKenney @ 2014-09-10 13:12 ` Peter Zijlstra 2014-09-12 16:30 ` Paul E. McKenney 0 siblings, 1 reply; 8+ messages in thread From: Peter Zijlstra @ 2014-09-10 13:12 UTC (permalink / raw) To: Paul E. McKenney Cc: Andreea-Cristina Bernat, paulus, mingo, acme, linux-kernel On Tue, Sep 09, 2014 at 09:16:48AM -0700, Paul E. McKenney wrote: > On Tue, Sep 09, 2014 at 11:42:35AM +0200, Peter Zijlstra wrote: > > Paul, why not do something like the below and do away with all this > > nonsense? > > We used to do that, but the compilers became uncooperative, despite > Stephen Hemminger's best efforts. Happen to have a link handy to that thread so I can educate myself? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kernel: events: core: Replace rcu_assign_pointer() with RCU_INIT_POINTER() 2014-09-10 13:12 ` Peter Zijlstra @ 2014-09-12 16:30 ` Paul E. McKenney 2014-09-26 14:53 ` Peter Zijlstra 0 siblings, 1 reply; 8+ messages in thread From: Paul E. McKenney @ 2014-09-12 16:30 UTC (permalink / raw) To: Peter Zijlstra Cc: Andreea-Cristina Bernat, paulus, mingo, acme, linux-kernel, stephen, eric.dumazet On Wed, Sep 10, 2014 at 03:12:51PM +0200, Peter Zijlstra wrote: > On Tue, Sep 09, 2014 at 09:16:48AM -0700, Paul E. McKenney wrote: > > On Tue, Sep 09, 2014 at 11:42:35AM +0200, Peter Zijlstra wrote: > > > > Paul, why not do something like the below and do away with all this > > > nonsense? > > > > We used to do that, but the compilers became uncooperative, despite > > Stephen Hemminger's best efforts. > > Happen to have a link handy to that thread so I can educate myself? After a bit of software archeology, here you go... Commit d322f45ceed52 (rcu: Make rcu_assign_pointer() unconditionally insert a memory barrier) made this change. According to the commit log: Recent changes to gcc give warning messages on rcu_assign_pointers()'s checks that allow it to determine when it is OK to omit the memory barrier. Stephen Hemminger tried a number of gcc tricks to silence this warning, but #pragmas and CPP macros do not work together in the way that would be required to make this work. This was applied in 2011, and searching LKML during that time gives the following: https://lkml.org/lkml/2011/7/29/305 And in this LKML post, Stephen Hemminger wrote: Gcc now generates warnings from rcu_assign_pointer when passed the address of something for example: rcu_assign_pointer(dev_queue->qdisc, &noop_qdisc); This warning is harmless and should be surpressed but there maybe other cases where we want that Gcc warning. I tried various combinations of in rcu_assign_pointer macro #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wlogical-op" ... #pragma GCC diagnostic pop but macro's and pragma's don't nest with the correct scope for this. Maybe some one with more Gcc foo and time to waste could take a crack at it. Adding Stephen on CC in case he remembers more. Thanx, Paul ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kernel: events: core: Replace rcu_assign_pointer() with RCU_INIT_POINTER() 2014-09-12 16:30 ` Paul E. McKenney @ 2014-09-26 14:53 ` Peter Zijlstra 2014-09-28 7:36 ` Paul E. McKenney 0 siblings, 1 reply; 8+ messages in thread From: Peter Zijlstra @ 2014-09-26 14:53 UTC (permalink / raw) To: Paul E. McKenney Cc: Andreea-Cristina Bernat, paulus, mingo, acme, linux-kernel, stephen, eric.dumazet On Fri, Sep 12, 2014 at 09:30:55AM -0700, Paul E. McKenney wrote: > On Wed, Sep 10, 2014 at 03:12:51PM +0200, Peter Zijlstra wrote: > > On Tue, Sep 09, 2014 at 09:16:48AM -0700, Paul E. McKenney wrote: > > > On Tue, Sep 09, 2014 at 11:42:35AM +0200, Peter Zijlstra wrote: > > > > > > Paul, why not do something like the below and do away with all this > > > > nonsense? > > > > > > We used to do that, but the compilers became uncooperative, despite > > > Stephen Hemminger's best efforts. > > > > Happen to have a link handy to that thread so I can educate myself? > > After a bit of software archeology, here you go... > > Commit d322f45ceed52 (rcu: Make rcu_assign_pointer() unconditionally > insert a memory barrier) made this change. According to the commit > log: > > Recent changes to gcc give warning messages on rcu_assign_pointers()'s > checks that allow it to determine when it is OK to omit the memory > barrier. Stephen Hemminger tried a number of gcc tricks to silence > this warning, but #pragmas and CPP macros do not work together in the > way that would be required to make this work. > > This was applied in 2011, and searching LKML during that time gives > the following: https://lkml.org/lkml/2011/7/29/305 > > And in this LKML post, Stephen Hemminger wrote: > > Gcc now generates warnings from rcu_assign_pointer when passed the > address of something for example: > rcu_assign_pointer(dev_queue->qdisc, &noop_qdisc); > This warning is harmless and should be surpressed but there maybe > other cases where we want that Gcc warning. > > I tried various combinations of in rcu_assign_pointer macro > #pragma GCC diagnostic push > #pragma GCC diagnostic ignored "-Wlogical-op" > ... > #pragma GCC diagnostic pop > but macro's and pragma's don't nest with the correct scope for > this. > > Maybe some one with more Gcc foo and time to waste could take > a crack at it. > > Adding Stephen on CC in case he remembers more. So I read all that and am still left wondering WTF the problem was :/ I googled a bit and found this thread: https://lkml.org/lkml/2014/3/24/39 Where Michael actually has two 'fixes' to the problem. Instead we continue clutter the API with this silly RCU_INIT_POINTER stuff, totally sad. But nowhere have I found the actual warning GCC gives, I suppose I can go change my local tree and compile some code to obtain it, but that seems backwards. The patch 'working' around that should have mentioned it and explained why the warning is bogus or not. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kernel: events: core: Replace rcu_assign_pointer() with RCU_INIT_POINTER() 2014-09-26 14:53 ` Peter Zijlstra @ 2014-09-28 7:36 ` Paul E. McKenney 0 siblings, 0 replies; 8+ messages in thread From: Paul E. McKenney @ 2014-09-28 7:36 UTC (permalink / raw) To: Peter Zijlstra Cc: Andreea-Cristina Bernat, paulus, mingo, acme, linux-kernel, stephen, eric.dumazet On Fri, Sep 26, 2014 at 04:53:35PM +0200, Peter Zijlstra wrote: > On Fri, Sep 12, 2014 at 09:30:55AM -0700, Paul E. McKenney wrote: > > On Wed, Sep 10, 2014 at 03:12:51PM +0200, Peter Zijlstra wrote: > > > On Tue, Sep 09, 2014 at 09:16:48AM -0700, Paul E. McKenney wrote: > > > > On Tue, Sep 09, 2014 at 11:42:35AM +0200, Peter Zijlstra wrote: > > > > > > > > Paul, why not do something like the below and do away with all this > > > > > nonsense? > > > > > > > > We used to do that, but the compilers became uncooperative, despite > > > > Stephen Hemminger's best efforts. > > > > > > Happen to have a link handy to that thread so I can educate myself? > > > > After a bit of software archeology, here you go... > > > > Commit d322f45ceed52 (rcu: Make rcu_assign_pointer() unconditionally > > insert a memory barrier) made this change. According to the commit > > log: > > > > Recent changes to gcc give warning messages on rcu_assign_pointers()'s > > checks that allow it to determine when it is OK to omit the memory > > barrier. Stephen Hemminger tried a number of gcc tricks to silence > > this warning, but #pragmas and CPP macros do not work together in the > > way that would be required to make this work. > > > > This was applied in 2011, and searching LKML during that time gives > > the following: https://lkml.org/lkml/2011/7/29/305 > > > > And in this LKML post, Stephen Hemminger wrote: > > > > Gcc now generates warnings from rcu_assign_pointer when passed the > > address of something for example: > > rcu_assign_pointer(dev_queue->qdisc, &noop_qdisc); > > This warning is harmless and should be surpressed but there maybe > > other cases where we want that Gcc warning. > > > > I tried various combinations of in rcu_assign_pointer macro > > #pragma GCC diagnostic push > > #pragma GCC diagnostic ignored "-Wlogical-op" > > ... > > #pragma GCC diagnostic pop > > but macro's and pragma's don't nest with the correct scope for > > this. > > > > Maybe some one with more Gcc foo and time to waste could take > > a crack at it. > > > > Adding Stephen on CC in case he remembers more. > > So I read all that and am still left wondering WTF the problem was :/ > > I googled a bit and found this thread: > > https://lkml.org/lkml/2014/3/24/39 > > Where Michael actually has two 'fixes' to the problem. Instead we > continue clutter the API with this silly RCU_INIT_POINTER stuff, totally > sad. Well, there are the other two use cases for RCU_INIT_POINTER(). But yes, most of the actual uses assign NULL. > But nowhere have I found the actual warning GCC gives, I suppose I can > go change my local tree and compile some code to obtain it, but that > seems backwards. The patch 'working' around that should have mentioned > it and explained why the warning is bogus or not. Indeed, I should have had a better commit log on the original change. Thanx, Paul ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tip:perf/core] perf/core: Replace rcu_assign_pointer() with RCU_INIT_POINTER() 2014-08-22 13:26 [PATCH] kernel: events: core: Replace rcu_assign_pointer() with RCU_INIT_POINTER() Andreea-Cristina Bernat 2014-09-09 9:42 ` Peter Zijlstra @ 2014-09-09 14:53 ` tip-bot for Andreea-Cristina Bernat 1 sibling, 0 replies; 8+ messages in thread From: tip-bot for Andreea-Cristina Bernat @ 2014-09-09 14:53 UTC (permalink / raw) To: linux-tip-commits Cc: linux-kernel, hpa, mingo, peterz, acme, tglx, bernat.ada Commit-ID: 70691d4a0bf7c871559d4ef1b0056edefbca123b Gitweb: http://git.kernel.org/tip/70691d4a0bf7c871559d4ef1b0056edefbca123b Author: Andreea-Cristina Bernat <bernat.ada@gmail.com> AuthorDate: Fri, 22 Aug 2014 16:26:05 +0300 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Tue, 9 Sep 2014 06:53:05 +0200 perf/core: Replace rcu_assign_pointer() with RCU_INIT_POINTER() The use of "rcu_assign_pointer()" is NULLing out the pointer. According to RCU_INIT_POINTER()'s block comment: "1. This use of RCU_INIT_POINTER() is NULLing out the pointer" it is better to use it instead of rcu_assign_pointer() because it has a smaller overhead. The following Coccinelle semantic patch was used: @@ @@ - rcu_assign_pointer + RCU_INIT_POINTER (..., NULL) Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: Arnaldo Carvalho de Melo <acme@kernel.org> Link: http://lkml.kernel.org/r/20140822132605.GA20130@ada Signed-off-by: Ingo Molnar <mingo@kernel.org> --- kernel/events/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/events/core.c b/kernel/events/core.c index 01bd42e..f917dec 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -5908,7 +5908,7 @@ static void swevent_hlist_release(struct swevent_htable *swhash) if (!hlist) return; - rcu_assign_pointer(swhash->swevent_hlist, NULL); + RCU_INIT_POINTER(swhash->swevent_hlist, NULL); kfree_rcu(hlist, rcu_head); } ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-09-28 7:36 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-08-22 13:26 [PATCH] kernel: events: core: Replace rcu_assign_pointer() with RCU_INIT_POINTER() Andreea-Cristina Bernat 2014-09-09 9:42 ` Peter Zijlstra 2014-09-09 16:16 ` Paul E. McKenney 2014-09-10 13:12 ` Peter Zijlstra 2014-09-12 16:30 ` Paul E. McKenney 2014-09-26 14:53 ` Peter Zijlstra 2014-09-28 7:36 ` Paul E. McKenney 2014-09-09 14:53 ` [tip:perf/core] perf/core: " tip-bot for Andreea-Cristina Bernat
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).