Message-ID: <CAPDyKFo6DT0cagnEQ61_UkEet6Ao60xfFM=YJ3VQqUYH8KeJyg@mail.gmail.com>
Date: Fri, 31 Oct 2025 11:03:47 +0100
From: Ulf Hansson <ulf.hansson@...aro.org>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: linux-pm@...r.kernel.org, Vincent Guittot <vincent.guittot@...aro.org>, 
	Peter Zijlstra <peterz@...radead.org>, Kevin Hilman <khilman@...libre.com>, 
	Pavel Machek <pavel@...nel.org>, Len Brown <len.brown@...el.com>, 
	Daniel Lezcano <daniel.lezcano@...aro.org>, Saravana Kannan <saravanak@...gle.com>, 
	Maulik Shah <quic_mkshah@...cinc.com>, Prasad Sodagudi <psodagud@...cinc.com>, 
	Dhruva Gole <d-gole@...com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/4] PM: QoS: Introduce a CPU system-wakeup QoS limit
 for s2idle

On Thu, 30 Oct 2025 at 17:36, Rafael J. Wysocki <rafael@...nel.org> wrote:
>
> On Thu, Oct 30, 2025 at 4:13 PM Ulf Hansson <ulf.hansson@...aro.org> wrote:
> >
> > On Thu, 30 Oct 2025 at 15:06, Rafael J. Wysocki <rafael@...nel.org> wrote:
> > >
> > > On Thu, Oct 30, 2025 at 1:44 PM Ulf Hansson <ulf.hansson@...aro.org> wrote:
> > > >
> > > > On Thu, 30 Oct 2025 at 13:29, Rafael J. Wysocki <rafael@...nel.org> wrote:
> > > > >
> > > > > On Thu, Oct 30, 2025 at 1:26 PM Rafael J. Wysocki <rafael@...nel.org> wrote:
> > > > > >
> > > > > > On Thu, Oct 30, 2025 at 1:23 PM Ulf Hansson <ulf.hansson@...aro.org> wrote:
> > > > > > >
> > > > > > > On Wed, 29 Oct 2025 at 15:53, Rafael J. Wysocki <rafael@...nel.org> wrote:
> > > > > > > >
> > > > > > > > On Thu, Oct 16, 2025 at 5:19 PM Ulf Hansson <ulf.hansson@...aro.org> wrote:
> > > > > > > > >
> > > > > > > > > Changes in v2:
> > > > > > > > >         - Limit the new QoS to CPUs  and make some corresponding renaming of the
> > > > > > > > >         functions along with name of the device node for user space.
> > > > > > > > >         - Make sure we deal with the failure/error path correctly when there are
> > > > > > > > >         no state available for s2idle.
> > > > > > > > >         - Add documentation.
> > > > > > > > >
> > > > > > > > > Some platforms supports multiple low-power states for CPUs that can be used
> > > > > > > > > when entering system-wide suspend and s2idle in particular. Currently we are
> > > > > > > > > always selecting the deepest possible state for the CPUs, which can break the
> > > > > > > > > system-wakeup latency constraint that may be required for some use-cases.
> > > > > > > > >
> > > > > > > > > Therefore, this series suggests to introduce a new interface for user-space,
> > > > > > > > > allowing us to specify the CPU system-wakeup QoS limit. The QoS limit is then
> > > > > > > > > taken into account when selecting a suitable low-power state for s2idle.
> > > > > > > >
> > > > > > > > Last time we discussed this I said I would like the new limit to be
> > > > > > > > taken into account by regular "runtime" cpuidle because the "s2idle"
> > > > > > > > limit should not be less that the "runtime" limit (or at least it
> > > > > > > > would be illogical if that happened).
> > > > > > >
> > > > > > > Yes, we discussed this, but that was also before we concluded to add a
> > > > > > > new file for user-space to operate on after all.
> > > > > > >
> > > > > > > To me, it looks unnecessarily limiting to not allow them to be
> > > > > > > orthogonal,
> > > > > >
> > > > > > So what's the use case in which it makes sense to have a lower latency
> > > > > > limit for s2idle than for runtime?
> > > >
> > > > Honestly, I don't know, but I just wanted to keep things more flexible.
> > > >
> > > > > >
> > > > > > > but I am not insisting that it needs to be like this. I
> > > > > > > was just thinking that we do not necessarily have to care about the
> > > > > > > same use-case in runtime as in the system-suspend state. Moreover,
> > > > > > > nothing would prevent user-space from applying the same constraint to
> > > > > > > both of them, if that is needed.
> > > > > > >
> > > > > > > >
> > > > > > > > It looks like that could be implemented by making
> > > > > > > > cpuidle_governor_latency_req() take cpu_wakeup_latency_qos_limit()
> > > > > > > > into account, couldn't it?
> > > > > > >
> > > > > > > Right, but I am not sure we want that. See above.
> > > > > >
> > > > > > I do or I need to be convinced that this is a bad idea.
> > > > >
> > > > > And there is a specific reason why I want that.
> > > > >
> > > > > Namely, say somebody wants to set the same limit for both s2idle and
> > > > > "runtime" cpuidle.  If the s2idle limit did not affect "runtime", they
> > > > > would need to open two device special files and write the same value
> > > > > to both of them.  Otherwise, they just need to use the s2idle limit
> > > > > and it will work for "runtime" automatically.
> > > >
> > > > Right. User-space would need to open two files instead of one, but is
> > > > that really a problem?
> > >
> > > It is potentially confusing and error-prone.
> > >
> > > > What if user-space doesn't want to affect the runtime state-selection,
> > > > but cares only about a use-case that requires a cpu-wakeup constraint
> > > > when resuming from s2idle.
> > >
> > > Well, I'm not sure if this use case exists at all, which is key here.
> > > If it doesn't exist, why make provisions for it?
> >
> > Well, because it's not possible to change afterwards as it becomes ABI.
> >
> > It would be silly having to add yet another file for userspace, down
> > the road, if it turns out to be needed.
>
> OTOH doing things without a good reason is also not particularly wise.

Right. As I said, I don't have a use-case at hand at this very moment
for why we shouldn't combine the constraints as you propose.

I will send a new version, unless someone speaks up and suggests a
different approach.

Kind regards
Uffe

Powered by blists - more mailing lists