From: Matthias Kaehlcke <mka@chromium.org>
To: Lukasz Luba <l.luba@partner.samsung.com>
Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
b.zolnierkie@samsung.com, myungjoo.ham@samsung.com,
cw00.choi@samsung.com, kyungmin.park@samsung.com,
m.szyprowski@samsung.com, s.nawrocki@samsung.com,
tkjos@google.com, joel@joelfernandes.org, chris.diamand@arm.com
Subject: Re: [PATCH v2 1/2] drivers: devfreq: change devfreq workqueue mechanism
Date: Tue, 12 Feb 2019 16:48:26 -0800 [thread overview]
Message-ID: <20190213004826.GW117604@google.com> (raw)
In-Reply-To: <6eb6fc90-27ef-87d2-1123-3d71f0a5102e@partner.samsung.com>
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
next prev parent reply other threads:[~2019-02-13 0:48 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20190211153030eucas1p19bd9a7eca565ca066ab00dc2243cfb46@eucas1p1.samsung.com>
2019-02-11 15:30 ` [PATCH v2 0/2] drivers: devfreq: fix and optimize workqueue mechanism Lukasz Luba
[not found] ` <CGME20190211153035eucas1p12ecdd3289a20ce9fb28588ba20869c60@eucas1p1.samsung.com>
2019-02-11 15:30 ` [PATCH v2 1/2] drivers: devfreq: change devfreq " Lukasz Luba
2019-02-11 21:42 ` Matthias Kaehlcke
2019-02-12 11:20 ` Lukasz Luba
2019-02-12 20:12 ` Matthias Kaehlcke
2019-02-12 21:37 ` Lukasz Luba
2019-02-13 0:48 ` Matthias Kaehlcke [this message]
[not found] ` <CGME20190211153037eucas1p20b80e44795e3599dabd9c2fc0291c063@eucas1p2.samsung.com>
2019-02-11 15:30 ` [PATCH v2 2/2] drivers: devfreq: change deferred work into delayed Lukasz Luba
2019-02-11 21:36 ` Matthias Kaehlcke
2019-02-12 11:03 ` Lukasz Luba
2019-02-12 5:46 ` [PATCH v2 0/2] drivers: devfreq: fix and optimize workqueue mechanism Chanwoo Choi
2019-02-12 12:05 ` Lukasz Luba
2019-02-13 1:09 ` Chanwoo Choi
2019-02-13 10:47 ` Lukasz Luba
2019-02-14 4:00 ` Chanwoo Choi
2019-02-12 19:32 ` Matthias Kaehlcke
2019-02-12 21:20 ` Lukasz Luba
2019-02-13 0:30 ` Matthias Kaehlcke
2019-02-13 13:00 ` Lukasz Luba
2019-02-14 20:40 ` Matthias Kaehlcke
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190213004826.GW117604@google.com \
--to=mka@chromium.org \
--cc=b.zolnierkie@samsung.com \
--cc=chris.diamand@arm.com \
--cc=cw00.choi@samsung.com \
--cc=joel@joelfernandes.org \
--cc=kyungmin.park@samsung.com \
--cc=l.luba@partner.samsung.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=myungjoo.ham@samsung.com \
--cc=s.nawrocki@samsung.com \
--cc=tkjos@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).