Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1f7dd31b-f4d0-5c1c-ce28-c27f75c17f05@joelfernandes.org>
Date:   Mon, 29 Aug 2022 12:45:40 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.0
Subject: Re: [PATCH v4 00/14] Implement call_rcu_lazy() and miscellaneous
 fixes
Content-Language: en-US
To:     Frederic Weisbecker <frederic@kernel.org>
Cc:     linux-kernel@vger.kernel.org, paulmck@kernel.org,
        Rushikesh S Kadam <rushikesh.s.kadam@intel.com>,
        "Uladzislau Rezki (Sony)" <urezki@gmail.com>,
        Neeraj upadhyay <neeraj.iitr10@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        rcu <rcu@vger.kernel.org>, vineeth@bitbyteword.org
References: <20220819204857.3066329-1-joel@joelfernandes.org>
 <20220829134045.GA54589@lothringen>
From:   Joel Fernandes <joel@joelfernandes.org>
In-Reply-To: <20220829134045.GA54589@lothringen>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Frederick,

On 8/29/2022 9:40 AM, Frederic Weisbecker wrote:
> On Fri, Aug 19, 2022 at 08:48:43PM +0000, Joel Fernandes (Google) wrote:
>> Refresh tested on real ChromeOS userspace and hardware, passes boot time tests
>> and rcuscale tests.
>>
>> Fixes on top of v3:
>> - Fix boot issues due to a race in the lazy RCU logic which caused a missed
>>   wakeup of the RCU GP thread, causing synchronize_rcu() to stall.
>> - Fixed trace_rcu_callback tracepoint
>>
>> I tested power previously [1], I am in the process of testing power again but I
>> wanted share my latest code as others who are testing power as well could use
>> the above fixes.
> 
> Your patch is very likely to be _generally_ useful and therefore,
> the more I look into this, the more I wonder if it is a good idea to rely on
> bypass at all, let alone NOCB. Of course in the long term the goal is to have
> bypass working without NOCB but why even bothering implementing it for nocb
> in the first place?

This was discussed with Paul [1]. Quoting:

----
Joel:
>> Also, does doing so not prevent usage of lazy CBs on systems without
>> NOCB? So if we want to future-proof this, I guess that might not be a
>> good decision.
>
Paul:
> True enough, but would this future actually arrive?  After all, if
> someone cared enough about energy efficiency to use call_rcu_lazy(),
> why wouldn't they also offload callbacks?

Joel: I am not sure, but I also don't mind making it depend on NOCB for now
(see below).

[1] https://www.spinics.net/lists/rcu/msg07908.html
----

While I agree with you that perhaps making it more generic is better, this did
take a significant amount of time, testing and corner case hunting to come up
with, and v5 is also in the works so I'd appreciate if we can do it the
bypass-way and optimize later. Arguably the bypass way is quite simple and
allows us to leverage its effects of rcu_barrier and such. And the API will not
change.

> Several highlights:
> 
> 1) NOCB is most often needed for nohz_full and the latter has terrible power
> management. The CPU 0 is active all the time there.

I see. We don't use nohz_full much. NOCB itself gives good power improvement.

> 2) NOCB without nohz_full has extremely rare usecase (RT niche:
> https://lore.kernel.org/lkml/CAFzL-7vqTX-y06Kc3HaLqRWAYE0d=ms3TzVtZLn0c6ATrKD+Qw@mail.gmail.com/
> )

Really? Android has been using it for a long time. It seems to be quite popular
in the battery-powered space.

> 2) NOCB implies performance issues.

Which kinds of? There is slightly worse boot times, but I'm guessing that's do
with the extra scheduling overhead of the extra threads which is usually not a
problem except that RCU is used in the critical path of boot up (on ChromeOS).

> 3) We are mixing up two very different things in a single list of callbacks:
>    lazy callbacks and flooding callbacks, as a result we are adding lots of
>    off-topic corner cases all around:
>      * a seperate lazy len field to struct rcu_cblist whose purpose is much more
>        general than just bypass/lazy
>      * "lazy" specialized parameters to general purpose cblist management
>        functions

I think just 1 or 2 functions have a new lazy param. It didn't seem too
intrusive to me.

> 4) This is further complexifying bypass core code, nocb timer management, core
>    nocb group management, all of which being already very complicated.

True, I agree, a few more cases to handle for sure, but I think I got them all
now (hopefully).

> 5) The !NOCB implementation is going to be very different
> 
> Ok I can admit one counter argument in favour of using NO_CB:
> 
> -1) The scheduler can benefit from a wake CPU to run the callbacks on behalf of a bunch
> of idle CPUs, instead of waking up that bunch of CPUs. But still we are dealing
> with callbacks that can actually wait...

Yeah that's huge. Significant amount of power improvement seems to come from
idle CPUs not being disturbed and their corresponding timer ticks turned off for
longer periods. That's experimentally confirmed (NO_CB giving significant power
improvement on battery-power systems as compared to !NO_CB).

> 
> So here is a proposal: how about forgetting NOCB for now and instead add a new
> RCU_LAZY_TAIL segment in the struct rcu_segcblist right after RCU_NEXT_TAIL?
> Then ignore that segment until some timer expiry has been met or the CPU is
> known to be busy? Probably some tiny bits need to be tweaked in segcblist
> management functions but probably not that much. And also make sure that entrain()
> queues to RCU_LAZY_TAIL.
> 
> Then the only difference in the case of NOCB is that we add a new timer to the
> nocb group leader instead of a local timer in !NOCB.

It sounds reasonable, but I'll go with Paul on the usecase argument - who would
actually care about lazy CBs outside of power, and would those guys ever use
!NO_CB if they cared about power / battery?

Thanks,

 - Joel