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

[Kamil Konieczny <k.konieczny@samsung.com>]

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@samsung.com>
>>>> ---
>>>>  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.

>>> 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,
Kamil Konieczny
Samsung R&D Institute Poland