Date:   Fri, 19 Jun 2020 16:38:51 +0100
From:   Qais Yousef <qais.yousef@....com>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Benson Leung <bleung@...omium.org>,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        hsinyi@...omium.org, Joel Fernandes <joelaf@...gle.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Nicolas Boichat <drinkcat@...omium.org>,
        Gwendal Grignou <gwendal@...omium.org>,
        Quentin Perret <qperret@...gle.com>, ctheegal@...eaurora.org,
        Guenter Roeck <groeck@...omium.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] cros_ec_spi: Even though we're RT priority, don't bump
 cpu freq

On 06/18/20 14:18, Doug Anderson wrote:
> Hi,
> 
> On Fri, Jun 12, 2020 at 5:52 AM Qais Yousef <qais.yousef@....com> wrote:
> >
> > On 06/10/20 15:18, Douglas Anderson wrote:
> > > The cros_ec_spi driver is realtime priority so that it doesn't get
> > > preempted by other taks while it's talking to the EC but overall it
> > > really doesn't need lots of compute power.  Unfortunately, by default,
> > > the kernel assumes that all realtime tasks should cause the cpufreq to
> > > jump to max and burn through power to get things done as quickly as
> > > possible.  That's just not the correct behavior for cros_ec_spi.
> > >
> > > Switch to manually overriding the default.
> > >
> > > This won't help us if our work moves over to the SPI pump thread but
> > > that's not the common code path.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@...omium.org>
> > > ---
> > > NOTE: This would cause a conflict if the patch
> > > https://lore.kernel.org/r/20200422112831.870192415@infradead.org lands
> > > first
> > >
> > >  drivers/platform/chrome/cros_ec_spi.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/platform/chrome/cros_ec_spi.c b/drivers/platform/chrome/cros_ec_spi.c
> > > index debea5c4c829..76d59d5e7efd 100644
> > > --- a/drivers/platform/chrome/cros_ec_spi.c
> > > +++ b/drivers/platform/chrome/cros_ec_spi.c
> > > @@ -709,8 +709,11 @@ static void cros_ec_spi_high_pri_release(void *worker)
> > >  static int cros_ec_spi_devm_high_pri_alloc(struct device *dev,
> > >                                          struct cros_ec_spi *ec_spi)
> > >  {
> > > -     struct sched_param sched_priority = {
> > > -             .sched_priority = MAX_RT_PRIO / 2,
> > > +     struct sched_attr sched_attr = {
> > > +             .sched_policy   = SCHED_FIFO,
> > > +             .sched_priority = MAX_RT_PRIO / 2,
> > > +             .sched_flags    = SCHED_FLAG_UTIL_CLAMP_MIN,
> > > +             .sched_util_min = 0,
> >
> > Hmm I thought Peter already removed all users that change RT priority directly.
> >
> > https://lore.kernel.org/lkml/20200422112719.826676174@infradead.org/
> 
> Yeah, I mentioned that patch series "after the cut" in my patch and
> also made sure to CC Peter on my patch.  I'm not sure what happened to
> that series since I don't see it in linuxnext...

Apologies I missed that.

> 
> 
> > >       };
> > >       int err;
> > >
> > > @@ -728,8 +731,7 @@ static int cros_ec_spi_devm_high_pri_alloc(struct device *dev,
> > >       if (err)
> > >               return err;
> > >
> > > -     err = sched_setscheduler_nocheck(ec_spi->high_pri_worker->task,
> > > -                                      SCHED_FIFO, &sched_priority);
> > > +     err = sched_setattr_nocheck(ec_spi->high_pri_worker->task, &sched_attr);
> > >       if (err)
> > >               dev_err(dev, "Can't set cros_ec high pri priority: %d\n", err);
> > >       return err;
> >
> > If I read the code correctly, if you fail here cros_ec_spi_probe() will fail
> > too and the whole thing will not be loaded. If you compile without uclamp then
> > sched_setattr_nocheck() will always fail with -EOPNOTSUPP.
> 
> Oops, definitely need to send out a v2 for that if nothing else.  Is
> there any good way for me to code this up or do I need a big #ifdef
> somewhere in my code?

A big #ifdef. But this kind of use I don't think was anticipated. And generally
if we want to allow that, it has to be done via a proper API. Drivers picking
random uclamp values is as bad as them picking random RT priority.

> 
> 
> > Why can't you manage the priority and boost value of such tasks via your init
> > scripts instead?
> 
> I guess I feel like it's weird in this case.  Userspace isn't creating
> this task and isn't the one marking it as realtime.  ...and, if we
> ever manage to upgrade the protocol which we use to talk to the EC we
> might fully get rid of this task the need to have something boosted up
> to realtime.
> 
> Said another way: the fact that we even have this task at all and the
> fact that it's realtime is an implementation detail of the kernel.  It
> seems really weird to add initscripts for it.

Yes this is the problem of RT for a general purpose systems. It's hard to
reason about their priorities/importance since it's not a special purpose
system with well defined spec of what hardware/software will be running on it
and their precise requirements is not known before hand.

> 
> 
> > I have to admit I need to think about whether it makes sense to have a generic
> > API that allows drivers to opt-out of the default boosting behavior for their
> > RT tasks.
> 
> Seems like it would be useful.

If you propose something that will help the discussion. I think based on the
same approach Peter has taken to prevent random RT priorities. In uclamp case
I think we just want to allow driver to opt RT tasks out of the default
boosting behavior.

I'm a bit wary that this extra layer of tuning might create a confusion, but
I can't reason about why is it bad for a driver to say I don't want my RT task
to be boosted too.

Thanks

--
Qais Yousef

Powered by blists - more mailing lists