On 06/18/20 14:18, Doug Anderson wrote: > Hi, > > On Fri, Jun 12, 2020 at 5:52 AM Qais Yousef <[email protected]> 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 <[email protected]> > > > --- > > > NOTE: This would cause a conflict if the patch > > > https://lore.kernel.org/r/[email protected] 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/[email protected]/ > > 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
