* [PATCH] rcuupdate: Do a single rhp->func read in rcu_head_after_call_rcu @ 2019-03-11 11:58 Neeraj Upadhyay 2019-03-11 15:48 ` Paul E. McKenney 0 siblings, 1 reply; 4+ messages in thread From: Neeraj Upadhyay @ 2019-03-11 11:58 UTC (permalink / raw) To: paulmck, josh, rostedt, mathieu.desnoyers, jiangshanlai Cc: rcu, Neeraj Upadhyay Read rhp->func pointer in rcu_head_after_call_rcu() only once, to avoid warning in the case, where call_rcu() happens between two reads of rhp->func. Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org> --- include/linux/rcupdate.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h index 6cdb1db..174768e 100644 --- a/include/linux/rcupdate.h +++ b/include/linux/rcupdate.h @@ -878,9 +878,10 @@ static inline void rcu_head_init(struct rcu_head *rhp) static inline bool rcu_head_after_call_rcu(struct rcu_head *rhp, rcu_callback_t f) { - if (READ_ONCE(rhp->func) == f) + rcu_callback_t func = READ_ONCE(rhp->func); + if (func == f) return true; - WARN_ON_ONCE(READ_ONCE(rhp->func) != (rcu_callback_t)~0L); + WARN_ON_ONCE(func != (rcu_callback_t)~0L); return false; } -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] rcuupdate: Do a single rhp->func read in rcu_head_after_call_rcu 2019-03-11 11:58 [PATCH] rcuupdate: Do a single rhp->func read in rcu_head_after_call_rcu Neeraj Upadhyay @ 2019-03-11 15:48 ` Paul E. McKenney 2019-03-11 15:52 ` Neeraj Upadhyay 0 siblings, 1 reply; 4+ messages in thread From: Paul E. McKenney @ 2019-03-11 15:48 UTC (permalink / raw) To: Neeraj Upadhyay; +Cc: josh, rostedt, mathieu.desnoyers, jiangshanlai, rcu On Mon, Mar 11, 2019 at 05:28:03PM +0530, Neeraj Upadhyay wrote: > Read rhp->func pointer in rcu_head_after_call_rcu() only once, > to avoid warning in the case, where call_rcu() happens between > two reads of rhp->func. > > Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org> This would more gracefully handle racing rcu_head_after_call_rcu() with call_rcu(). But this thing is not yet used, so let's see what Neil Brown says. If he isn't going to use it, my thought is to instead just remove this. Thanx, Paul > --- > include/linux/rcupdate.h | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > index 6cdb1db..174768e 100644 > --- a/include/linux/rcupdate.h > +++ b/include/linux/rcupdate.h > @@ -878,9 +878,10 @@ static inline void rcu_head_init(struct rcu_head *rhp) > static inline bool > rcu_head_after_call_rcu(struct rcu_head *rhp, rcu_callback_t f) > { > - if (READ_ONCE(rhp->func) == f) > + rcu_callback_t func = READ_ONCE(rhp->func); > + if (func == f) > return true; > - WARN_ON_ONCE(READ_ONCE(rhp->func) != (rcu_callback_t)~0L); > + WARN_ON_ONCE(func != (rcu_callback_t)~0L); > return false; > } > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a > member of the Code Aurora Forum, hosted by The Linux Foundation > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] rcuupdate: Do a single rhp->func read in rcu_head_after_call_rcu 2019-03-11 15:48 ` Paul E. McKenney @ 2019-03-11 15:52 ` Neeraj Upadhyay 2019-03-11 22:09 ` Paul E. McKenney 0 siblings, 1 reply; 4+ messages in thread From: Neeraj Upadhyay @ 2019-03-11 15:52 UTC (permalink / raw) To: paulmck; +Cc: josh, rostedt, mathieu.desnoyers, jiangshanlai, rcu On 3/11/19 9:18 PM, Paul E. McKenney wrote: > On Mon, Mar 11, 2019 at 05:28:03PM +0530, Neeraj Upadhyay wrote: >> Read rhp->func pointer in rcu_head_after_call_rcu() only once, >> to avoid warning in the case, where call_rcu() happens between >> two reads of rhp->func. >> >> Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org> > > This would more gracefully handle racing rcu_head_after_call_rcu() > with call_rcu(). > > But this thing is not yet used, so let's see what Neil Brown says. > If he isn't going to use it, my thought is to instead just remove > this. > > Thanx, Paul > Agree, makes sense. Thanks Neeraj >> --- >> include/linux/rcupdate.h | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h >> index 6cdb1db..174768e 100644 >> --- a/include/linux/rcupdate.h >> +++ b/include/linux/rcupdate.h >> @@ -878,9 +878,10 @@ static inline void rcu_head_init(struct rcu_head *rhp) >> static inline bool >> rcu_head_after_call_rcu(struct rcu_head *rhp, rcu_callback_t f) >> { >> - if (READ_ONCE(rhp->func) == f) >> + rcu_callback_t func = READ_ONCE(rhp->func); >> + if (func == f) >> return true; >> - WARN_ON_ONCE(READ_ONCE(rhp->func) != (rcu_callback_t)~0L); >> + WARN_ON_ONCE(func != (rcu_callback_t)~0L); >> return false; >> } >> >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a >> member of the Code Aurora Forum, hosted by The Linux Foundation >> > -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] rcuupdate: Do a single rhp->func read in rcu_head_after_call_rcu 2019-03-11 15:52 ` Neeraj Upadhyay @ 2019-03-11 22:09 ` Paul E. McKenney 0 siblings, 0 replies; 4+ messages in thread From: Paul E. McKenney @ 2019-03-11 22:09 UTC (permalink / raw) To: Neeraj Upadhyay; +Cc: josh, rostedt, mathieu.desnoyers, jiangshanlai, rcu On Mon, Mar 11, 2019 at 09:22:30PM +0530, Neeraj Upadhyay wrote: > > > On 3/11/19 9:18 PM, Paul E. McKenney wrote: > >On Mon, Mar 11, 2019 at 05:28:03PM +0530, Neeraj Upadhyay wrote: > >>Read rhp->func pointer in rcu_head_after_call_rcu() only once, > >>to avoid warning in the case, where call_rcu() happens between > >>two reads of rhp->func. > >> > >>Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org> > > > >This would more gracefully handle racing rcu_head_after_call_rcu() > >with call_rcu(). > > > >But this thing is not yet used, so let's see what Neil Brown says. > >If he isn't going to use it, my thought is to instead just remove > >this. > > Agree, makes sense. And Neil said that he intends to use it, so I applied your patch, updated as shown below. Ah, and please use scripts/checkpatch.pl -- it sometimes gets overly enthusiastic, but the blank line following the declarations is good practice. Thanx, Paul ------------------------------------------------------------------------ commit fcf4326ee3fb7e0925fe0f299c385c31f5d62fbf Author: Neeraj Upadhyay <neeraju@codeaurora.org> Date: Mon Mar 11 17:28:03 2019 +0530 rcu: Do a single rhp->func read in rcu_head_after_call_rcu() The rcu_head_after_call_rcu() function reads the rhp->func pointer twice, which can result in a false-positive WARN_ON_ONCE() if the callback were passed to call_rcu() between the two reads. Although racing rcu_head_after_call_rcu() with call_rcu() is to be a dubious use case (the return value is not reliable in that case), intermittent and irreproducible warnings are also quite dubious. This commit therefore uses a single READ_ONCE() to pick up the value of rhp->func once, then tests that value twice, thus guaranteeing consistent processing within rcu_head_after_call_rcu()(). Neverthless, racing rcu_head_after_call_rcu() with call_rcu() is still a dubious use case. Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org> [ paulmck: Add blank line after declaration per checkpatch.pl. ] Signed-off-by: Paul E. McKenney <paulmck@linux.ibm.com> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h index 6cdb1db776cf..922bb6848813 100644 --- a/include/linux/rcupdate.h +++ b/include/linux/rcupdate.h @@ -878,9 +878,11 @@ static inline void rcu_head_init(struct rcu_head *rhp) static inline bool rcu_head_after_call_rcu(struct rcu_head *rhp, rcu_callback_t f) { - if (READ_ONCE(rhp->func) == f) + rcu_callback_t func = READ_ONCE(rhp->func); + + if (func == f) return true; - WARN_ON_ONCE(READ_ONCE(rhp->func) != (rcu_callback_t)~0L); + WARN_ON_ONCE(func != (rcu_callback_t)~0L); return false; } ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-03-11 22:09 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-11 11:58 [PATCH] rcuupdate: Do a single rhp->func read in rcu_head_after_call_rcu Neeraj Upadhyay 2019-03-11 15:48 ` Paul E. McKenney 2019-03-11 15:52 ` Neeraj Upadhyay 2019-03-11 22:09 ` Paul E. McKenney
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.