Hi Frederick, On 9/5/2022 8:59 AM, Frederic Weisbecker wrote: >>> Also that's a subtle change which purpose isn't explained. It means that >>> rcu_barrier_entrain() used to wait for the bypass timer in the worst case >>> but now we force rcuog into it immediately. Should that be a separate change? >> It could be split, but it is laziness that amplifies the issue so I thought of >> keeping it in the same patch. I don't mind one way or the other. > Ok then lets keep it here but please add a comment for the reason to > force wake here. Ok will do, thanks. >>>> + case RCU_NOCB_WAKE_BYPASS: >>>> + mod_jif = 2; >>>> + break; >>>> + >>>> + case RCU_NOCB_WAKE: >>>> + case RCU_NOCB_WAKE_FORCE: >>>> + // For these, make it wake up the soonest if we >>>> + // were in a bypass or lazy sleep before. >>>> if (rdp_gp->nocb_defer_wakeup < RCU_NOCB_WAKE) >>>> - mod_timer(&rdp_gp->nocb_timer, jiffies + 1); >>>> - if (rdp_gp->nocb_defer_wakeup < waketype) >>>> - WRITE_ONCE(rdp_gp->nocb_defer_wakeup, waketype); >>>> + mod_jif = 1; >>>> + break; >>>> } >>>> >>>> + if (mod_jif) >>>> + mod_timer(&rdp_gp->nocb_timer, jiffies + mod_jif); >>>> + >>>> + if (rdp_gp->nocb_defer_wakeup < waketype) >>>> + WRITE_ONCE(rdp_gp->nocb_defer_wakeup, waketype); >>> So RCU_NOCB_WAKE_BYPASS and RCU_NOCB_WAKE_LAZY don't override the timer state >>> anymore? Looks like something is missing. >> My goal was to make sure that NOCB_WAKE_LAZY wake keeps the timer lazy. If I >> don't do this, then when CPU enters idle, it will immediately do a wake up via >> this call: >> >> rcu_nocb_need_deferred_wakeup(rdp_gp, RCU_NOCB_WAKE) > But if the timer is in RCU_NOCB_WAKE_LAZY mode, that shouldn't be a problem. > >> That was almost always causing lazy CBs to be non-lazy thus negating all the >> benefits. >> >> Note that bypass will also have same issue where the bypass CB will not wait for >> intended bypass duration. To make it consistent with lazy, I made bypass also >> not override nocb_defer_wakeup. > I'm surprised because rcu_nocb_flush_deferred_wakeup() should only do the wake up > if the timer is RCU_NOCB_WAKE or RCU_NOCB_WAKE_FORCE. Or is that code buggy > somehow? > Actually your change is modifying the timer delay without changing the timer > mode, which may shortcut rcu_nocb_flush_deferred_wakeup() check and actually > make it perform early upon idle loop entry. > > Or am I missing something? > You could very well have a point and I am not sure now (I happen to 'forget' the issue once the code was working). I distinctly remember not being able to be lazy without doing this. Maybe there is some other path. I am kicking myself for not commenting in the code or change log enough about the issue. I will test again sync'ing the lazy timer and the ->nocb_defer_wakeup field properly and see if I can trigger the issue. Thanks! - Joel
