Re: [PATCH 0/1] drivers: devfreq: use DELAYED_WORK in DEVFREQ monitoring subsystem

[Lukasz Luba]

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