Re: [PATCH] rcu/nocb: Keep lazy timer intact on second lazy callback

[jfern]

On chriswiseman@xxxxxxxxxx wrote:
> wake_nocb_gp_defer() checks waketype == RCU_NOCB_WAKE_LAZY and
> rdp_gp->nocb_defer_wakeup == RCU_NOCB_WAKE_NOT in one if-statement.
> 
> When the first lazy call_rcu() in a NOCB group arms rdp_gp->nocb_timer
> for the 10-second lazy flush delay and sets rdp_gp->nocb_defer_wakeup to
> RCU_NOCB_WAKE_LAZY, the next CPU in the group that queues its first lazy
> callback calls wake_nocb_gp_defer(rdp, RCU_NOCB_WAKE_LAZY, ...). Because
> rdp_gp->nocb_defer_wakeup is no longer RCU_NOCB_WAKE_NOT, it misses the
> first branch and falls into the else block for RCU_NOCB_WAKE, calling
> mod_timer(&rdp_gp->nocb_timer, jiffies + 1) and resetting the 10-second
> lazy timer down to 1 jiffy. The same fallthrough also shortens a pending
> 2-jiffy RCU_NOCB_WAKE_BYPASS timer to 1 jiffy.
> 
> Commit 3cb278e73be5 ("rcu: Make call_rcu() lazy to save power") wrote
> this check against rdp->nocb_defer_wakeup, and commit 463d46044f04
> ("rcu: Fix delayed execution of hurry callbacks") switched it to
> rdp_gp->nocb_defer_wakeup, which exposed the fallthrough across all CPUs
> sharing rdp_gp.
> 
> Give waketype == RCU_NOCB_WAKE_LAZY its own branch and only arm
> nocb_timer when rdp_gp->nocb_defer_wakeup is RCU_NOCB_WAKE_NOT.
> 
> Fixes: 3cb278e73be5 ("rcu: Make call_rcu() lazy to save power")
> Fixes: 463d46044f04 ("rcu: Fix delayed execution of hurry callbacks")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Chris Wiseman <chriswiseman@xxxxxxxxxx>

Looks reasonable to me. Is there a test case that you added to find this, or any
power savings that you found with this added patch?

thanks,

Joel Fernandes

> ---
>  kernel/rcu/tree_nocb.h | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/rcu/tree_nocb.h b/kernel/rcu/tree_nocb.h
> index b6cfee4e5039..347d718befd0 100644
> --- a/kernel/rcu/tree_nocb.h
> +++ b/kernel/rcu/tree_nocb.h
> @@ -335,10 +335,12 @@ static void wake_nocb_gp_defer(struct rcu_data *rdp, int waketype,
>  	 * Bypass wakeup overrides previous deferments. In case of
>  	 * callback storms, no need to wake up too early.
>  	 */
> -	if (waketype == RCU_NOCB_WAKE_LAZY &&
> -	    rdp_gp->nocb_defer_wakeup == RCU_NOCB_WAKE_NOT) {
> -		mod_timer(&rdp_gp->nocb_timer, jiffies + rcu_get_jiffies_lazy_flush());
> -		WRITE_ONCE(rdp_gp->nocb_defer_wakeup, waketype);
> +	if (waketype == RCU_NOCB_WAKE_LAZY) {
> +		if (rdp_gp->nocb_defer_wakeup == RCU_NOCB_WAKE_NOT) {
> +			mod_timer(&rdp_gp->nocb_timer,
> +				  jiffies + rcu_get_jiffies_lazy_flush());
> +			WRITE_ONCE(rdp_gp->nocb_defer_wakeup, waketype);
> +		}
>  	} else if (waketype == RCU_NOCB_WAKE_BYPASS) {
>  		mod_timer(&rdp_gp->nocb_timer, jiffies + 2);
>  		WRITE_ONCE(rdp_gp->nocb_defer_wakeup, waketype);
> -- 
> 2.52.0.rc1