| Message ID | 1549899005-7760-2-git-send-email-l.luba@partner.samsung.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | drivers: devfreq: fix and optimize workqueue mechanism | expand |
Hi Lukasz, On Mon, Feb 11, 2019 at 04:30:04PM +0100, Lukasz Luba wrote: > There is no need for creating another workqueue in the system, > the existing one should meet the requirements. > This patch removes devfreq's custom workqueue and uses system one. > It switches from queue_delayed_work() to schedule_delayed_work(). > It also does not wake up the system when it enters suspend (this > functionality stays the same). > > Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> > --- > drivers/devfreq/devfreq.c | 25 ++++++------------------- > 1 file changed, 6 insertions(+), 19 deletions(-) > > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > index 0ae3de7..882e717 100644 > --- a/drivers/devfreq/devfreq.c > +++ b/drivers/devfreq/devfreq.c > @@ -31,13 +31,6 @@ > > static struct class *devfreq_class; > > -/* > - * devfreq core provides delayed work based load monitoring helper > - * functions. Governors can use these or can implement their own > - * monitoring mechanism. > - */ > -static struct workqueue_struct *devfreq_wq; > - > /* The list of all device-devfreq governors */ > static LIST_HEAD(devfreq_governor_list); > /* The list of all device-devfreq */ > @@ -391,8 +384,8 @@ static void devfreq_monitor(struct work_struct *work) > if (err) > dev_err(&devfreq->dev, "dvfs failed with (%d) error\n", err); > > - queue_delayed_work(devfreq_wq, &devfreq->work, > - msecs_to_jiffies(devfreq->profile->polling_ms)); > + schedule_delayed_work(&devfreq->work, > + msecs_to_jiffies(devfreq->profile->polling_ms)); > mutex_unlock(&devfreq->lock); > } > > @@ -409,7 +402,7 @@ void devfreq_monitor_start(struct devfreq *devfreq) > { > INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor); > if (devfreq->profile->polling_ms) > - queue_delayed_work(devfreq_wq, &devfreq->work, > + schedule_delayed_work(&devfreq->work, > msecs_to_jiffies(devfreq->profile->polling_ms)); > } > EXPORT_SYMBOL(devfreq_monitor_start); > @@ -473,7 +466,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq) > > if (!delayed_work_pending(&devfreq->work) && > devfreq->profile->polling_ms) > - queue_delayed_work(devfreq_wq, &devfreq->work, > + schedule_delayed_work(&devfreq->work, > msecs_to_jiffies(devfreq->profile->polling_ms)); > > devfreq->last_stat_updated = jiffies; > @@ -516,7 +509,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) > > /* if current delay is zero, start polling with new delay */ > if (!cur_delay) { > - queue_delayed_work(devfreq_wq, &devfreq->work, > + schedule_delayed_work(&devfreq->work, > msecs_to_jiffies(devfreq->profile->polling_ms)); > goto out; > } > @@ -527,7 +520,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) > cancel_delayed_work_sync(&devfreq->work); > mutex_lock(&devfreq->lock); > if (!devfreq->stop_polling) > - queue_delayed_work(devfreq_wq, &devfreq->work, > + schedule_delayed_work(&devfreq->work, > msecs_to_jiffies(devfreq->profile->polling_ms)); > } > out: > @@ -1430,12 +1423,6 @@ static int __init devfreq_init(void) > return PTR_ERR(devfreq_class); > } > > - devfreq_wq = create_freezable_workqueue("devfreq_wq"); > - if (!devfreq_wq) { > - class_destroy(devfreq_class); > - pr_err("%s: couldn't create workqueue\n", __FILE__); > - return -ENOMEM; > - } > devfreq_class->dev_groups = devfreq_groups; > > return 0; As commented on v1, the change from a custom to a system workqueue seems reasonable to me. However this patch also changes from a freezable workqueue to a non-freezable one. C&P of my comments on v1: ``WQ_FREEZABLE`` A freezable wq participates in the freeze phase of the system suspend operations. Work items on the wq are drained and no new work item starts execution until thawed. I'm not entirely sure what the impact of this is. I imagine suspend is potentially quicker because the wq isn't drained, but could works that execute during the suspend phase be a problem? Cheers Matthias
Hi Matthias, On 2/11/19 10:42 PM, Matthias Kaehlcke wrote: > Hi Lukasz, > > On Mon, Feb 11, 2019 at 04:30:04PM +0100, Lukasz Luba wrote: >> There is no need for creating another workqueue in the system, >> the existing one should meet the requirements. >> This patch removes devfreq's custom workqueue and uses system one. >> It switches from queue_delayed_work() to schedule_delayed_work(). >> It also does not wake up the system when it enters suspend (this >> functionality stays the same). >> >> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> >> --- >> drivers/devfreq/devfreq.c | 25 ++++++------------------- >> 1 file changed, 6 insertions(+), 19 deletions(-) >> >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >> index 0ae3de7..882e717 100644 >> --- a/drivers/devfreq/devfreq.c >> +++ b/drivers/devfreq/devfreq.c >> @@ -31,13 +31,6 @@ >> >> static struct class *devfreq_class; >> >> -/* >> - * devfreq core provides delayed work based load monitoring helper >> - * functions. Governors can use these or can implement their own >> - * monitoring mechanism. >> - */ >> -static struct workqueue_struct *devfreq_wq; >> - >> /* The list of all device-devfreq governors */ >> static LIST_HEAD(devfreq_governor_list); >> /* The list of all device-devfreq */ >> @@ -391,8 +384,8 @@ static void devfreq_monitor(struct work_struct *work) >> if (err) >> dev_err(&devfreq->dev, "dvfs failed with (%d) error\n", err); >> >> - queue_delayed_work(devfreq_wq, &devfreq->work, >> - msecs_to_jiffies(devfreq->profile->polling_ms)); >> + schedule_delayed_work(&devfreq->work, >> + msecs_to_jiffies(devfreq->profile->polling_ms)); >> mutex_unlock(&devfreq->lock); >> } >> >> @@ -409,7 +402,7 @@ void devfreq_monitor_start(struct devfreq *devfreq) >> { >> INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor); >> if (devfreq->profile->polling_ms) >> - queue_delayed_work(devfreq_wq, &devfreq->work, >> + schedule_delayed_work(&devfreq->work, >> msecs_to_jiffies(devfreq->profile->polling_ms)); >> } >> EXPORT_SYMBOL(devfreq_monitor_start); >> @@ -473,7 +466,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq) >> >> if (!delayed_work_pending(&devfreq->work) && >> devfreq->profile->polling_ms) >> - queue_delayed_work(devfreq_wq, &devfreq->work, >> + schedule_delayed_work(&devfreq->work, >> msecs_to_jiffies(devfreq->profile->polling_ms)); >> >> devfreq->last_stat_updated = jiffies; >> @@ -516,7 +509,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) >> >> /* if current delay is zero, start polling with new delay */ >> if (!cur_delay) { >> - queue_delayed_work(devfreq_wq, &devfreq->work, >> + schedule_delayed_work(&devfreq->work, >> msecs_to_jiffies(devfreq->profile->polling_ms)); >> goto out; >> } >> @@ -527,7 +520,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) >> cancel_delayed_work_sync(&devfreq->work); >> mutex_lock(&devfreq->lock); >> if (!devfreq->stop_polling) >> - queue_delayed_work(devfreq_wq, &devfreq->work, >> + schedule_delayed_work(&devfreq->work, >> msecs_to_jiffies(devfreq->profile->polling_ms)); >> } >> out: >> @@ -1430,12 +1423,6 @@ static int __init devfreq_init(void) >> return PTR_ERR(devfreq_class); >> } >> >> - devfreq_wq = create_freezable_workqueue("devfreq_wq"); >> - if (!devfreq_wq) { >> - class_destroy(devfreq_class); >> - pr_err("%s: couldn't create workqueue\n", __FILE__); >> - return -ENOMEM; >> - } >> devfreq_class->dev_groups = devfreq_groups; >> >> return 0; > > As commented on v1, the change from a custom to a system workqueue > seems reasonable to me. However this patch also changes from a > freezable workqueue to a non-freezable one. C&P of my comments on v1: > > ``WQ_FREEZABLE`` > A freezable wq participates in the freeze phase of the system > suspend operations. Work items on the wq are drained and no > new work item starts execution until thawed. > > I'm not entirely sure what the impact of this is. > > I imagine suspend is potentially quicker because the wq isn't drained, > but could works that execute during the suspend phase be a problem? The devfreq supports suspend from v4.20-rc6, which picks OPP for a device based on its DT 'opp-suspend'. For the devices which do not choose the suspend OPP it is possible to enter that state with any frequency. Queuing work for calling governor during suspend which calculates the device's frequency for the next period is IMO not needed, The 'next period' is actually suspend and is not related to 'predicted' load by the governor. You are right, the suspend will be faster. Regards, Lukasz > > Cheers > > Matthias > >
On Tue, Feb 12, 2019 at 12:20:42PM +0100, Lukasz Luba wrote: > Hi Matthias, > > On 2/11/19 10:42 PM, Matthias Kaehlcke wrote: > > Hi Lukasz, > > > > On Mon, Feb 11, 2019 at 04:30:04PM +0100, Lukasz Luba wrote: > >> There is no need for creating another workqueue in the system, > >> the existing one should meet the requirements. > >> This patch removes devfreq's custom workqueue and uses system one. > >> It switches from queue_delayed_work() to schedule_delayed_work(). > >> It also does not wake up the system when it enters suspend (this > >> functionality stays the same). > >> > >> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> > >> --- > >> drivers/devfreq/devfreq.c | 25 ++++++------------------- > >> 1 file changed, 6 insertions(+), 19 deletions(-) > >> > >> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > >> index 0ae3de7..882e717 100644 > >> --- a/drivers/devfreq/devfreq.c > >> +++ b/drivers/devfreq/devfreq.c > >> @@ -31,13 +31,6 @@ > >> > >> static struct class *devfreq_class; > >> > >> -/* > >> - * devfreq core provides delayed work based load monitoring helper > >> - * functions. Governors can use these or can implement their own > >> - * monitoring mechanism. > >> - */ > >> -static struct workqueue_struct *devfreq_wq; > >> - > >> /* The list of all device-devfreq governors */ > >> static LIST_HEAD(devfreq_governor_list); > >> /* The list of all device-devfreq */ > >> @@ -391,8 +384,8 @@ static void devfreq_monitor(struct work_struct *work) > >> if (err) > >> dev_err(&devfreq->dev, "dvfs failed with (%d) error\n", err); > >> > >> - queue_delayed_work(devfreq_wq, &devfreq->work, > >> - msecs_to_jiffies(devfreq->profile->polling_ms)); > >> + schedule_delayed_work(&devfreq->work, > >> + msecs_to_jiffies(devfreq->profile->polling_ms)); > >> mutex_unlock(&devfreq->lock); > >> } > >> > >> @@ -409,7 +402,7 @@ void devfreq_monitor_start(struct devfreq *devfreq) > >> { > >> INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor); > >> if (devfreq->profile->polling_ms) > >> - queue_delayed_work(devfreq_wq, &devfreq->work, > >> + schedule_delayed_work(&devfreq->work, > >> msecs_to_jiffies(devfreq->profile->polling_ms)); > >> } > >> EXPORT_SYMBOL(devfreq_monitor_start); > >> @@ -473,7 +466,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq) > >> > >> if (!delayed_work_pending(&devfreq->work) && > >> devfreq->profile->polling_ms) > >> - queue_delayed_work(devfreq_wq, &devfreq->work, > >> + schedule_delayed_work(&devfreq->work, > >> msecs_to_jiffies(devfreq->profile->polling_ms)); > >> > >> devfreq->last_stat_updated = jiffies; > >> @@ -516,7 +509,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) > >> > >> /* if current delay is zero, start polling with new delay */ > >> if (!cur_delay) { > >> - queue_delayed_work(devfreq_wq, &devfreq->work, > >> + schedule_delayed_work(&devfreq->work, > >> msecs_to_jiffies(devfreq->profile->polling_ms)); > >> goto out; > >> } > >> @@ -527,7 +520,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) > >> cancel_delayed_work_sync(&devfreq->work); > >> mutex_lock(&devfreq->lock); > >> if (!devfreq->stop_polling) > >> - queue_delayed_work(devfreq_wq, &devfreq->work, > >> + schedule_delayed_work(&devfreq->work, > >> msecs_to_jiffies(devfreq->profile->polling_ms)); > >> } > >> out: > >> @@ -1430,12 +1423,6 @@ static int __init devfreq_init(void) > >> return PTR_ERR(devfreq_class); > >> } > >> > >> - devfreq_wq = create_freezable_workqueue("devfreq_wq"); > >> - if (!devfreq_wq) { > >> - class_destroy(devfreq_class); > >> - pr_err("%s: couldn't create workqueue\n", __FILE__); > >> - return -ENOMEM; > >> - } > >> devfreq_class->dev_groups = devfreq_groups; > >> > >> return 0; > > > > As commented on v1, the change from a custom to a system workqueue > > seems reasonable to me. However this patch also changes from a > > freezable workqueue to a non-freezable one. C&P of my comments on v1: > > > > ``WQ_FREEZABLE`` > > A freezable wq participates in the freeze phase of the system > > suspend operations. Work items on the wq are drained and no > > new work item starts execution until thawed. > > > > I'm not entirely sure what the impact of this is. > > > > I imagine suspend is potentially quicker because the wq isn't drained, > > but could works that execute during the suspend phase be a problem? > The devfreq supports suspend from v4.20-rc6, which picks OPP for a > device based on its DT 'opp-suspend'. For the devices which do not > choose the suspend OPP it is possible to enter that state with any > frequency. Queuing work for calling governor during suspend which > calculates the device's frequency for the next period is IMO not needed, > The 'next period' is actually suspend and is not related to > 'predicted' load by the governor. If I am not mistaken the monitor can still be running after a device was suspended: devfreq_suspend list_for_each_entry(devfreq, &devfreq_list, node) devfreq_suspend_device devfreq->governor->event_handler(devfreq, DEVFREQ_GOV_SUSPEND, NULL); According to the comment of devfreq_monitor_suspend() the function is supposed to be called by the governor in response to DEVFREQ_GOV_SUSPEND, however this doesn't seem to be universally the case: git grep devfreq_monitor_suspend drivers/devfreq/governor_simpleondemand.c: devfreq_monitor_suspend(devfreq); drivers/devfreq/tegra-devfreq.c: devfreq_monitor_suspend(devfreq); i.e. the other governors don't seem to call devfreq_monitor_suspend(). Am I missing something? Thanks Matthias
Hi Matthias, On 2/12/19 9:12 PM, Matthias Kaehlcke wrote: > On Tue, Feb 12, 2019 at 12:20:42PM +0100, Lukasz Luba wrote: >> Hi Matthias, >> >> On 2/11/19 10:42 PM, Matthias Kaehlcke wrote: >>> Hi Lukasz, >>> >>> On Mon, Feb 11, 2019 at 04:30:04PM +0100, Lukasz Luba wrote: >>>> There is no need for creating another workqueue in the system, >>>> the existing one should meet the requirements. >>>> This patch removes devfreq's custom workqueue and uses system one. >>>> It switches from queue_delayed_work() to schedule_delayed_work(). >>>> It also does not wake up the system when it enters suspend (this >>>> functionality stays the same). >>>> >>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> >>>> --- >>>> drivers/devfreq/devfreq.c | 25 ++++++------------------- >>>> 1 file changed, 6 insertions(+), 19 deletions(-) >>>> >>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c >>>> index 0ae3de7..882e717 100644 >>>> --- a/drivers/devfreq/devfreq.c >>>> +++ b/drivers/devfreq/devfreq.c >>>> @@ -31,13 +31,6 @@ >>>> >>>> static struct class *devfreq_class; >>>> >>>> -/* >>>> - * devfreq core provides delayed work based load monitoring helper >>>> - * functions. Governors can use these or can implement their own >>>> - * monitoring mechanism. >>>> - */ >>>> -static struct workqueue_struct *devfreq_wq; >>>> - >>>> /* The list of all device-devfreq governors */ >>>> static LIST_HEAD(devfreq_governor_list); >>>> /* The list of all device-devfreq */ >>>> @@ -391,8 +384,8 @@ static void devfreq_monitor(struct work_struct *work) >>>> if (err) >>>> dev_err(&devfreq->dev, "dvfs failed with (%d) error\n", err); >>>> >>>> - queue_delayed_work(devfreq_wq, &devfreq->work, >>>> - msecs_to_jiffies(devfreq->profile->polling_ms)); >>>> + schedule_delayed_work(&devfreq->work, >>>> + msecs_to_jiffies(devfreq->profile->polling_ms)); >>>> mutex_unlock(&devfreq->lock); >>>> } >>>> >>>> @@ -409,7 +402,7 @@ void devfreq_monitor_start(struct devfreq *devfreq) >>>> { >>>> INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor); >>>> if (devfreq->profile->polling_ms) >>>> - queue_delayed_work(devfreq_wq, &devfreq->work, >>>> + schedule_delayed_work(&devfreq->work, >>>> msecs_to_jiffies(devfreq->profile->polling_ms)); >>>> } >>>> EXPORT_SYMBOL(devfreq_monitor_start); >>>> @@ -473,7 +466,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq) >>>> >>>> if (!delayed_work_pending(&devfreq->work) && >>>> devfreq->profile->polling_ms) >>>> - queue_delayed_work(devfreq_wq, &devfreq->work, >>>> + schedule_delayed_work(&devfreq->work, >>>> msecs_to_jiffies(devfreq->profile->polling_ms)); >>>> >>>> devfreq->last_stat_updated = jiffies; >>>> @@ -516,7 +509,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) >>>> >>>> /* if current delay is zero, start polling with new delay */ >>>> if (!cur_delay) { >>>> - queue_delayed_work(devfreq_wq, &devfreq->work, >>>> + schedule_delayed_work(&devfreq->work, >>>> msecs_to_jiffies(devfreq->profile->polling_ms)); >>>> goto out; >>>> } >>>> @@ -527,7 +520,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) >>>> cancel_delayed_work_sync(&devfreq->work); >>>> mutex_lock(&devfreq->lock); >>>> if (!devfreq->stop_polling) >>>> - queue_delayed_work(devfreq_wq, &devfreq->work, >>>> + schedule_delayed_work(&devfreq->work, >>>> msecs_to_jiffies(devfreq->profile->polling_ms)); >>>> } >>>> out: >>>> @@ -1430,12 +1423,6 @@ static int __init devfreq_init(void) >>>> return PTR_ERR(devfreq_class); >>>> } >>>> >>>> - devfreq_wq = create_freezable_workqueue("devfreq_wq"); >>>> - if (!devfreq_wq) { >>>> - class_destroy(devfreq_class); >>>> - pr_err("%s: couldn't create workqueue\n", __FILE__); >>>> - return -ENOMEM; >>>> - } >>>> devfreq_class->dev_groups = devfreq_groups; >>>> >>>> return 0; >>> >>> As commented on v1, the change from a custom to a system workqueue >>> seems reasonable to me. However this patch also changes from a >>> freezable workqueue to a non-freezable one. C&P of my comments on v1: >>> >>> ``WQ_FREEZABLE`` >>> A freezable wq participates in the freeze phase of the system >>> suspend operations. Work items on the wq are drained and no >>> new work item starts execution until thawed. >>> >>> I'm not entirely sure what the impact of this is. >>> >>> I imagine suspend is potentially quicker because the wq isn't drained, >>> but could works that execute during the suspend phase be a problem? >> The devfreq supports suspend from v4.20-rc6, which picks OPP for a >> device based on its DT 'opp-suspend'. For the devices which do not >> choose the suspend OPP it is possible to enter that state with any >> frequency. Queuing work for calling governor during suspend which >> calculates the device's frequency for the next period is IMO not needed, >> The 'next period' is actually suspend and is not related to >> 'predicted' load by the governor. > > If I am not mistaken the monitor can still be running after a device > was suspended: > > devfreq_suspend > list_for_each_entry(devfreq, &devfreq_list, node) > devfreq_suspend_device > devfreq->governor->event_handler(devfreq, > DEVFREQ_GOV_SUSPEND, NULL); > > According to the comment of devfreq_monitor_suspend() the function is > supposed to be called by the governor in response to > DEVFREQ_GOV_SUSPEND, however this doesn't seem to be universally the case: > > git grep devfreq_monitor_suspend > drivers/devfreq/governor_simpleondemand.c: devfreq_monitor_suspend(devfreq); > drivers/devfreq/tegra-devfreq.c: devfreq_monitor_suspend(devfreq); > > i.e. the other governors don't seem to call devfreq_monitor_suspend(). > > Am I missing something? Probably not. Good catch, these governors should support case DEVFREQ_GOV_SUSPEND. The system suspend which calls 'devfreq_suspend' does it when the workqueues are frozen and sets the desired OPP for later resume. The other use use cases (like pm_suspend) might assume that these governors are ready for DEVFREQ_GOV_SUSPEND... Do you like to write a patch for them (I can test it) or should I do it? Regards, Lukasz > > Thanks > > Matthias > >
Hi Lukasz, On Tue, Feb 12, 2019 at 10:37:20PM +0100, Lukasz Luba wrote: > Hi Matthias, > > On 2/12/19 9:12 PM, Matthias Kaehlcke wrote: > > On Tue, Feb 12, 2019 at 12:20:42PM +0100, Lukasz Luba wrote: > >> Hi Matthias, > >> > >> On 2/11/19 10:42 PM, Matthias Kaehlcke wrote: > >>> Hi Lukasz, > >>> > >>> On Mon, Feb 11, 2019 at 04:30:04PM +0100, Lukasz Luba wrote: > >>>> There is no need for creating another workqueue in the system, > >>>> the existing one should meet the requirements. > >>>> This patch removes devfreq's custom workqueue and uses system one. > >>>> It switches from queue_delayed_work() to schedule_delayed_work(). > >>>> It also does not wake up the system when it enters suspend (this > >>>> functionality stays the same). > >>>> > >>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> > >>>> --- > >>>> drivers/devfreq/devfreq.c | 25 ++++++------------------- > >>>> 1 file changed, 6 insertions(+), 19 deletions(-) > >>>> > >>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c > >>>> index 0ae3de7..882e717 100644 > >>>> --- a/drivers/devfreq/devfreq.c > >>>> +++ b/drivers/devfreq/devfreq.c > >>>> @@ -31,13 +31,6 @@ > >>>> > >>>> static struct class *devfreq_class; > >>>> > >>>> -/* > >>>> - * devfreq core provides delayed work based load monitoring helper > >>>> - * functions. Governors can use these or can implement their own > >>>> - * monitoring mechanism. > >>>> - */ > >>>> -static struct workqueue_struct *devfreq_wq; > >>>> - > >>>> /* The list of all device-devfreq governors */ > >>>> static LIST_HEAD(devfreq_governor_list); > >>>> /* The list of all device-devfreq */ > >>>> @@ -391,8 +384,8 @@ static void devfreq_monitor(struct work_struct *work) > >>>> if (err) > >>>> dev_err(&devfreq->dev, "dvfs failed with (%d) error\n", err); > >>>> > >>>> - queue_delayed_work(devfreq_wq, &devfreq->work, > >>>> - msecs_to_jiffies(devfreq->profile->polling_ms)); > >>>> + schedule_delayed_work(&devfreq->work, > >>>> + msecs_to_jiffies(devfreq->profile->polling_ms)); > >>>> mutex_unlock(&devfreq->lock); > >>>> } > >>>> > >>>> @@ -409,7 +402,7 @@ void devfreq_monitor_start(struct devfreq *devfreq) > >>>> { > >>>> INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor); > >>>> if (devfreq->profile->polling_ms) > >>>> - queue_delayed_work(devfreq_wq, &devfreq->work, > >>>> + schedule_delayed_work(&devfreq->work, > >>>> msecs_to_jiffies(devfreq->profile->polling_ms)); > >>>> } > >>>> EXPORT_SYMBOL(devfreq_monitor_start); > >>>> @@ -473,7 +466,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq) > >>>> > >>>> if (!delayed_work_pending(&devfreq->work) && > >>>> devfreq->profile->polling_ms) > >>>> - queue_delayed_work(devfreq_wq, &devfreq->work, > >>>> + schedule_delayed_work(&devfreq->work, > >>>> msecs_to_jiffies(devfreq->profile->polling_ms)); > >>>> > >>>> devfreq->last_stat_updated = jiffies; > >>>> @@ -516,7 +509,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) > >>>> > >>>> /* if current delay is zero, start polling with new delay */ > >>>> if (!cur_delay) { > >>>> - queue_delayed_work(devfreq_wq, &devfreq->work, > >>>> + schedule_delayed_work(&devfreq->work, > >>>> msecs_to_jiffies(devfreq->profile->polling_ms)); > >>>> goto out; > >>>> } > >>>> @@ -527,7 +520,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) > >>>> cancel_delayed_work_sync(&devfreq->work); > >>>> mutex_lock(&devfreq->lock); > >>>> if (!devfreq->stop_polling) > >>>> - queue_delayed_work(devfreq_wq, &devfreq->work, > >>>> + schedule_delayed_work(&devfreq->work, > >>>> msecs_to_jiffies(devfreq->profile->polling_ms)); > >>>> } > >>>> out: > >>>> @@ -1430,12 +1423,6 @@ static int __init devfreq_init(void) > >>>> return PTR_ERR(devfreq_class); > >>>> } > >>>> > >>>> - devfreq_wq = create_freezable_workqueue("devfreq_wq"); > >>>> - if (!devfreq_wq) { > >>>> - class_destroy(devfreq_class); > >>>> - pr_err("%s: couldn't create workqueue\n", __FILE__); > >>>> - return -ENOMEM; > >>>> - } > >>>> devfreq_class->dev_groups = devfreq_groups; > >>>> > >>>> return 0; > >>> > >>> As commented on v1, the change from a custom to a system workqueue > >>> seems reasonable to me. However this patch also changes from a > >>> freezable workqueue to a non-freezable one. C&P of my comments on v1: > >>> > >>> ``WQ_FREEZABLE`` > >>> A freezable wq participates in the freeze phase of the system > >>> suspend operations. Work items on the wq are drained and no > >>> new work item starts execution until thawed. > >>> > >>> I'm not entirely sure what the impact of this is. > >>> > >>> I imagine suspend is potentially quicker because the wq isn't drained, > >>> but could works that execute during the suspend phase be a problem? > >> The devfreq supports suspend from v4.20-rc6, which picks OPP for a > >> device based on its DT 'opp-suspend'. For the devices which do not > >> choose the suspend OPP it is possible to enter that state with any > >> frequency. Queuing work for calling governor during suspend which > >> calculates the device's frequency for the next period is IMO not needed, > >> The 'next period' is actually suspend and is not related to > >> 'predicted' load by the governor. > > > > If I am not mistaken the monitor can still be running after a device > > was suspended: > > > > devfreq_suspend > > list_for_each_entry(devfreq, &devfreq_list, node) > > devfreq_suspend_device > > devfreq->governor->event_handler(devfreq, > > DEVFREQ_GOV_SUSPEND, NULL); > > > > According to the comment of devfreq_monitor_suspend() the function is > > supposed to be called by the governor in response to > > DEVFREQ_GOV_SUSPEND, however this doesn't seem to be universally the case: > > > > git grep devfreq_monitor_suspend > > drivers/devfreq/governor_simpleondemand.c: devfreq_monitor_suspend(devfreq); > > drivers/devfreq/tegra-devfreq.c: devfreq_monitor_suspend(devfreq); > > > > i.e. the other governors don't seem to call devfreq_monitor_suspend(). > > > > Am I missing something? > Probably not. > Good catch, these governors should support case DEVFREQ_GOV_SUSPEND. > The system suspend which calls 'devfreq_suspend' does it when the > workqueues are frozen and sets the desired OPP for later resume. > The other use use cases (like pm_suspend) might assume that these > governors are ready for DEVFREQ_GOV_SUSPEND... Thanks for the confirmation! > Do you like to write a patch for them (I can test it) or should I do it? I can send a patch, testing will be appreciated :) Thanks Matthias
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 0ae3de7..882e717 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -31,13 +31,6 @@ static struct class *devfreq_class; -/* - * devfreq core provides delayed work based load monitoring helper - * functions. Governors can use these or can implement their own - * monitoring mechanism. - */ -static struct workqueue_struct *devfreq_wq; - /* The list of all device-devfreq governors */ static LIST_HEAD(devfreq_governor_list); /* The list of all device-devfreq */ @@ -391,8 +384,8 @@ static void devfreq_monitor(struct work_struct *work) if (err) dev_err(&devfreq->dev, "dvfs failed with (%d) error\n", err); - queue_delayed_work(devfreq_wq, &devfreq->work, - msecs_to_jiffies(devfreq->profile->polling_ms)); + schedule_delayed_work(&devfreq->work, + msecs_to_jiffies(devfreq->profile->polling_ms)); mutex_unlock(&devfreq->lock); } @@ -409,7 +402,7 @@ void devfreq_monitor_start(struct devfreq *devfreq) { INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor); if (devfreq->profile->polling_ms) - queue_delayed_work(devfreq_wq, &devfreq->work, + schedule_delayed_work(&devfreq->work, msecs_to_jiffies(devfreq->profile->polling_ms)); } EXPORT_SYMBOL(devfreq_monitor_start); @@ -473,7 +466,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq) if (!delayed_work_pending(&devfreq->work) && devfreq->profile->polling_ms) - queue_delayed_work(devfreq_wq, &devfreq->work, + schedule_delayed_work(&devfreq->work, msecs_to_jiffies(devfreq->profile->polling_ms)); devfreq->last_stat_updated = jiffies; @@ -516,7 +509,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) /* if current delay is zero, start polling with new delay */ if (!cur_delay) { - queue_delayed_work(devfreq_wq, &devfreq->work, + schedule_delayed_work(&devfreq->work, msecs_to_jiffies(devfreq->profile->polling_ms)); goto out; } @@ -527,7 +520,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay) cancel_delayed_work_sync(&devfreq->work); mutex_lock(&devfreq->lock); if (!devfreq->stop_polling) - queue_delayed_work(devfreq_wq, &devfreq->work, + schedule_delayed_work(&devfreq->work, msecs_to_jiffies(devfreq->profile->polling_ms)); } out: @@ -1430,12 +1423,6 @@ static int __init devfreq_init(void) return PTR_ERR(devfreq_class); } - devfreq_wq = create_freezable_workqueue("devfreq_wq"); - if (!devfreq_wq) { - class_destroy(devfreq_class); - pr_err("%s: couldn't create workqueue\n", __FILE__); - return -ENOMEM; - } devfreq_class->dev_groups = devfreq_groups; return 0;
There is no need for creating another workqueue in the system, the existing one should meet the requirements. This patch removes devfreq's custom workqueue and uses system one. It switches from queue_delayed_work() to schedule_delayed_work(). It also does not wake up the system when it enters suspend (this functionality stays the same). Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com> --- drivers/devfreq/devfreq.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-)