Re: [PATCH 2/4] PM / devfreq: add possibility for delayed work
From: Kamil Konieczny
Date: Mon Jan 20 2020 - 09:20:26 EST
Hi Chanwoo,
here are some logs from Exynos XU3 with next-20200113
As you can see from them, counters saturate to -1 and then are useless.
The solution will be to change workqueue type from DEFERRED into DELAYED
(default polling time 50ms or few times more seems fine).
I changed exynos-nocp for some more debug data:
--- a/drivers/devfreq/event/exynos-nocp.c
+++ b/drivers/devfreq/event/exynos-nocp.c
@@ -167,7 +167,8 @@ static int exynos_nocp_get_event(struct devfreq_event_dev *edev,
edata->load_count = ((counter[1] << 16) | counter[0]);
edata->total_count = ((counter[3] << 16) | counter[2]);
- dev_dbg(&edev->dev, "%s (event: %ld/%ld)\n", edev->desc->name,
+ dev_dbg(&edev->dev, "%s (event: %ld/%ld) %08lx/%08lx\n", edev->desc->name,
+edata->load_count, edata->total_count,
edata->load_count, edata->total_count);
Below are logs:
root@target:~/devfreq# cat nocp_debug
echo -n 'func exynos_nocp_get_event +p' > /sys/kernel/debug/dynamic_debug/control
root@target:~/devfreq# dmesg|tail
[ 171.619022] devfreq-event event2: nocp@10ca1800 (event: 15170920/4932961) 00e77d68/004b4561
[ 171.619088] devfreq-event event3: nocp@10ca1c00 (event: 0/4931345) 00000000/004b3f11
[ 194.358276] devfreq-event event0: nocp@10ca1000 (event: -1/1875960257) ffffffff/6fd0e1c1
[ 194.358529] devfreq-event event1: nocp@10ca1400 (event: 0/1875978297) 00000000/6fd12839
[ 194.358860] devfreq-event event2: nocp@10ca1800 (event: -1/1875994997) ffffffff/6fd16975
[ 194.359162] devfreq-event event3: nocp@10ca1c00 (event: 0/1876014197) 00000000/6fd1b475
[ 194.457796] devfreq-event event0: nocp@10ca1000 (event: 25127060/8085549) 017f6894/007b602d
[ 194.457930] devfreq-event event1: nocp@10ca1400 (event: 0/8062505) 00000000/007b0629
[ 194.458044] devfreq-event event2: nocp@10ca1800 (event: 24674808/8032785) 017881f8/007a9211
[ 194.458160] devfreq-event event3: nocp@10ca1c00 (event: 0/8014513) 00000000/007a4ab1
root@target:~/devfreq# dmesg|tail
[ 938.626509] devfreq-event event2: nocp@10ca1800 (event: -1/-1) ffffffff/ffffffff
[ 938.626656] devfreq-event event3: nocp@10ca1c00 (event: 0/-1) 00000000/ffffffff
[ 1026.756029] devfreq-event event0: nocp@10ca1000 (event: -1/-1) ffffffff/ffffffff
[ 1026.756386] devfreq-event event1: nocp@10ca1400 (event: 0/-1) 00000000/ffffffff
[ 1026.756620] devfreq-event event2: nocp@10ca1800 (event: -1/-1) ffffffff/ffffffff
[ 1026.756831] devfreq-event event3: nocp@10ca1c00 (event: 0/-1) 00000000/ffffffff
[ 1026.875345] devfreq-event event0: nocp@10ca1000 (event: 30306432/9742217) 01ce7080/0094a789
[ 1026.875466] devfreq-event event1: nocp@10ca1400 (event: 0/9733001) 00000000/00948389
[ 1026.875562] devfreq-event event2: nocp@10ca1800 (event: 29891212/9717537) 01c81a8c/00944721
[ 1026.875658] devfreq-event event3: nocp@10ca1c00 (event: 0/9702245) 00000000/00940b65
root@target:~/devfreq# dmesg|tail
[ 1231.215227] devfreq-event event2: nocp@10ca1800 (event: 20069796/6510789) 01323da4/006358c5
[ 1231.215362] devfreq-event event3: nocp@10ca1c00 (event: 0/6505265) 00000000/00634331
[ 1231.274744] devfreq-event event0: nocp@10ca1000 (event: 15373176/4877213) 00ea9378/004a6b9d
[ 1231.274840] devfreq-event event1: nocp@10ca1400 (event: 0/4869301) 00000000/004a4cb5
[ 1231.274928] devfreq-event event2: nocp@10ca1800 (event: 15107524/4860117) 00e685c4/004a28d5
[ 1231.275017] devfreq-event event3: nocp@10ca1c00 (event: 0/4850901) 00000000/004a04d5
[ 1345.925227] devfreq-event event0: nocp@10ca1000 (event: -1/-1) ffffffff/ffffffff
[ 1345.925540] devfreq-event event1: nocp@10ca1400 (event: 0/-1) 00000000/ffffffff
[ 1345.925751] devfreq-event event2: nocp@10ca1800 (event: -1/-1) ffffffff/ffffffff
[ 1345.926095] devfreq-event event3: nocp@10ca1c00 (event: 0/-1) 00000000/ffffffff
root@target:~/devfreq# uname -a
Linux target 5.5.0-rc5-next-20200113-00001-g9c27c4bdca7e #82 SMP PREEMPT Mon Jan 20 14:31:11 CET 2020 armv7l GNU/Linux
root@target:~/devfreq# cat /sys/class/devfreq/devfreq2/trans_stat
From : To
: 88700000 133000000 177400000 266000000 532000000 time(ms)
* 88700000: 0 0 0 0 31 660390
133000000: 4 0 0 0 3 32630
177400000: 2 0 0 0 1 103520
266000000: 1 2 1 0 0 8020
532000000: 24 5 2 4 0 680050
Total transition : 80
On 10.12.2019 10:03, Kamil Konieczny wrote:
> 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@xxxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxx>
>>> +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@xxxxxxxxxx>
>>> 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