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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200612125250.7bwjfnxhurdf5bwj@e107158-lin.cambridge.arm.com>
Date:   Fri, 12 Jun 2020 13:52:50 +0100
From:   Qais Yousef <qais.yousef@....com>
To:     Douglas Anderson <dianders@...omium.org>
Cc:     Benson Leung <bleung@...omium.org>,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        hsinyi@...omium.org, joelaf@...gle.com, peterz@...radead.org,
        drinkcat@...omium.org, gwendal@...omium.org, qperret@...gle.com,
        ctheegal@...eaurora.org, Guenter Roeck <groeck@...omium.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] cros_ec_spi: Even though we're RT priority, don't bump
 cpu freq

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/

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

Why can't you manage the priority and boost value of such tasks via your init
scripts instead?

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.

Thanks

--
Qais Yousef

> -- 
> 2.27.0.290.gba653c62da-goog
>