Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 2/4] PM / devfreq: add possibility for delayed work
To:     Kamil Konieczny <k.konieczny@samsung.com>
Cc:     Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        Kyungmin Park <kyungmin.park@samsung.com>,
        linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        MyungJoo Ham <myungjoo.ham@samsung.com>
From:   Chanwoo Choi <cw00.choi@samsung.com>
Organization: Samsung Electronics
Message-ID: <2881c0b8-9d05-aa5f-c216-8d5a09eb0b00@samsung.com>
Date:   Tue, 10 Dec 2019 10:47:52 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101
        Thunderbird/59.0
MIME-Version: 1.0
In-Reply-To: <20191209144425.13321-3-k.konieczny@samsung.com>
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <20191209144425.13321-1-k.konieczny@samsung.com>
        <CGME20191209144441eucas1p2ccd371e5861e8c0a3948cdc6640ad0d5@eucas1p2.samsung.com>
        <20191209144425.13321-3-k.konieczny@samsung.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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