Re: [PATCH] cros_ec_spi: Even though we're RT priority, don't bump cpu freq

[Doug Anderson]

Hi,

On Fri, Jun 12, 2020 at 5:52 AM Qais Yousef <qais.yousef@xxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> > NOTE: This would cause a conflict if the patch
> > https://lore.kernel.org/r/20200422112831.870192415@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx/

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


> >       };
> >       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?


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


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

-Doug