[PATCH] PM/QoS: resume latency 0 should be not latency allowed

[PATCH] PM/QoS: resume latency 0 should be not latency allowed

[Youquan Song]

ICX and more recent Intel CPU has C1 exit_latency equal 1 in intel_idle
driver, There are PM QoS interfaces: global /dev/cpu_dma_latency and 
per-device latency constraints. per-CPU set pm_qos_resume_latency_us
to limit single CPU entery into deepest C-state. 
When it is required to disable entry into even C1, we need to set "0"
 to /sys pm_qos_resume_latency_us, but "0" of pm_qos_resume_latency_us
is redefined to no constraint now. 

so this patch set "0" back to original meaning "no latency at all".

Signed-off-by: Youquan Song <youquan.song@intel.com>
---
 drivers/base/power/sysfs.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index a1474fb67db9..b4bd67e6f440 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -214,10 +214,8 @@ static ssize_t pm_qos_resume_latency_us_show(struct device *dev,
 {
 	s32 value = dev_pm_qos_requested_resume_latency(dev);
 
-	if (value == 0)
-		return sysfs_emit(buf, "n/a\n");
 	if (value == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
-		value = 0;
+		return sysfs_emit(buf, "no constraint\n");
 
 	return sysfs_emit(buf, "%d\n", value);
 }
@@ -237,10 +235,8 @@ static ssize_t pm_qos_resume_latency_us_store(struct device *dev,
 		if (value < 0 || value == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
 			return -EINVAL;
 
-		if (value == 0)
-			value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
 	} else if (sysfs_streq(buf, "n/a")) {
-		value = 0;
+		value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
 	} else {
 		return -EINVAL;
 	}
-- 
2.27.0

Re: [PATCH] PM/QoS: resume latency 0 should be not latency allowed

[Rafael J. Wysocki]

On Mon, Nov 1, 2021 at 1:50 PM Youquan Song <youquan.song@intel.com> wrote:
>
> ICX and more recent Intel CPU has C1 exit_latency equal 1 in intel_idle

Please expand "ICX".

> driver, There are PM QoS interfaces: global /dev/cpu_dma_latency and
> per-device latency constraints. per-CPU set pm_qos_resume_latency_us
> to limit single CPU entery into deepest C-state.
> When it is required to disable entry into even C1, we need to set "0"
>  to /sys pm_qos_resume_latency_us, but "0" of pm_qos_resume_latency_us
> is redefined to no constraint now.
>
> so this patch set "0" back to original meaning "no latency at all".

It is not "back", it simply changes the meaning of 0.  And that's why
Documentation/ABI/testing/sysfs-devices-power needs to be updated.

> Signed-off-by: Youquan Song <youquan.song@intel.com>
> ---
>  drivers/base/power/sysfs.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index a1474fb67db9..b4bd67e6f440 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -214,10 +214,8 @@ static ssize_t pm_qos_resume_latency_us_show(struct device *dev,
>  {
>         s32 value = dev_pm_qos_requested_resume_latency(dev);
>
> -       if (value == 0)
> -               return sysfs_emit(buf, "n/a\n");
>         if (value == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
> -               value = 0;
> +               return sysfs_emit(buf, "no constraint\n");
>
>         return sysfs_emit(buf, "%d\n", value);
>  }
> @@ -237,10 +235,8 @@ static ssize_t pm_qos_resume_latency_us_store(struct device *dev,
>                 if (value < 0 || value == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
>                         return -EINVAL;
>
> -               if (value == 0)
> -                       value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
>         } else if (sysfs_streq(buf, "n/a")) {
> -               value = 0;
> +               value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
>         } else {
>                 return -EINVAL;
>         }
> --

But do you really need to make these changes?  What problem is there
with using the interface as currently documented, ie. writing "n/a" to
it to get "no latency at all"?

Re: [PATCH] PM/QoS: resume latency 0 should be not latency allowed

[Rafael J. Wysocki]

On 11/10/2021 11:58 AM, Youquan Song wrote:
>> But do you really need to make these changes?  What problem is there with using the interface as currently documented, ie. writing "n/a" to it to get "no latency at all"?
> I think so. "0" is "latency is not allowed", but "n/a" is "no latency".
>
Actually, "0" means "I don't care" and "n/a" means "no latency" which 
means latency tolerance == 0.

Re: [PATCH] PM/QoS: resume latency 0 should be not latency allowed

[Youquan Song]

On Wed, Nov 10, 2021 at 08:24:12PM +0100, Rafael J. Wysocki wrote:
> On 11/10/2021 11:58 AM, Youquan Song wrote:
> >>But do you really need to make these changes?  What problem is there with using the interface as currently documented, ie. writing "n/a" to it to get "no latency at all"?
> >I think so. "0" is "latency is not allowed", but "n/a" is "no latency".
> >
> Actually, "0" means "I don't care" and "n/a" means "no latency"
> which means latency tolerance == 0.
So it is not reasonable and need change. "0" is the smallest latency and means "latency not allowed"; if latency_req==0, it will not allow to enter into C-state. "n/a" is largest latency and means "I don't care" the input of latency_req at all. 

Re: [PATCH] PM/QoS: resume latency 0 should be not latency allowed

[Rafael J. Wysocki]

On Thu, Nov 11, 2021 at 6:40 AM Youquan Song
<youquan.song@linux.intel.com> wrote:
>
> On Wed, Nov 10, 2021 at 08:24:12PM +0100, Rafael J. Wysocki wrote:
> > On 11/10/2021 11:58 AM, Youquan Song wrote:
> > >>But do you really need to make these changes?  What problem is there with using the interface as currently documented, ie. writing "n/a" to it to get "no latency at all"?
> > >I think so. "0" is "latency is not allowed", but "n/a" is "no latency".
> > >
> > Actually, "0" means "I don't care" and "n/a" means "no latency"
> > which means latency tolerance == 0.
> So it is not reasonable and need change.

Why isn't it?

It may not be intuitive, but it doesn't imply "unreasonable".

> "0" is the smallest latency and means "latency not allowed";

Not in this interface.  The documentation is very clear in that respect.

> if latency_req==0, it will not allow to enter into C-state. "n/a" is largest latency and means "I don't care" the input of latency_req at all.

I'm afraid that the existing interface cannot be changed without
introducing regressions for some users.

Please use it as documented or demonstrate to me why it cannot be used
as documented.