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

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

On 25.10.2019 00:11, Rafael J. Wysocki wrote:
> On Thu, Oct 24, 2019 at 7:47 PM Leonard Crestez <leonard.crestez@nxp.com> wrote:
>>
>> 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.
> 
> Well, kind of. :-)
> 
>>>> 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
> 
> That can be achieved by providing a helper to add a frequency QoS
> request to the min or max QoS list of the policy covering a given CPU.
> The caller of it would just need to pass the CPU number, a pointer to
> the request struct and the type.
> 
> It wasn't necessary to add it at this time, though, and there would be
> the extra complication that the caller would need to know whether or
> not the policy had been created already.

Using dev_pm_qos already provides that, and since the request is tied to 
the struct device of the CPU there is no need to know anything about 
cpufreq_policy at all.

It would just need an additional layer of aggregation from CPU to 
cpufreq_policy.

>>>>>>> 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.
> 
> Well, fair enough.
> 
>> Maybe struct dev_pm_qos could host a "struct freq_constraints freq"
>> instead of two separate "struct pm_qos_constraints min/max_frequency"?
> 
> That is possible too.
> 
>> 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.
> 
> Sure.

Posted for review: https://patchwork.kernel.org/cover/11212887/