[PATCH] rcuupdate: Do a single rhp->func read in rcu_head_after_call_rcu

[PATCH] rcuupdate: Do a single rhp->func read in rcu_head_after_call_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

Re: [PATCH] rcuupdate: Do a single rhp->func read in rcu_head_after_call_rcu

[Paul E. McKenney]

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
> 

Re: [PATCH] rcuupdate: Do a single rhp->func read in rcu_head_after_call_rcu

[Neeraj Upadhyay]

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

Re: [PATCH] rcuupdate: Do a single rhp->func read in rcu_head_after_call_rcu

[Paul E. McKenney]

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;
 }