Re: [RFT][PATCH 2/2] PM / QoS: Fix device resume latency framework

From: Rafael J. Wysocki
Date: Fri Nov 03 2017 - 03:58:54 EST

Next message: Juergen Gross: "Re: [PATCH] xen/time: Return -ENODEV from xen_get_wallclock()"
Previous message: Huang, Ying: "[RFC -mm] mm, userfaultfd, THP: Avoid waiting when PMD under THP migration"
In reply to: Ramesh Thomas: "Re: [RFT][PATCH 2/2] PM / QoS: Fix device resume latency framework"
Next in thread: Rafael J. Wysocki: "Re: [RFT][PATCH 2/2] PM / QoS: Fix device resume latency framework"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Nov 3, 2017 at 8:43 AM, Ramesh Thomas <ramesh.thomas@xxxxxxxxx> wrote:
> On 2017-11-02 at 00:03:54 +0100, Rafael J. Wysocki wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>>
>> The special value of 0 for device resume latency PM QoS means
>> "no restriction", but there are two problems with that.
>>
>> First, device resume latency PM QoS requests with 0 as the
>> value are always put in front of requests with positive
>> values in the priority lists used internally by the PM QoS
>> framework, causing 0 to be chosen as an effective constraint
>> value. However, that 0 is then interpreted as "no restriction"
>> effectively overriding the other requests with specific
>> restrictions which is incorrect.
>>
>> Second, the users of device resume latency PM QoS have no
>> way to specify that *any* resume latency at all should be
>> avoided, which is an artificial limitation in general.
>>
>> To address these issues, modify device resume latency PM QoS to
>> use S32_MAX as the "no constraint" value and 0 as the "no
>> latency at all" one and rework its users (the cpuidle menu
>> governor, the genpd QoS governor and the runtime PM framework)
>> to follow these changes.
>>
>> Also add a special "n/a" value to the corresponding user space I/F
>> to allow user space to indicate that it cannot accept any resume
>> latencies at all for the given device.
>>
>> Fixes: 85dc0b8a4019 (PM / QoS: Make it possible to expose PM QoS latency constraints)
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=197323
>> Reported-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>> ---
>> Documentation/ABI/testing/sysfs-devices-power | 4 ++-
>> drivers/base/cpu.c | 3 +-
>> drivers/base/power/domain.c | 2 -
>> drivers/base/power/domain_governor.c | 27 ++++++++++++--------------
>> drivers/base/power/qos.c | 5 +++-
>> drivers/base/power/runtime.c | 2 -
>> drivers/base/power/sysfs.c | 25 ++++++++++++++++++++----
>> drivers/cpuidle/governors/menu.c | 4 +--
>> include/linux/pm_qos.h | 24 +++++++++++++++--------
>> 9 files changed, 63 insertions(+), 33 deletions(-)
>>
>> Index: linux-pm/drivers/base/power/sysfs.c
>> ===================================================================
>> --- linux-pm.orig/drivers/base/power/sysfs.c
>> +++ linux-pm/drivers/base/power/sysfs.c
>> @@ -218,7 +218,14 @@ static ssize_t pm_qos_resume_latency_sho
>> struct device_attribute *attr,
>> char *buf)
>> {
>> - return sprintf(buf, "%d\n", dev_pm_qos_requested_resume_latency(dev));
>> + s32 value = dev_pm_qos_requested_resume_latency(dev);
>> +
>> + if (value == 0)
>> + return sprintf(buf, "n/a\n");
>> + else if (value == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
>> + value = 0;
>> +
>> + return sprintf(buf, "%d\n", value);
>> }
>>
>> static ssize_t pm_qos_resume_latency_store(struct device *dev,
>> @@ -228,11 +235,21 @@ static ssize_t pm_qos_resume_latency_sto
>> s32 value;
>> int ret;
>>
>> - if (kstrtos32(buf, 0, &value))
>> - return -EINVAL;
>> + if (!kstrtos32(buf, 0, &value)) {
>> + /*
>> + * Prevent users from writing negative or "no constraint" values
>> + * directly.
>> + */
>> + if (value < 0 || value == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
>> + return -EINVAL;
>>
>> - if (value < 0)
>> + if (value == 0)
>> + value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
>> + } else if (!strcmp(buf, "n/a") || !strcmp(buf, "n/a\n")) {
>> + value = 0;
>> + } else {
>> return -EINVAL;
>> + }
>>
>> ret = dev_pm_qos_update_request(dev->power.qos->resume_latency_req,
>> value);
>> Index: linux-pm/include/linux/pm_qos.h
>> ===================================================================
>> --- linux-pm.orig/include/linux/pm_qos.h
>> +++ linux-pm/include/linux/pm_qos.h
>> @@ -27,16 +27,17 @@ enum pm_qos_flags_status {
>> PM_QOS_FLAGS_ALL,
>> };
>>
>> -#define PM_QOS_DEFAULT_VALUE -1
>> +#define PM_QOS_DEFAULT_VALUE (-1)
>> +#define PM_QOS_LATENCY_ANY S32_MAX
>>
>> #define PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
>> #define PM_QOS_NETWORK_LAT_DEFAULT_VALUE (2000 * USEC_PER_SEC)
>> #define PM_QOS_NETWORK_THROUGHPUT_DEFAULT_VALUE 0
>> #define PM_QOS_MEMORY_BANDWIDTH_DEFAULT_VALUE 0
>> -#define PM_QOS_RESUME_LATENCY_DEFAULT_VALUE 0
>> +#define PM_QOS_RESUME_LATENCY_DEFAULT_VALUE PM_QOS_LATENCY_ANY
>> +#define PM_QOS_RESUME_LATENCY_NO_CONSTRAINT PM_QOS_LATENCY_ANY
>> #define PM_QOS_LATENCY_TOLERANCE_DEFAULT_VALUE 0
>> #define PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT (-1)
>> -#define PM_QOS_LATENCY_ANY ((s32)(~(__u32)0 >> 1))
>>
>> #define PM_QOS_FLAG_NO_POWER_OFF (1 << 0)
>>
>> @@ -173,7 +174,8 @@ static inline s32 dev_pm_qos_requested_f
>> static inline s32 dev_pm_qos_raw_read_value(struct device *dev)
>> {
>> return IS_ERR_OR_NULL(dev->power.qos) ?
>> - 0 : pm_qos_read_value(&dev->power.qos->resume_latency);
>> + PM_QOS_RESUME_LATENCY_NO_CONSTRAINT :
>> + pm_qos_read_value(&dev->power.qos->resume_latency);
>> }
>> #else
>> static inline enum pm_qos_flags_status __dev_pm_qos_flags(struct device *dev,
>> @@ -183,9 +185,9 @@ static inline enum pm_qos_flags_status d
>> s32 mask)
>> { return PM_QOS_FLAGS_UNDEFINED; }
>> static inline s32 __dev_pm_qos_read_value(struct device *dev)
>> - { return 0; }
>> + { return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT; }
>> static inline s32 dev_pm_qos_read_value(struct device *dev)
>> - { return 0; }
>> + { return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT; }
>> static inline int dev_pm_qos_add_request(struct device *dev,
>> struct dev_pm_qos_request *req,
>> enum dev_pm_qos_req_type type,
>> @@ -231,9 +233,15 @@ static inline int dev_pm_qos_expose_late
>> { return 0; }
>> static inline void dev_pm_qos_hide_latency_tolerance(struct device *dev) {}
>>
>> -static inline s32 dev_pm_qos_requested_resume_latency(struct device *dev) { return 0; }
>> +static inline s32 dev_pm_qos_requested_resume_latency(struct device *dev)
>> +{
>> + return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
>> +}
>> static inline s32 dev_pm_qos_requested_flags(struct device *dev) { return 0; }
>> -static inline s32 dev_pm_qos_raw_read_value(struct device *dev) { return 0; }
>> +static inline s32 dev_pm_qos_raw_read_value(struct device *dev)
>> +{
>> + return PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
>> +}
>> #endif
>>
>> #endif
>> Index: linux-pm/drivers/cpuidle/governors/menu.c
>> ===================================================================
>> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
>> +++ linux-pm/drivers/cpuidle/governors/menu.c
>> @@ -298,8 +298,8 @@ static int menu_select(struct cpuidle_dr
>> data->needs_update = 0;
>> }
>>
>> - /* resume_latency is 0 means no restriction */
>> - if (resume_latency && resume_latency < latency_req)
>> + if (resume_latency < latency_req &&
>> + resume_latency != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
>> latency_req = resume_latency;
>>
>> /* Special case when user has set very strict latency requirement */
>> Index: linux-pm/drivers/base/power/runtime.c
>> ===================================================================
>> --- linux-pm.orig/drivers/base/power/runtime.c
>> +++ linux-pm/drivers/base/power/runtime.c
>> @@ -253,7 +253,7 @@ static int rpm_check_suspend_allowed(str
>> || (dev->power.request_pending
>> && dev->power.request == RPM_REQ_RESUME))
>> retval = -EAGAIN;
>> - else if (__dev_pm_qos_read_value(dev) < 0)
>> + else if (__dev_pm_qos_read_value(dev) == 0)
>> retval = -EPERM;
>> else if (dev->power.runtime_status == RPM_SUSPENDED)
>> retval = 1;
>> Index: linux-pm/drivers/base/cpu.c
>> ===================================================================
>> --- linux-pm.orig/drivers/base/cpu.c
>> +++ linux-pm/drivers/base/cpu.c
>> @@ -377,7 +377,8 @@ int register_cpu(struct cpu *cpu, int nu
>>
>> per_cpu(cpu_sys_devices, num) = &cpu->dev;
>> register_cpu_under_node(num, cpu_to_node(num));
>> - dev_pm_qos_expose_latency_limit(&cpu->dev, 0);
>> + dev_pm_qos_expose_latency_limit(&cpu->dev,
>> + PM_QOS_RESUME_LATENCY_NO_CONSTRAINT);
>>
>> return 0;
>> }
>> Index: linux-pm/drivers/base/power/qos.c
>> ===================================================================
>> --- linux-pm.orig/drivers/base/power/qos.c
>> +++ linux-pm/drivers/base/power/qos.c
>> @@ -139,6 +139,9 @@ static int apply_constraint(struct dev_p
>>
>> switch(req->type) {
>> case DEV_PM_QOS_RESUME_LATENCY:
>> + if (WARN_ON(value < 0))
>> + value = 0;
>> +
>> ret = pm_qos_update_target(&qos->resume_latency,
>> &req->data.pnode, action, value);
>> break;
>> @@ -189,7 +192,7 @@ static int dev_pm_qos_constraints_alloca
>> plist_head_init(&c->list);
>> c->target_value = PM_QOS_RESUME_LATENCY_DEFAULT_VALUE;
>> c->default_value = PM_QOS_RESUME_LATENCY_DEFAULT_VALUE;
>> - c->no_constraint_value = PM_QOS_RESUME_LATENCY_DEFAULT_VALUE;
>> + c->no_constraint_value = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
>> c->type = PM_QOS_MIN;
>> c->notifiers = n;
>>
>> Index: linux-pm/Documentation/ABI/testing/sysfs-devices-power
>> ===================================================================
>> --- linux-pm.orig/Documentation/ABI/testing/sysfs-devices-power
>> +++ linux-pm/Documentation/ABI/testing/sysfs-devices-power
>> @@ -211,7 +211,9 @@ Description:
>> device, after it has been suspended at run time, from a resume
>> request to the moment the device will be ready to process I/O,
>> in microseconds. If it is equal to 0, however, this means that
>> - the PM QoS resume latency may be arbitrary.
>> + the PM QoS resume latency may be arbitrary and the special value
>> + "n/a" means that user space cannot accept any resume latency at
>> + all for the given device.
>>
>> Not all drivers support this attribute. If it isn't supported,
>> it is not present.
>> Index: linux-pm/drivers/base/power/domain.c
>> ===================================================================
>> --- linux-pm.orig/drivers/base/power/domain.c
>> +++ linux-pm/drivers/base/power/domain.c
>> @@ -1331,7 +1331,7 @@ static struct generic_pm_domain_data *ge
>>
>> gpd_data->base.dev = dev;
>> gpd_data->td.constraint_changed = true;
>> - gpd_data->td.effective_constraint_ns = 0;
>> + gpd_data->td.effective_constraint_ns = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
>> gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
>>
>> spin_lock_irq(&dev->power.lock);
>> Index: linux-pm/drivers/base/power/domain_governor.c
>> ===================================================================
>> --- linux-pm.orig/drivers/base/power/domain_governor.c
>> +++ linux-pm/drivers/base/power/domain_governor.c
>> @@ -26,11 +26,11 @@ static int dev_update_qos_constraint(str
>> * to take effect, the device has to be resumed and suspended again.
>> */
>> constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
>> - /* 0 means "no constraint" */
>> - if (constraint_ns == 0)
>> + if (constraint_ns == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
>> return 0;
>>
>> - if (constraint_ns < *constraint_ns_p || *constraint_ns_p == 0)
>> + if (constraint_ns < *constraint_ns_p ||
>> + *constraint_ns_p == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
>
> Check for constraint_ns < *constraint_ns_p should be enough because
> PM_QOS_RESUME_LATENCY_NO_CONSTRAINT = S32_MAX which is the largest possible
> value, if that can be assumed here.

Well, not quite. effective_constraint_ns is s64 and it is the QoS
value multiplied by NSEC_PER_USEC.

Surely can be greater than PM_QOS_RESUME_LATENCY_NO_CONSTRAINT.

Which reminds me about the possible collision in default_suspend_ok()
which needs to be addressed. :-)

>> *constraint_ns_p = constraint_ns;
>>
>> return 0;
>> @@ -58,12 +58,12 @@ static bool default_suspend_ok(struct de
>> }
>> td->constraint_changed = false;
>> td->cached_suspend_ok = false;
>> - td->effective_constraint_ns = -1;
>> + td->effective_constraint_ns = 0;
>> constraint_ns = __dev_pm_qos_read_value(dev);
>
> Why not initialize td->effective_constraint_ns with what is read by
> __dev_pm_qos_read_value()? Even though this value will stay only if false
> is returned, in which case it does not matter anyway, it looks like more
> meaningful value. Also PM_QOS_RESUME_LATENCY_NO_CONSTRAINT may be ok as it
> is initialized with at creation.

This looks like a leftover or similar, thanks.

>>
>> spin_unlock_irqrestore(&dev->power.lock, flags);
>>
>> - if (constraint_ns < 0)
>> + if (constraint_ns == 0)
>> return false;
>>
>> constraint_ns *= NSEC_PER_USEC;
>
> This will go wrong if PM_QOS_RESUME_LATENCY_NO_CONSTRAINT was the value
> Code below and in dev_update_qos_constraint check for that special value.
>
> May be those places can check PM_QOS_RESUME_LATENCY_NO_CONSTRAINT * NSEC_PER_USEC
> There may be other places that this might be an issue

Right, I forgot about this one.

>> @@ -76,14 +76,14 @@ static bool default_suspend_ok(struct de
>> device_for_each_child(dev, &constraint_ns,
>> dev_update_qos_constraint);
>>
>> - if (constraint_ns == 0) {
>> + if (constraint_ns == PM_QOS_RESUME_LATENCY_NO_CONSTRAINT) {
>> /* "No restriction", so the device is allowed to suspend. */
>> - td->effective_constraint_ns = 0;
>> + td->effective_constraint_ns = PM_QOS_RESUME_LATENCY_NO_CONSTRAINT;
>> td->cached_suspend_ok = true;
>> - } else if (WARN_ON(constraint_ns < 0)) {
>> + } else if (WARN_ON(constraint_ns == 0)) {
>
> This can never happen as this device will not be suspending if the child had
> failed to suspend (only case where this value is possible).
>
>> /*
>> - * effective_constraint_ns is negative already and
>> - * cached_suspend_ok is false, so return right away.
>> + * effective_constraint_ns is 0 already and cached_suspend_ok is
>> + * false, so return right away.
>> */
>> return false;
>> } else {
>> @@ -156,12 +156,11 @@ static bool __default_power_down_ok(stru
>> */
>> td = &to_gpd_data(pdd)->td;
>> constraint_ns = td->effective_constraint_ns;
>> - /* Negative values mean "no suspend at all" */
>> - if (WARN_ON(constraint_ns < 0))
>> + /* 0 mean "no suspend at all" */
>> + if (WARN_ON(constraint_ns == 0))
>> return false;
>
> If it did not suspend then it cannot powerdown. This check may not be
> very useful here.

The checks with WARN_ON() are there to catch the unexpected. They
aren't meant to trigger at all under normal conditions.

Thanks,
Rafael

Next message: Juergen Gross: "Re: [PATCH] xen/time: Return -ENODEV from xen_get_wallclock()"
Previous message: Huang, Ying: "[RFC -mm] mm, userfaultfd, THP: Avoid waiting when PMD under THP migration"
In reply to: Ramesh Thomas: "Re: [RFT][PATCH 2/2] PM / QoS: Fix device resume latency framework"
Next in thread: Rafael J. Wysocki: "Re: [RFT][PATCH 2/2] PM / QoS: Fix device resume latency framework"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]