Re: [PATCH 2/4] PM / devfreq: add possibility for delayed work

[Kamil Konieczny]

Hi Chanwoo,

On 10.12.2019 02:47, Chanwoo Choi wrote:
> 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.

Unfortunatly I do not have such tests. Do you have them ?
May you share them with me or Marek ?

> 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. 

sysfs allows change in runtime

> 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.

Good point, performance or powersave with fixed max or min freq
do not need them.

> If you suggest the reasonable data with test result,
> I prefer to add the new flag to 'struct devfreq_dev_profile'.

imho users of devfreq may give it a try and perform tests themselfs
and then share results.

>>
>> Signed-off-by: Kamil Konieczny <k.konieczny@xxxxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxx>
>> +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@xxxxxxxxxx>
>> 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,
Kamil Konieczny
Samsung R&D Institute Poland