From: Kamil Konieczny <k.konieczny@samsung.com>
To: Chanwoo Choi <cw00.choi@samsung.com>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
Krzysztof Kozlowski <krzk@kernel.org>,
Kyungmin Park <kyungmin.park@samsung.com>,
linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
Marek Szyprowski <m.szyprowski@samsung.com>,
MyungJoo Ham <myungjoo.ham@samsung.com>
Subject: Re: [PATCH 2/4] PM / devfreq: add possibility for delayed work
Date: Tue, 10 Dec 2019 10:03:27 +0100 [thread overview]
Message-ID: <03350bb0-11a8-a179-7197-1049dc5e1ef6@samsung.com> (raw)
In-Reply-To: <2881c0b8-9d05-aa5f-c216-8d5a09eb0b00@samsung.com>
Hi Chanwoo,
On 10.12.2019 02:47, Chanwoo Choi wrote:
> On 12/9/19 11:44 PM, Kamil Konieczny wrote:
>> Current devfreq workqueue uses deferred timer. Introduce sysfs
>> file delayed_timer and use it for change from deferred to
>> delayed work. The default is to use old deferred one, which
>> saves power, but can miss increased demand for higher bus
>> frequency if timer was assigned to idle cpu.
>
> As I commented on patch1, If you hope to change the feature
> related to both performance and power-consumption,
> you have to suggest the reasonable data with test result
> on multiple scenarios.
Unfortunatly I do not have such tests. Do you have them ?
May you share them with me or Marek ?
> Firstly,
> I don't agree to add 'delayed_timer' sysfs entries.
> If some device driver want to use the different type of
> workqueue, they can choice the workqueue type in the
> probe function of device driver.
sysfs allows change in runtime
> Secondly, the 'dealyed_timer' is not for all devfreq
> device driver. Only devfreq device driver uses the
> 'simple_ondemand' governor. It is wrong to show
> without any specific reason.
Good point, performance or powersave with fixed max or min freq
do not need them.
> If you suggest the reasonable data with test result,
> I prefer to add the new flag to 'struct devfreq_dev_profile'.
imho users of devfreq may give it a try and perform tests themselfs
and then share results.
>>
>> Signed-off-by: Kamil Konieczny <k.konieczny@samsung.com>
>> ---
>> Documentation/ABI/testing/sysfs-class-devfreq | 10 ++++
>> drivers/devfreq/devfreq.c | 46 ++++++++++++++++++-
>> include/linux/devfreq.h | 2 +
>> 3 files changed, 57 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-devfreq b/Documentation/ABI/testing/sysfs-class-devfreq
>> index 9758eb85ade3..07bfd0df6a4a 100644
>> --- a/Documentation/ABI/testing/sysfs-class-devfreq
>> +++ b/Documentation/ABI/testing/sysfs-class-devfreq
>> @@ -30,6 +30,16 @@ Description:
>> target_freq when get_cur_freq() is not implemented by
>> devfreq driver.
>>
>> +What: /sys/class/devfreq/.../delayed_timer
>> +Date: December 2019
>> +Contact: Kamil Konieczny <k.konieczny@samsung.com>
>> +Description:
>> + This ABI shows or clears timer type used by devfreq
>> + workqueue. When 0, it uses default deferred timer.
>> + When set to 1 devfreq will use delayed timer. Example
>> + useage:
>> + echo 1 > /sys/class/devfreq/.../delayed_timer
>> +
>> What: /sys/class/devfreq/.../target_freq
>> Date: September 2012
>> Contact: Rajagopal Venkat <rajagopal.venkat@linaro.org>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 955949c6fc1f..c277d1770fef 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -445,7 +445,11 @@ void devfreq_monitor_start(struct devfreq *devfreq)
>> if (devfreq->governor->interrupt_driven)
>> return;
>>
>> - INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
>> + if (devfreq->delayed_timer)
>> + INIT_DELAYED_WORK(&devfreq->work, devfreq_monitor);
>> + else
>> + INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
>> +
>> if (devfreq->profile->polling_ms)
>> queue_delayed_work(devfreq_wq, &devfreq->work,
>> msecs_to_jiffies(devfreq->profile->polling_ms));
>> @@ -698,6 +702,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>> devfreq->last_status.current_frequency = profile->initial_freq;
>> devfreq->data = data;
>> devfreq->nb.notifier_call = devfreq_notifier_call;
>> + devfreq->delayed_timer = false;
>>
>> if (!devfreq->profile->max_state && !devfreq->profile->freq_table) {
>> mutex_unlock(&devfreq->lock);
>> @@ -1288,6 +1293,44 @@ static ssize_t available_governors_show(struct device *d,
>> }
>> static DEVICE_ATTR_RO(available_governors);
>>
>> +static ssize_t delayed_timer_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + int i;
>> +
>> + i = to_devfreq(dev)->delayed_timer ? 1 : 0;
>> + return sprintf(buf, "%d\n", i);
>> +}
>> +
>> +static ssize_t delayed_timer_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct devfreq *df = to_devfreq(dev);
>> + bool old_timer;
>> + int value, ret;
>> +
>> + if (!df->governor)
>> + return -EINVAL;
>> +
>> + ret = kstrtoint(buf, 10, &value);
>> + if (ret || (value != 1 && value != 0))
>> + return -EINVAL;
>> +
>> + mutex_lock(&df->lock);
>> + old_timer = df->delayed_timer;
>> + df->delayed_timer = value == 0 ? false : true;
>> + mutex_unlock(&df->lock);
>> +
>> + if (old_timer != df->delayed_timer) {
>> + devfreq_monitor_stop(df);
>> + devfreq_monitor_start(df);
>> + }
>> +
>> + return count;
>> +}
>> +static DEVICE_ATTR_RW(delayed_timer);
>> +
>> static ssize_t cur_freq_show(struct device *dev, struct device_attribute *attr,
>> char *buf)
>> {
>> @@ -1513,6 +1556,7 @@ static struct attribute *devfreq_attrs[] = {
>> &dev_attr_name.attr,
>> &dev_attr_governor.attr,
>> &dev_attr_available_governors.attr,
>> + &dev_attr_delayed_timer.attr,
>> &dev_attr_cur_freq.attr,
>> &dev_attr_available_frequencies.attr,
>> &dev_attr_target_freq.attr,
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index de2fdc56aa5b..761aa0a09db7 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -134,6 +134,7 @@ struct devfreq_stats {
>> * reevaluate operable frequencies. Devfreq users may use
>> * devfreq.nb to the corresponding register notifier call chain.
>> * @work: delayed work for load monitoring.
>> + * @delayed_timer: use delayed or deferred timer for workqueue.
>> * @previous_freq: previously configured frequency value.
>> * @data: Private data of the governor. The devfreq framework does not
>> * touch this.
>> @@ -166,6 +167,7 @@ struct devfreq {
>> char governor_name[DEVFREQ_NAME_LEN];
>> struct notifier_block nb;
>> struct delayed_work work;
>> + bool delayed_timer;
>>
>> unsigned long previous_freq;
>> struct devfreq_dev_status last_status;
>>
>
>
--
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland
next prev parent reply other threads:[~2019-12-10 9:03 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20191209144440eucas1p10fc74a8cbc07df72bf1bcd52a7260c42@eucas1p1.samsung.com>
2019-12-09 14:44 ` [PATCH 0/4] PM / devfreq: add possibility for delayed work Kamil Konieczny
[not found] ` <CGME20191209144441eucas1p16945780c1a1ff3302a233414ae6aace2@eucas1p1.samsung.com>
2019-12-09 14:44 ` [PATCH 1/4] PM / devfreq: reuse system workqueue machanism Kamil Konieczny
2019-12-10 1:41 ` Chanwoo Choi
2019-12-10 7:28 ` Kamil Konieczny
2019-12-10 7:53 ` Chanwoo Choi
2019-12-10 9:28 ` Kamil Konieczny
2019-12-10 9:42 ` Chanwoo Choi
[not found] ` <CGME20191209144441eucas1p2ccd371e5861e8c0a3948cdc6640ad0d5@eucas1p2.samsung.com>
2019-12-09 14:44 ` [PATCH 2/4] PM / devfreq: add possibility for delayed work Kamil Konieczny
2019-12-09 19:27 ` Matthias Kaehlcke
2019-12-10 10:15 ` Kamil Konieczny
2019-12-10 1:47 ` Chanwoo Choi
2019-12-10 9:03 ` Kamil Konieczny [this message]
2019-12-10 9:25 ` Chanwoo Choi
2020-01-20 14:20 ` Kamil Konieczny
[not found] ` <CGME20191209144442eucas1p1e4f5cf4a1716262e2b6715fb41876f91@eucas1p1.samsung.com>
2019-12-09 14:44 ` [PATCH 3/4] PM / devfreq: Kconfig: add DEVFREQ_DELAYED_TIMER option Kamil Konieczny
2019-12-09 19:34 ` Matthias Kaehlcke
2019-12-10 9:39 ` Kamil Konieczny
2019-12-10 1:54 ` Chanwoo Choi
[not found] ` <CGME20191209144442eucas1p214c553519b7a9d3d005802984bc6fb31@eucas1p2.samsung.com>
2019-12-09 14:44 ` [PATCH 4/4] PM / devfreq: use delayed work if DEVFREQ_DELAYED_TIMER set Kamil Konieczny
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=03350bb0-11a8-a179-7197-1049dc5e1ef6@samsung.com \
--to=k.konieczny@samsung.com \
--cc=b.zolnierkie@samsung.com \
--cc=cw00.choi@samsung.com \
--cc=krzk@kernel.org \
--cc=kyungmin.park@samsung.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=myungjoo.ham@samsung.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).