Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Lukasz Luba <lukasz.luba@arm.com>
To: Chanwoo Choi <cw00.choi@samsung.com>,
	myungjoo.ham@samsung.com, kyungmin.park@samsung.com,
	linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: b.zolnierkie@samsung.com, Kamil Konieczny <k.konieczny@samsung.com>
Subject: Re: [PATCH 0/1] drivers: devfreq: use DELAYED_WORK in DEVFREQ monitoring subsystem
Date: Fri, 31 Jan 2020 09:38:19 +0000
Message-ID: <729911b2-434b-fbe5-b556-5bdba8114085@arm.com> (raw)
In-Reply-To: <f5c5cd64-b72c-2802-f6ea-ab3d28483260@samsung.com>

Hi Chanwoo,

On 1/31/20 12:47 AM, Chanwoo Choi wrote:
> On 1/31/20 9:42 AM, Chanwoo Choi wrote:
>> Hi Lukasz,
>>
>> On 1/30/20 8:47 PM, Lukasz Luba wrote:
>>> Hi Chanwoo, MyungJoo,
>>>
>>> Gentle ping. The issue is not only in the devfreq itself,
>>> but also it affects thermal. The devfreq cooling rely on
>>> busy_time and total_time updated by the devfreq monitoring
>>> (in simple_ondemand).
>>> Thermal uses DELAYED_WORK and is more reliable, but uses stale
>>> data from devfreq_dev_stats. It is especially visible when
>>> you have cgroup spanning one cluster. Android uses cgroups
>>> heavily. You can make easily this setup using 'taskset',
>>> run some benchmarks and observe 'devfreq_monitor' traces and
>>> timestamps, i.e. for your exynos-bus.
>>>
>>> The patch is really non-invasive and simple. It can be a good starting
>>> point for testing and proposing other solutions.
>>
>> Sorry for late reply. I'm preparing the RFC patch about my approach
>> to support this requirement as following:
>>
>> As you knew, DEFERRABLE_WORK with CONFIG_NO_HZ focuses on removing
>> the redundant of power-consumption by preventing the unneeded wakeup
>> from idle state if there are no any interrupts and runnable threads.
>>
>> Finally, I agree the requirement of delaywd_work for devfreq subsystem.
>> But, I would like to support both deferrable_work and delayed_work
>> on devfreq subsystem. It is better to select either deferrable_work
>> or delayed_work by user like Kamil's suggestion[1].
>> [1] https://lore.kernel.org/patchwork/patch/1164317/
>> - [2/4] PM / devfreq: add possibility for delayed work
>>
>> But, I want to change the timer type for devfreq device
>> using simple_ondemand governor via sysfs as following:
>>
>> Example:
>>
>> 1.
>> enum work_timer_type {
>> 	DEVFREQ_WORK_TIMER_DEFERRABLE = 0,
>> 	DEVFREQ_WORK_TIMER_DELAYED = 0,
>> };
>>
>> struct devfreq_simple_ondemand_data {
>> 	unsigned int upthreshold;
>> 	unsigned int downdifferential;
>> 	enum work_timer_type timer_type;
>> };
>>
>> The developer of devfreq device driver can choose
>> the default work time type by initializing the 'timer_type of
>> struct devfreq_simple_ondemand_data'.
>>
>> 2. Change the work timer type at the runtime
>> - Change the work timer type from 'deferrable' to 'delayed'
>> $ echo delayed > /sys/class/devfreq/devfreq0/work_timer_type
>> $ cat /sys/class/devfreq/devfreq0/work_timer_type
>> delayed
>>
>> - Change the work timer type from 'delayed' to 'deferrable'
>> $ echo deferrable > /sys/class/devfreq/devfreq0/work_timer_type
>> $ cat /sys/class/devfreq/devfreq0/work_timer_type
>> deferrable
>>
> 
> And
> Only show '/sys/class/devfreq/devfreq0/work_timer_type' sysfs attribute,
> if devfreq device uses the simple_ondemand. Because this 'work_timer_type'
> sysfs attribute only depends on simple_ondemand governor and are useful.
> 
> So, 'work_timer_type' sysfs attribute will be handled
> at drivers/devfreq/governor_simpleondemand.c.
> 
> After posting my suggestion, we can discuss it.
> 
> 
>> I'm developing the RFC patch and then I'll send it as soon as possible.

Good, thank you for the explanation. For the first glance the design
looks OK, we can discuss it a bit more in you RFC series.
I would recommend to not make it conditional on simple_ondemand governor
just add a comment that for i.e. performance or passive governors it has
less sense to use this setting. There might be some other governors
loaded as modules, which could benefit from it, or in Android e.g.
https://android.googlesource.com/kernel/msm/+/refs/heads/android-msm-coral-4.14-android10/drivers/devfreq/governor_msm_adreno_tz.c

It would be good if it can land in mainline before v5.8-v5.9.

Regards,
Lukasz



  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-27 15:17 lukasz.luba
2020-01-27 15:17 ` [PATCH 1/1] drivers: devfreq: add DELAYED_WORK to " lukasz.luba
2020-01-30 11:47 ` [PATCH 0/1] drivers: devfreq: use DELAYED_WORK in DEVFREQ " Lukasz Luba
2020-01-31  0:42   ` Chanwoo Choi
2020-01-31  0:47     ` Chanwoo Choi
2020-01-31  9:38       ` Lukasz Luba [this message]
2020-02-03  1:10         ` Chanwoo Choi
2020-01-30 11:47 ` Lukasz Luba

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=729911b2-434b-fbe5-b556-5bdba8114085@arm.com \
    --to=lukasz.luba@arm.com \
    --cc=b.zolnierkie@samsung.com \
    --cc=cw00.choi@samsung.com \
    --cc=k.konieczny@samsung.com \
    --cc=kyungmin.park@samsung.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --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

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git