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

[Doug Anderson]

Hi,

On Fri, Jun 19, 2020 at 8:38 AM Qais Yousef <qais.yousef@xxxxxxx> wrote:
>
> On 06/18/20 14:18, Doug Anderson wrote:
> > 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...
>
> 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.

Right.  I was basically just trying to say "turn my boosting off".

...so I guess you're saying that doing a v2 of my patch with the
proper #ifdef protection wouldn't be a good way to go and I'd need to
propose some sort of API for this?

-Doug