> On Sep 24, 2022, at 7:28 PM, Joel Fernandes <joel@xxxxxxxxxxxxxxxxx> wrote:
>
> Hi Frederic, thanks for the response, replies
> below courtesy fruit company’s device:
>
>>> On Sep 24, 2022, at 6:46 PM, Frederic Weisbecker <frederic@xxxxxxxxxx> wrote:
>>>
>>> On Thu, Sep 22, 2022 at 10:01:01PM +0000, Joel Fernandes (Google) wrote:
>>> @@ -3902,7 +3939,11 @@ static void rcu_barrier_entrain(struct rcu_data *rdp)
>>> rdp->barrier_head.func = rcu_barrier_callback;
>>> debug_rcu_head_queue(&rdp->barrier_head);
>>> rcu_nocb_lock(rdp);
>>> - WARN_ON_ONCE(!rcu_nocb_flush_bypass(rdp, NULL, jiffies));
>>> + /*
>>> + * Flush the bypass list, but also wake up the GP thread as otherwise
>>> + * bypass/lazy CBs maynot be noticed, and can cause real long delays!
>>> + */
>>> + WARN_ON_ONCE(!rcu_nocb_flush_bypass(rdp, NULL, jiffies, FLUSH_BP_WAKE));
>>
>> This fixes an issue that goes beyond lazy implementation. It should be done
>> in a separate patch, handling rcu_segcblist_entrain() as well, with "Fixes: " tag.
>
> I wanted to do that, however on discussion with
> Paul I thought of making this optimization only for
> all lazy bypass CBs. That makes it directly related
> this patch since the laziness notion is first
> introduced here. On the other hand I could make
> this change in a later patch since we are not
> super bisectable anyway courtesy of the last
> patch (which is not really an issue if the CONFIG
> is kept off during someone’s bisection.
Or are we saying it’s worth doing the wake up for rcu barrier even for regular bypass CB? That’d save 2 jiffies on rcu barrier. If we agree it’s needed, then yes splitting the patch makes sense.
Please let me know your opinions, thanks,
- Joel
>
>> And then FLUSH_BP_WAKE is probably not needed anymore.
>
> It is needed as the API is in tree_nocb.h and we
> have to have that handle the details of laziness
> there rather than tree.c. We could add new apis
> to get rid of flag but it’s cleaner (and Paul seemed
> to be ok with it).
>
>>> if (rcu_segcblist_entrain(&rdp->cblist, &rdp->barrier_head)) {
>>> atomic_inc(&rcu_state.barrier_cpu_count);
>>> } else {
>>> @@ -269,10 +294,14 @@ static void wake_nocb_gp_defer(struct rcu_data *rdp, int waketype,
>>> raw_spin_lock_irqsave(&rdp_gp->nocb_gp_lock, flags);
>>>
>>> /*
>>> - * Bypass wakeup overrides previous deferments. In case
>>> - * of callback storm, no need to wake up too early.
>>> + * Bypass wakeup overrides previous deferments. In case of
>>> + * callback storm, no need to wake up too early.
>>> */
>>> - if (waketype == RCU_NOCB_WAKE_BYPASS) {
>>> + if (waketype == RCU_NOCB_WAKE_LAZY
>>> + && READ_ONCE(rdp->nocb_defer_wakeup) == RCU_NOCB_WAKE_NOT) {
>>
>> This can be a plain READ since ->nocb_defer_wakeup is only written under ->nocb_gp_lock.
>
> Yes makes sense, will do.
>
>>> + mod_timer(&rdp_gp->nocb_timer, jiffies + jiffies_till_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);
>>> } else {
>>> @@ -512,9 +598,16 @@ static void __call_rcu_nocb_wake(struct rcu_data *rdp, bool was_alldone,
>>> }
>>> // Need to actually to a wakeup.
>>> len = rcu_segcblist_n_cbs(&rdp->cblist);
>>> + bypass_len = rcu_cblist_n_cbs(&rdp->nocb_bypass);
>>> + lazy_len = READ_ONCE(rdp->lazy_len);
>>> if (was_alldone) {
>>> rdp->qlen_last_fqs_check = len;
>>> - if (!irqs_disabled_flags(flags)) {
>>> + // Only lazy CBs in bypass list
>>> + if (lazy_len && bypass_len == lazy_len) {
>>> + rcu_nocb_unlock_irqrestore(rdp, flags);
>>> + wake_nocb_gp_defer(rdp, RCU_NOCB_WAKE_LAZY,
>>> + TPS("WakeLazy"));
>>
>> I'm trying to think of a case where rcu_nocb_try_bypass() returns false
>> (queue to regular list) but then call_rcu() -> __call_rcu_nocb_wake() ends up
>> seeing a lazy bypass queue even though we are queueing a non-lazy callback
>> (should have flushed in this case).
>>
>> Looks like it shouldn't happen, even with concurrent (de-offloading) but just
>> in case, can we add:
>
> Yes I also feel this couldn’t happen because irq is
> off and nocb lock is held throughout the calls to
> the above 2 functions. Unless I missed the race
> you’re describing?
>
>>
>> WARN_ON_ONCE(lazy_len != len)
>
> But this condition can be true even in normal
> circumstances? len also contains DONE CBs
> which are ready to be invoked. Or did I miss
> something?
>
> Thanks,
>
> - Joel
>
>>
>>> + } else if (!irqs_disabled_flags(flags)) {
>>> /* ... if queue was empty ... */
>>> rcu_nocb_unlock_irqrestore(rdp, flags);
>>> wake_nocb_gp(rdp, false);
>>
>> Thanks.
