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

[Rafael J. Wysocki]

On Tue, Jun 26, 2018 at 12:15 AM, Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> wrote:
> On 25/06/2018 23:55, Rafael J. Wysocki wrote:
>> On Mon, Jun 25, 2018 at 1:52 PM, Daniel Lezcano
>> <daniel.lezcano@xxxxxxxxxx> 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.