| Message ID | 20191209144425.13321-2-k.konieczny@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PM / devfreq: add possibility for delayed work | expand |
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? 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. 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; > >
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 ? > 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; >> >> > >
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. - 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. > >> 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; >>> >>> >> >> >
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;
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@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. 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; >
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; 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;
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(-)