linux-kernel - Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 6 Nov 2017 13:49:57 +0100
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Ulf Hansson <ulf.hansson@...aro.org>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Linux PM <linux-pm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Tero Kristo <t-kristo@...com>,
        Reinette Chatre <reinette.chatre@...el.com>,
        Ramesh Thomas <ramesh.thomas@...el.com>,
        Alex Shi <alex.shi@...aro.org>
Subject: Re: [RFT][PATCH v2 1/2] PM / domains: Rework governor code to be more consistent

On Mon, Nov 6, 2017 at 1:44 PM, Ulf Hansson <ulf.hansson@...aro.org> wrote:
> [...]
>
>>>>  static int dev_update_qos_constraint(struct device *dev, void *data)
>>>>  {
>>>>         s64 *constraint_ns_p = data;
>>>> -       s32 constraint_ns = -1;
>>>> -
>>>> -       if (dev->power.subsys_data && dev->power.subsys_data->domain_data)
>>>> -               constraint_ns = dev_gpd_data(dev)->td.effective_constraint_ns;
>>>> +       s64 constraint_ns;
>>>>
>>>> -       if (constraint_ns < 0) {
>>>> -               constraint_ns = dev_pm_qos_read_value(dev);
>>>> -               constraint_ns *= NSEC_PER_USEC;
>>>> -       }
>>>> -       if (constraint_ns == 0)
>>>> +       if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data)
>>>>                 return 0;
>>>>
>>>>         /*
>>>> -        * constraint_ns cannot be negative here, because the device has been
>>>> -        * suspended.
>>>> +        * Only take suspend-time QoS constraints of devices into account,
>>>> +        * because constraints updated after the device has been suspended are
>>>> +        * not guaranteed to be taken into account anyway.  In order for them
>>>> +        * to take effect, the device has to be resumed and suspended again.
>>>>          */
>>>
>>> This means a change in behavior, because earlier we took into account
>>> QoS values for child devices that were not attached to a genpd.
>>
>> OK
>>
>> I have overlooked it or rather have forgotten about that.
>>
>>> Is there any reason you think we should change this, or is it just
>>> something you overlooked here?
>>>
>>> I understand, that if the QoS constraint has been updated after such
>>> child device has been suspended, it's tricky to take a correct
>>> decision.
>>
>> Right, but if they are not in a domain, the best we can do is to look
>> at the current value.
>>
>>> To really solve this, we would either have to register a QoS notifier
>>> for all children devices that has its parent attached to a genpd, or
>>> always runtime resume devices before updating QoS constraints.
>>>
>>> Non of these options is perfect, so perhaps we should consider a "best
>>> effort" approach instead? Whatever that may be.
>>
>> I think best effort makes most sense.
>
> Okay!
>
>>
>> So I guess I'll simply evaluate dev_pm_qos_read_value(dev) if
>> subsys_data or subsys_data->domain_data is not there.
>
> Yes.
>
> However, if it returns -1, what value should you pick? 0?

Without the second patch, -1 will just mean "no suspend", so the
parent cannot be suspended too, but that should just work AFAICS
(effective_constraint_ns may be -1 too at that point, if present).

With the second patch it cannot be -1 any more. :-)

Thanks,
Rafael