linux-kernel - Re: [PATCH v6 1/4] rcu: Make call

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <19217A4C-7183-4D78-A714-FBFE7BB20742@joelfernandes.org>
Date:   Sat, 24 Sep 2022 19:28:16 -0400
From:   Joel Fernandes <joel@...lfernandes.org>
To:     Frederic Weisbecker <frederic@...nel.org>
Cc:     rcu@...r.kernel.org, linux-kernel@...r.kernel.org,
        rushikesh.s.kadam@...el.com, urezki@...il.com,
        neeraj.iitr10@...il.com, paulmck@...nel.org, rostedt@...dmis.org
Subject: Re: [PATCH v6 1/4] rcu: Make call_rcu() lazy to save power

Hi Frederic, thanks for the response, replies
below courtesy fruit company’s device:

> On Sep 24, 2022, at 6:46 PM, Frederic Weisbecker <frederic@...nel.org> 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.

> 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.