Re: [PATCH 1/4] PM / devfreq: reuse system workqueue machanism

From: Chanwoo Choi
Date: Tue Dec 10 2019 - 04:36:03 EST


On 12/10/19 6:28 PM, Kamil Konieczny wrote:
> Hi Chanwoo,
>
> On 10.12.2019 08:53, Chanwoo Choi wrote:
>> On 12/10/19 4:28 PM, Kamil Konieczny wrote:
>>> On 10.12.2019 02:41, Chanwoo Choi wrote:
>>>> On 12/9/19 11:44 PM, Kamil Konieczny wrote:
>>>>> There is no need for creating another workqueue, it is enough
>>>>> to reuse system_freezable_power_efficient one.
>>>>>
>>>>> Signed-off-by: Kamil Konieczny <k.konieczny@xxxxxxxxxxx>
>>>>> ---
>>>>> drivers/devfreq/devfreq.c | 6 +++---
>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>>>> index 46a7ff7c2994..955949c6fc1f 100644
>>>>> --- a/drivers/devfreq/devfreq.c
>>>>> +++ b/drivers/devfreq/devfreq.c
>>>>> @@ -1532,11 +1532,11 @@ static int __init devfreq_init(void)
>>>>> return PTR_ERR(devfreq_class);
>>>>> }
>>>>>
>>>>> - devfreq_wq = create_freezable_workqueue("devfreq_wq");
>>>>> + devfreq_wq = system_freezable_power_efficient_wq;
>>>>
>>>> It affect the behaviors of whole device drivers using devfreq subsystem.
>>>> It is not good to change the workqueue type without any reasonable
>>>> data like experiment result, power-consumption result and performance
>>>> result for almost device drivers using devfreq subsystem.
>>>>
>>>> Are there any problem or any benefit to change workqueue type?
>>>
>>> The workqueue is freezable with additional capability of 'power_efficient',
>>> it is already developed by linux community so why not reuse it ?
>>
>> As you agreed below, why don't you suggest the any reasonable test result
>> with this patch? As I commented, it affects the all device drivers.
>> It is necessary to suggest the test result on multiple scenarios
>> in order to prevent the any power-consumption and performance regression.
>> It is not easy to change them without any data.
>>
>> Frankly, if you test almost scenarios and suggest the reasonable result
>> that anyone can understand, like there are never difference
>> between "create_freezable_workqueue("devfreq_wq");" and system_freezable_power_efficient_wq.
>> But you don't suggest any data.
>
> I agree about tests data needed for deciding about change. As I already wrote in other
> letter, I do not have such tests procedures, so if you have them and you may share
> them with me or Marek, I can run them and gather tests results.
>
>> - The original devfreq_wq include the only work related to devfreq.
>> - system_freezable_power_efficient_wq include the all works registered
>> from both other subsystem and device drivers in linux kernel.
>
> I do not know that good system wq, devfreq_wq have only one work item so
> imho it is not beneficial to use separate wq. Seperate wq can be good
> during debugging problems with wq.

No, devfreq_wq has not ony one work item. If one target use the multiple
devfreq device with simple_ondemand governor, devfreq_wq has the
the multiple work item. It depends on the number of devfreq device
with simple_ondemand governor.

>
>>>> Actually, it is not simple to change the like just one device driver
>>>> because devfreq subsytem is very important for both performance
>>>> and power-consumption.
>>>
>>> I agree. The name of this wq promises what you want, both freezable
>>> and power efficiency.
>>>
>>>> If you hope to change the feature related to both performance
>>>> and power-consumption, please suggest the reasonable data
>>>> with fundamental reason.
>>>>
>>>> So, I can't agree it.
>>>>
>>>>
>>>>> if (!devfreq_wq) {
>>>>> class_destroy(devfreq_class);
>>>>> - pr_err("%s: couldn't create workqueue\n", __FILE__);
>>>>> - return -ENOMEM;
>>>>> + pr_err("%s: system_freezable_power_efficient_wq isn't initialized\n", __FILE__);
>>>>> + return -EINVAL;
>>>>> }
>>>>> devfreq_class->dev_groups = devfreq_groups;
>


--
Best Regards,
Chanwoo Choi
Samsung Electronics