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

[Chanwoo Choi]

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