Re: [PATCH v2 1/2] drivers: devfreq: change devfreq workqueue mechanism

[Matthias Kaehlcke <mka@chromium.org>]

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