| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 10 Dec 2019 11:15:13 +0100
From: Kamil Konieczny <k.konieczny@...sung.com>
To: Matthias Kaehlcke <mka@...omium.org>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
Chanwoo Choi <cw00.choi@...sung.com>,
Krzysztof Kozlowski <krzk@...nel.org>,
Kyungmin Park <kyungmin.park@...sung.com>,
linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
Marek Szyprowski <m.szyprowski@...sung.com>,
MyungJoo Ham <myungjoo.ham@...sung.com>
Subject: Re: [PATCH 2/4] PM / devfreq: add possibility for delayed work
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@...sung.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@...sung.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@...aro.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
Powered by blists - more mailing lists