Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200610151818.1.I666ecd9c6f3c6405bd75831a21001b8109b6438c@changeid>
 <20200611110301.GA132747@google.com>
In-Reply-To: <20200611110301.GA132747@google.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Thu, 11 Jun 2020 10:48:40 -0700
Message-ID: <CAD=FV=V2FvFcYrghxUSdHNxmcS3DKpnBbk2oL64w7hh=tV-DfQ@mail.gmail.com>
Subject: Re: [PATCH] cros_ec_spi: Even though we're RT priority, don't bump
 cpu freq
To:     Quentin Perret <qperret@google.com>
Cc:     Benson Leung <bleung@chromium.org>,
        Enric Balletbo i Serra <enric.balletbo@collabora.com>,
        hsinyi@chromium.org, Joel Fernandes <joelaf@google.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Nicolas Boichat <drinkcat@chromium.org>,
        Gwendal Grignou <gwendal@chromium.org>,
        ctheegal@codeaurora.org, Guenter Roeck <groeck@chromium.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi,

On Thu, Jun 11, 2020 at 4:03 AM Quentin Perret <qperret@google.com> wrote:
>
> Hi Doug,
>
> On Wednesday 10 Jun 2020 at 15:18:43 (-0700), 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.
>
> Is this specific to this driver, or something you would want applied
> more globally to all RT tasks in ChromeOS (which is what we'd like to
> have in Android for instance)?

Hrm.  I guess my first instinct is to say that we still want this
patch even if we have something that is applied more globally.
Specifically it sounds as if the patch you point at is suggesting that
we'd tweak the boost value to something other than max but we'd still
have a boost value.  In the case of cros_ec_spi I don't believe we
need any boost value at all, so my patch would still be useful.  The
computational needs of cros_ec_spi are very modest and it can do its
work at lower CPU frequencies just fine.  It just can't be interrupted
for large swaths of time.


> IOW, how do you feel about 20200511154053.7822-1-qais.yousef@arm.com ?

I'm not totally a fan, but I'm definitely not an expert in this area
(I've also only read the patch description and not the patch or the
whole thread).  I really don't want yet another value that I need to
tune from board to board.  Even worse, this tuning value isn't
board-specific but a combination of board and software specific.  By
this, I'd imagine a scenario where you're using a real-time task to
get audio decoding done within a certain latency.  I guess you'd tune
this value to make sure that you can get all your audio decoding done
in time but also not burn extra power.  Now, imagine that the OS
upgrades and the audio task suddenly has to decode more complex
streams.  You've got to go across all of your boards and re-tune every
one?  ...or, nobody thinks about it and older boards start getting
stuttery audio?  Perhaps the opposite happens and someone comes up
with a newer lower-cpu-intensive codec and you could save power.
Sounds like a bit of a nightmare.

I'd rather have a boolean value: boost all RT threads to max vs. don't
boost all RT threads to max.  Someone that just wanted RT stuff to run
as fast as possible without any hassle on their system and didn't care
about power efficiency could turn this on.  Anyone who really cared
about power could turn this off and then could find a more targeted
way to boost things, hopefully in a way that doesn't require tuning.
One option would be to still boost the CPU to max but only for certain
tasks known to be really latency sensitive.  Another might be to
somehow measure whether or not the task is making its deadlines and
boost the CPU frequency up if deadlines are not being met.  I'm sure
there are fancier ways.

...of course, I believe your patch allows me to do what I want: I can
just set the default boost to 0.  It just leaves in the temptation for
others to require a default boost of something else and then I'm stuck
in my tuning nightmare.

-Doug

-Doug