Messages in this thread | | | Subject | Re: [PATCH 2/4] PM / devfreq: add possibility for delayed work | From | Kamil Konieczny <> | Date | Tue, 10 Dec 2019 11:15:13 +0100 |
| |
Hi,
On 09.12.2019 20:27, Matthias Kaehlcke wrote: > Hi, > > On Mon, Dec 09, 2019 at 03:44:23PM +0100, 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. >> >> 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; > > devfreq is zero initialized (allocated with kzalloc()), hence this is > unnecessary.
ok, I will remove it
> >> >> 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); > > get rid of 'i' and just use df->delayed_timer in sprintf().
ok
> >> +} >> + >> +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; > > Not a great name, the variable doesn't hold a timer. I'd suggest something > like 'prev_val'.
ok, will change to 'bool prev, value;'
> >> + int value, ret; >> + >> + if (!df->governor) >> + return -EINVAL; >> + >> + ret = kstrtoint(buf, 10, &value); >> + if (ret || (value != 1 && value != 0)) >> + return -EINVAL; > > use kstrtobool() instead of partially re-implementing it.
ok
>> + >> + mutex_lock(&df->lock); >> + old_timer = df->delayed_timer; >> + df->delayed_timer = value == 0 ? false : true; > > What's wrong with: > > df->delayed_timer = value; > > ?
ok, when I change type of value and use above kstrtobool, this will be ok
> >> + mutex_unlock(&df->lock); > > Does the locking as is actually provide any value? The use case seems to > be concurrent setting of the sysfs attribute. The lock is released after > the assignment, hence the value of 'df->delayed_timer' could be overwritten > before the condition below is evaluated.
Good point, before send I spotted these lines and "optimized" code so it got worse...
> If you want to protect against this case you need something like this: > > // don't release the lock before evaluating the condition > >> + if (old_timer != df->delayed_timer) {
even better: if (prev != value) { so no need for ugly mutex_unlock in both if-else paths
> mutex_unlock(&df->lock);
>> + devfreq_monitor_stop(df); >> + devfreq_monitor_start(df); >> + } > else { > mutex_unlock(&df->lock); > } > > I don't pretend it's pretty ;-)
Thank you for review.
-- Best regards, Kamil Konieczny Samsung R&D Institute Poland
| |