Re: [RFT][PATCH 0/3] cpufreq / PM: QoS: Introduce frequency QoS and use it in cpufreq

[Leonard Crestez <leonard.crestez@nxp.com>]

On 24.10.2019 16:42, Rafael J. Wysocki wrote:
> On Wed, Oct 23, 2019 at 3:33 PM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>>
>> On 2019-10-23 11:54 AM, Rafael J. Wysocki wrote:
>>> On Wed, Oct 23, 2019 at 4:20 AM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>>>> On 2019-10-23 1:48 AM, Rafael J. Wysocki wrote:
> 
> [cut]
> 
>>>>> But combining the lists of requests for all the CPUs in a policy
>>>>> defeats the idea of automatic aggregation of requests which really is
>>>>> what PM QoS is about.
>>>>
>>>> My primary interest is the "dev" part of dev_pm_qos: making pm_qos
>>>> requests tied to a specific device.
>>>
>>> The list of requests needs to be associated with the user of the
>>> effective constraint.  If that is the device, it is all good.
>>
>> The phrase "user of the effective constraint" is somewhat unclear.
> 
> Fair enough, so let me elaborate.
> 
> The effective constraint (ie. the one resulting from taking all of the
> requests in the relevant QoS list into account) affects the selection
> of an OPP, so it is natural to associate the QoS list producing it
> with a list of OPPs to select.  In the cpufreq case, the policy holds
> the list of OPPs and so it also should hold the corresponding QoS
> lists (for the min and max frequency limits).  It "uses" the effective
> constraints produced by those QoS lists by preventing the OPPs out of
> the between the min and max values  from being selected.
> 
> Essentially, the policy represents a power (clock/voltage) domain with
> multiple components (it doesn't matter what they are at this level of
> abstraction). While there can be multiple sources of QoS requests
> associated with each component, all of these requests ultimately need
> to be passed to the domain for aggregation, because that's where the
> frequency selection decisions are made and so that's where the
> effective constraint value needs to be known. Now, the natural way to
> allow requests from multiple sources to be passed for aggregation is
> to provide a QoS list that they can be added to. That really is what
> PM QoS is for.
> 
>> I'm using the target device as dev for dev_pm_qos, not the requestor.
>> This is consistent with how it was used for cpufreq: thermal called a
>> dev_pm_qos_add_request on with dev = cpu_dev not a thermal sensor or
>> anything else.
> 
> Not really, but close. :-)
> 
> Without my series (that is 5.4-rc4, say), the cpu_cooling driver adds
> its constraint to the device PM QoS of cpufreq_cdev which is a special
> device created by that driver.  That would be fine, except that the
> cpufreq core doesn't use that QoS.  It uses the device PM QoS of the
> policy->cpu device instead.  That is, that's where it adds its
> notifiers (see cpufreq_policy_alloc()), that's where user space
> requests are added (see cpufreq_online()), and (most important) that's
> where the effective constraint value is read from (see
> cpufreq_set_policy()).  That turns out to be problematic (in addition
> to the cpu_cooling driver's QoS requests going nowhere), because
> confusion ensues if the current policy->cpu goes away.

That behavior in cpu_cooling seems like a bug.

>> However looking at other dev_pm_qos users there are instances of a
>> driver calling dev_pm_qos_add_request on it's own device but this is not
>> a strict requirement, correct?
> 
> No, it isn't.
> 
>>>>> There have to be two lists of requests per policy, one for the max and
>>>>> one for the min frequency >
>>>>>> If cpufreq needs a group of CPUs to run at the same frequency then it
>>>>>> should deal with this by doing dev_pm_qos_read_frequency on each CPU
>>>>>> device and picking a frequency that attempts to satisfy all constraints.
>>>>>
>>>>> No, that would be combining the requests by hand.
>>>>
>>>> It's just a loop though.
>>>
>>> Yes, it is, and needs to be run on every change of an effective
>>> constraint for any CPU even if the total effective constraint doesn't
>>> change.  And, of course, the per-policy user space limits would need
>>> to be combined with that by hand.
>>>
>>> Not particularly straightforward if you asked me.
>>
>> Well, this cpu-to-policy aggregation could also use a pm_qos_constraint
>> object instead of looping.
> 
> Yes, it could, but then somebody would need to add those
> "intermediate" requests to a proper policy-level QoS and it would need
> an extra notifier invocation to update each of them on a "component"
> QoS change.
> 
> This is an interesting idea in case we ever need to improve the
> scalability of the QoS lists, but I'd rather use the simpler approach
> for now.

The advantage I see is reducing the exposure of cpufreq internals

>>>>> Well, the cpufreq sysfs is per-policy and not per-CPU and we really
>>>>> need a per-policy min and max frequency in cpufreq, for governors etc.
>>>>
>>>> Aggregation could be performed at two levels:
>>>>
>>>> 1) Per cpu device (by dev_pm_qos)
>>>> 2) Per policy (inside cpufreq)
>>>>
>>>> The per-cpu dev_pm_qos notifier would just update a per-policy
>>>> pm_qos_constraints object. The second step could even be done strictly
>>>> inside the cpufreq core using existing pm_qos, no need to invent new
>>>> frameworks.
>>>>
>>>> Maybe dev_pm_qos is not a very good fit for cpufreq because of these
>>>> "cpu device versus cpufreq_policy" issues but it makes a ton of sense
>>>> for devfreq. Can you maybe hold PATCH 3 from this series pending further
>>>> discussion?
>>>
>>> It can be reverted at any time if need be and in 5.4 that would be dead code.
>>
>> I guess I can post v10 of my "devfreq pm qos" which starts by reverting
>> "PATCH 3" of this series?
> 
> You may do that, but I would consider adding a struct freq_constraints
> pointer directly to struct dev_pm_info and using the new frequency QoS
> helpers to manage it.
> 
> Arguably, there is no need to bundle that with the rest of device PM
> QoS and doing the above would help to avoid some code duplication too.

Adding to struct dev_pm_info would increase sizeof(struct device) while 
dev_pm_qos only allocates memory when constraints are added. My 
expectation is that very few devices would even have min_freq and 
max_freq constraints.

Maybe struct dev_pm_qos could host a "struct freq_constraints freq" 
instead of two separate "struct pm_qos_constraints min/max_frequency"? 
This way there would be two users of freq_constraints: cpufreq_policy 
(which is not a device) and dev_pm_qos.

In the future freq_constraints might be extended to implement some logic 
for conflicts between min_freq and max_freq requests.

--
Regards,
Leonard