linux-kernel - Re: [PATCH V9] powercap/drivers/idle_injection: Add an idle injection framework

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] [day] [month] [year] [list]

Message-ID: <CAJZ5v0i6Mi0MtrcQRCJs-v1PiogKzxGZj-N_+rzkJ9+98wvuGw@mail.gmail.com>
Date:   Tue, 26 Jun 2018 11:27:39 +0200
From:   "Rafael J. Wysocki" <rafael@...nel.org>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Eduardo Valentin <edubezval@...il.com>,
        Javi Merino <javi.merino@...nel.org>,
        Leo Yan <leo.yan@...aro.org>,
        Kevin Wangtao <kevin.wangtao@...aro.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Rui Zhang <rui.zhang@...el.com>,
        Daniel Thompson <daniel.thompson@...aro.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Andrea Parri <andrea.parri@...rulasolutions.com>,
        "open list:POWER MANAGEMENT CORE" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH V9] powercap/drivers/idle_injection: Add an idle injection framework

On Tue, Jun 26, 2018 at 12:15 AM, Daniel Lezcano
<daniel.lezcano@...aro.org> wrote:
> On 25/06/2018 23:55, Rafael J. Wysocki wrote:
>> On Mon, Jun 25, 2018 at 1:52 PM, Daniel Lezcano
>> <daniel.lezcano@...aro.org> wrote:
>>> On 25/06/2018 10:23, Rafael J. Wysocki wrote:
>>>
>>> [ ... ]
>>>
>>>>> + * It is up to the user of this framework to provide a lock at an
>>>>> + * upper level to prevent stupid things to happen, like starting while
>>>>> + * we are unregistering.
>>>>> + */
>>>>
>>>> The English above and elsewhere needs some polishing IMO, but I can
>>>> take care of that. :-)
>>>
>>> I can give a try and if you are still unhappy, you change them in a
>>> better way.
>>
>> Well, I could tell you what I would write, but then it'll take less
>> time and effort if I just write it myself. :-)
>
> Ok.
>
>>> [ ... ]
>>>
>>>>> +static void idle_injection_setup(unsigned int cpu)
>>>>> +{
>>>>> +       struct sched_param param = { .sched_priority = MAX_USER_RT_PRIO / 2 };
>>>>> +
>>>>> +       set_freezable();
>>>>
>>>> Why do you need set_freezable() here?
>>>
>>> We don't want to continue injecting idle cycles when the system is
>>> suspended.
>>
>> And where's the corresponding try_to_freeze() called?
>
> Yes, it is missing. I suppose try_to_freeze() should be put at the
> beginning of the idle_inject_fn() function, right ?

Well, given that people want to get rid of kthread freezing entirely,
adding it may not be a good idea at all.

Also I'm not sure if we should avoid injecting idle on system suspend.
It may slow down the suspend process, but that's OK and after that it
shouldn't matter.  And if it turns out to matter, we'll address the
problem instead of trying to avoid it by freezing the kthreads.

So, I would just drop the set_freezable() and leave it like that.