lkml.org 
[lkml]   [2020]   [Jun]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@arm.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@chromium.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

\
 
 \ /
  Last update: 2020-06-19 17:39    [W:0.086 / U:1.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site