Messages in this thread | | | Subject | Re: [PATCH 2/4] PM / devfreq: add possibility for delayed work | From | Chanwoo Choi <> | Date | Tue, 10 Dec 2019 10:47:52 +0900 |
| |
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.
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.
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.
If you suggest the reasonable data with test result, I prefer to add the new flag to 'struct devfreq_dev_profile'.
> > 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, Chanwoo Choi Samsung Electronics
| |