On Sat, May 7, 2022 at 6:32 PM Paul E. McKenney <[email protected]> wrote:
>
> On Sat, May 07, 2022 at 11:11:58AM +0200, Uladzislau Rezki wrote:
> > > On Fri, May 06, 2022 at 06:22:26PM +0200, Uladzislau Rezki wrote:
> > > > > On Thu, May 05, 2022 at 12:16:41PM +0200, Uladzislau Rezki (Sony) wrote:
> > > > > > Introduce a RCU_NOCB_CPU_CB_BOOST kernel option. So a user can
> > > > > > decide if an offloading has to be done in a high-prio context or
> > > > > > not. Please note an option depends on RCU_NOCB_CPU and RCU_BOOST
> > > > > > parameters and by default it is off.
> > > > > >
> > > > > > This patch splits the boosting preempted RCU readers and those
> > > > > > kthreads which directly responsible for driving expedited grace
> > > > > > periods forward with enabling/disabling the offloading from/to
> > > > > > SCHED_FIFO/SCHED_OTHER contexts.
> > > > > >
> > > > > > The main reason of such split is, for example on Android there
> > > > > > are some workloads which require fast expedited grace period to
> > > > > > be done whereas offloading in RT context can lead to starvation
> > > > > > and hogging a CPU for a long time what is not acceptable for
> > > > > > latency sensitive environment. For instance:
> > > > > >
> > > > > > <snip>
> > > > > > <...>-60 [006] d..1 2979.028717: rcu_batch_start: rcu_preempt CBs=34619 bl=270
> > > > > > <snip>
> > > > > >
> > > > > > invoking 34 619 callbacks will take time thus making other CFS
> > > > > > tasks waiting in run-queue to be starved due to such behaviour.
> > > > > >
> > > > > > Signed-off-by: Uladzislau Rezki (Sony) <[email protected]>
> > > > >
> > > > > All good points!
> > > > >
> > > > > Some questions and comments below.
> > > > >
> > > > > Adding Sebastian on CC for his perspective.
> > > > >
> > > > > Thanx, Paul
> > > > >
> > > > > > ---
> > > > > > kernel/rcu/Kconfig | 14 ++++++++++++++
> > > > > > kernel/rcu/tree.c | 5 ++++-
> > > > > > kernel/rcu/tree_nocb.h | 3 ++-
> > > > > > 3 files changed, 20 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/kernel/rcu/Kconfig b/kernel/rcu/Kconfig
> > > > > > index 27aab870ae4c..074630b94902 100644
> > > > > > --- a/kernel/rcu/Kconfig
> > > > > > +++ b/kernel/rcu/Kconfig
> > > > > > @@ -275,6 +275,20 @@ config RCU_NOCB_CPU_DEFAULT_ALL
> > > > > > Say Y here if you want offload all CPUs by default on boot.
> > > > > > Say N here if you are unsure.
> > > > > >
> > > > > > +config RCU_NOCB_CPU_CB_BOOST
> > > > > > + bool "Perform offloading from real-time kthread"
> > > > > > + depends on RCU_NOCB_CPU && RCU_BOOST
> > > > > > + default n
> > > > >
> > > > > I understand that you need this to default to "n" on your systems.
> > > > > However, other groups already using callback offloading should not see
> > > > > a sudden change. I don't see an Android-specific defconfig file, but
> > > > > perhaps something in drivers/android/Kconfig?
> > > > >
> > We saw a sudden change when the priority was lifted up for rcuop kthreads.
> > I would like to know the reason. As for Android, i would like to avoid
> > it to be Android specific. It is better just to enable boosting by
> > default for nocb kthreads.
>
> No, because that breaks an existing use case, which uses RCU_BOOST
> to avoid OOM on busy systems.
>
> > > > > One easy way to make this work would be to invert the sense of this
> > > > > Kconfig option ("RCU_NOCB_CB_NO_BOOST"?), continue having it default to
> > > > > "n", but then select it somewhere in drivers/android/Kconfig. But I
> > > > > would not be surprised if there is a better way.
> >
> > In that situation probably we should just enable it by default.
>
> You are within your rights to cause it to be enabled by default -within-
> -Android-. You are -not- within your rights to break other workloads.
>
> If ChromeOS needs it too, they too can enable it -within- -ChromeOS-.
>
> It is not -that- hard, guys! ;-)
I think on the topic of RT, +Steven Rostedt should chime in as well
considering he wrote a good chunk of the RT scheduler ;-). Personally,
I feel the issue of "rcu callback offload" threads running as RT or
not should not be a matter of CONFIG option or the system in concern.
Instead it should be a function of how many callbacks there are to
run. The reason I say this is, RT threads should not be doing a lot
of work anyway, lest they cause RT throttling and starvation of other
threads.
Also, I think it is wrong to assume that a certain kind of system will
always have a certain number of callbacks to process at a time. That
seems prone to poor design due to assumptions which may not always be
true.
Can we not have 2 sets of RCU offload threads, one which operate at RT
and only process few callbacks at a time, while another which is the
lower priority CFS offload thread - executes whenever there is a lot
of CBs pending? Just a thought.
Otherwise, I feel like we might be again proliferating CONFIG options
and increasing burden on the user to get it the CONFIG right.
thanks,
- Joel
On Sun, May 08, 2022 at 02:28:49AM -0400, Joel Fernandes wrote:
> On Sat, May 7, 2022 at 6:32 PM Paul E. McKenney <[email protected]> wrote:
> >
> > On Sat, May 07, 2022 at 11:11:58AM +0200, Uladzislau Rezki wrote:
> > > > On Fri, May 06, 2022 at 06:22:26PM +0200, Uladzislau Rezki wrote:
> > > > > > On Thu, May 05, 2022 at 12:16:41PM +0200, Uladzislau Rezki (Sony) wrote:
> > > > > > > Introduce a RCU_NOCB_CPU_CB_BOOST kernel option. So a user can
> > > > > > > decide if an offloading has to be done in a high-prio context or
> > > > > > > not. Please note an option depends on RCU_NOCB_CPU and RCU_BOOST
> > > > > > > parameters and by default it is off.
> > > > > > >
> > > > > > > This patch splits the boosting preempted RCU readers and those
> > > > > > > kthreads which directly responsible for driving expedited grace
> > > > > > > periods forward with enabling/disabling the offloading from/to
> > > > > > > SCHED_FIFO/SCHED_OTHER contexts.
> > > > > > >
> > > > > > > The main reason of such split is, for example on Android there
> > > > > > > are some workloads which require fast expedited grace period to
> > > > > > > be done whereas offloading in RT context can lead to starvation
> > > > > > > and hogging a CPU for a long time what is not acceptable for
> > > > > > > latency sensitive environment. For instance:
> > > > > > >
> > > > > > > <snip>
> > > > > > > <...>-60 [006] d..1 2979.028717: rcu_batch_start: rcu_preempt CBs=34619 bl=270
> > > > > > > <snip>
> > > > > > >
> > > > > > > invoking 34 619 callbacks will take time thus making other CFS
> > > > > > > tasks waiting in run-queue to be starved due to such behaviour.
> > > > > > >
> > > > > > > Signed-off-by: Uladzislau Rezki (Sony) <[email protected]>
> > > > > >
> > > > > > All good points!
> > > > > >
> > > > > > Some questions and comments below.
> > > > > >
> > > > > > Adding Sebastian on CC for his perspective.
> > > > > >
> > > > > > Thanx, Paul
> > > > > >
> > > > > > > ---
> > > > > > > kernel/rcu/Kconfig | 14 ++++++++++++++
> > > > > > > kernel/rcu/tree.c | 5 ++++-
> > > > > > > kernel/rcu/tree_nocb.h | 3 ++-
> > > > > > > 3 files changed, 20 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/kernel/rcu/Kconfig b/kernel/rcu/Kconfig
> > > > > > > index 27aab870ae4c..074630b94902 100644
> > > > > > > --- a/kernel/rcu/Kconfig
> > > > > > > +++ b/kernel/rcu/Kconfig
> > > > > > > @@ -275,6 +275,20 @@ config RCU_NOCB_CPU_DEFAULT_ALL
> > > > > > > Say Y here if you want offload all CPUs by default on boot.
> > > > > > > Say N here if you are unsure.
> > > > > > >
> > > > > > > +config RCU_NOCB_CPU_CB_BOOST
> > > > > > > + bool "Perform offloading from real-time kthread"
> > > > > > > + depends on RCU_NOCB_CPU && RCU_BOOST
> > > > > > > + default n
> > > > > >
> > > > > > I understand that you need this to default to "n" on your systems.
> > > > > > However, other groups already using callback offloading should not see
> > > > > > a sudden change. I don't see an Android-specific defconfig file, but
> > > > > > perhaps something in drivers/android/Kconfig?
> > > > > >
> > > We saw a sudden change when the priority was lifted up for rcuop kthreads.
> > > I would like to know the reason. As for Android, i would like to avoid
> > > it to be Android specific. It is better just to enable boosting by
> > > default for nocb kthreads.
> >
> > No, because that breaks an existing use case, which uses RCU_BOOST
> > to avoid OOM on busy systems.
> >
> > > > > > One easy way to make this work would be to invert the sense of this
> > > > > > Kconfig option ("RCU_NOCB_CB_NO_BOOST"?), continue having it default to
> > > > > > "n", but then select it somewhere in drivers/android/Kconfig. But I
> > > > > > would not be surprised if there is a better way.
> > >
> > > In that situation probably we should just enable it by default.
> >
> > You are within your rights to cause it to be enabled by default -within-
> > -Android-. You are -not- within your rights to break other workloads.
> >
> > If ChromeOS needs it too, they too can enable it -within- -ChromeOS-.
> >
> > It is not -that- hard, guys! ;-)
>
> I think on the topic of RT, +Steven Rostedt should chime in as well
> considering he wrote a good chunk of the RT scheduler ;-). Personally,
> I feel the issue of "rcu callback offload" threads running as RT or
> not should not be a matter of CONFIG option or the system in concern.
> Instead it should be a function of how many callbacks there are to
> run. The reason I say this is, RT threads should not be doing a lot
> of work anyway, lest they cause RT throttling and starvation of other
> threads.
This gets complicated surprisingly quickly. For but one example, you
would find yourself wanting time-based boosting, most likely before you
wanted boosting based on numbers of callbacks. And it is all too easy
to drive considerably complexity into the mix before proving that it is
really needed. Especially given how rare the need for RCU priority
boosting is to begin with.
> Also, I think it is wrong to assume that a certain kind of system will
> always have a certain number of callbacks to process at a time. That
> seems prone to poor design due to assumptions which may not always be
> true.
Who was assuming that? Uladzislau was measuring rather than assuming,
if that was what you were getting at. Or if you are thinking about
things like qhimark, your point is exactly why there is both a default
(which has worked quite well for a very long time) and the ability to
adjust based on the needs of your specific system.
> Can we not have 2 sets of RCU offload threads, one which operate at RT
> and only process few callbacks at a time, while another which is the
> lower priority CFS offload thread - executes whenever there is a lot
> of CBs pending? Just a thought.
How about if we start by solving the problems we know that we have?
> Otherwise, I feel like we might be again proliferating CONFIG options
> and increasing burden on the user to get it the CONFIG right.
I bet that we will solve this without adding any new Kconfig options.
And I bet that the burden is at worst on the device designer, not on
the user. Plus it is entirely possible that there might be a way to
automatically configure things to handle what we know about today,
again without adding Kconfig options.
Thanx, Paul
On Sun, May 8, 2022 at 5:32 PM Paul E. McKenney <[email protected]> wrote:
[...]
> > > > > > > One easy way to make this work would be to invert the sense of this
> > > > > > > Kconfig option ("RCU_NOCB_CB_NO_BOOST"?), continue having it default to
> > > > > > > "n", but then select it somewhere in drivers/android/Kconfig. But I
> > > > > > > would not be surprised if there is a better way.
> > > >
> > > > In that situation probably we should just enable it by default.
> > >
> > > You are within your rights to cause it to be enabled by default -within-
> > > -Android-. You are -not- within your rights to break other workloads.
> > >
> > > If ChromeOS needs it too, they too can enable it -within- -ChromeOS-.
> > >
> > > It is not -that- hard, guys! ;-)
> >
> > I think on the topic of RT, +Steven Rostedt should chime in as well
> > considering he wrote a good chunk of the RT scheduler ;-). Personally,
> > I feel the issue of "rcu callback offload" threads running as RT or
> > not should not be a matter of CONFIG option or the system in concern.
> > Instead it should be a function of how many callbacks there are to
> > run. The reason I say this is, RT threads should not be doing a lot
> > of work anyway, lest they cause RT throttling and starvation of other
> > threads.
>
> This gets complicated surprisingly quickly. For but one example, you
> would find yourself wanting time-based boosting, most likely before you
> wanted boosting based on numbers of callbacks. And it is all too easy
> to drive considerably complexity into the mix before proving that it is
> really needed. Especially given how rare the need for RCU priority
> boosting is to begin with.
I think this patch does not deal with or change the behavior of
dynamic priority boosting preempted RCU readers, but rather it makes
it such that the no-cb offload threads that execute the callbacks. So
I am not sure why you are talking about the boosting behavior of
preempted RCU readers? I was referring only to the nocb offload
kthreads which as I understand, Vlad *does not* want to run at RT
priority.
> > Also, I think it is wrong to assume that a certain kind of system will
> > always have a certain number of callbacks to process at a time. That
> > seems prone to poor design due to assumptions which may not always be
> > true.
>
> Who was assuming that? Uladzislau was measuring rather than assuming,
> if that was what you were getting at. Or if you are thinking about
> things like qhimark, your point is exactly why there is both a default
> (which has worked quite well for a very long time) and the ability to
> adjust based on the needs of your specific system.
I was merely saying that based on measurements make assumptions, but
in the real world the assumption may not be true, then everything
falls apart. Instead I feel, callback threads should be RT only if 1.
As you mentioned, the time based thing. 2. If the CB list is long and
there's lot of processing. But instead, if it is made a CONFIG option,
then that forces a fixed behavior which may fall apart in the real
world. I think adding more CONFIGs and special cases is more complex
but that's my opinion.
> > Can we not have 2 sets of RCU offload threads, one which operate at RT
> > and only process few callbacks at a time, while another which is the
> > lower priority CFS offload thread - executes whenever there is a lot
> > of CBs pending? Just a thought.
>
> How about if we start by solving the problems we know that we have?
I don't know why you would say that, because we are talking about
solving the specific problem Vlad's patch addresses, not random
problems. Which is that, Android wants to run expedited GPs, but when
the callback list is large, the RT nocb thread can starve other
things. Did I misunderstand the patch? If so, sorry about that but
that's what my email was discussing. i.e. running of CBs in RT
threads. I suck at writing well as I clearly miscommunicated.
> > Otherwise, I feel like we might be again proliferating CONFIG options
> > and increasing burden on the user to get it the CONFIG right.
>
> I bet that we will solve this without adding any new Kconfig options.
> And I bet that the burden is at worst on the device designer, not on
> the user. Plus it is entirely possible that there might be a way to
> automatically configure things to handle what we know about today,
> again without adding Kconfig options.
Yes, agreed.
Thanks,
Joel