Patchwork [v2,2/2] drivers: devfreq: change deferred work into delayed

login
register
mail settings
Submitter Lukasz Luba
Date Feb. 11, 2019, 3:30 p.m.
Message ID <1549899005-7760-3-git-send-email-l.luba@partner.samsung.com>
Download mbox | patch
Permalink /patch/723057/
State New
Headers show

Comments

Lukasz Luba - Feb. 11, 2019, 3:30 p.m.
This patch changes deferred work to delayed work, which is now not missed
when timer is put on CPU that entered idle state.
The devfreq framework governor was not called, thus changing the device's
frequency did not happen.
Benchmarks for stressing Dynamic Memory Controller show x2 (in edge cases
even x5) performance boost with this patch when 'simpleondemand_governor'
is responsible for monitoring the device load and frequency changes.

With this patch, the delayed work is done no mater CPUs' idle.
All of the drivers in devfreq which rely on periodic, guaranteed wakeup
intervals should benefit from it.

Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 drivers/devfreq/devfreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Matthias Kaehlcke - Feb. 11, 2019, 9:36 p.m.
Hi Lukasz,

On Mon, Feb 11, 2019 at 04:30:05PM +0100, Lukasz Luba wrote:
> This patch changes deferred work to delayed work, which is now not missed
> when timer is put on CPU that entered idle state.
> The devfreq framework governor was not called, thus changing the device's
> frequency did not happen.
> Benchmarks for stressing Dynamic Memory Controller show x2 (in edge cases
> even x5) performance boost with this patch when 'simpleondemand_governor'
> is responsible for monitoring the device load and frequency changes.
> 
> With this patch, the delayed work is done no mater CPUs' idle.
> All of the drivers in devfreq which rely on periodic, guaranteed wakeup
> intervals should benefit from it.
> 
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  drivers/devfreq/devfreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 882e717..c200b3c 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -400,7 +400,7 @@ static void devfreq_monitor(struct work_struct *work)
>   */
>  void devfreq_monitor_start(struct devfreq *devfreq)
>  {
> -	INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
> +	INIT_DELAYED_WORK(&devfreq->work, devfreq_monitor);
>  	if (devfreq->profile->polling_ms)
>  		schedule_delayed_work(&devfreq->work,
>  			msecs_to_jiffies(devfreq->profile->polling_ms));

I'd suggest to swap the order of the patches in this series.

Why, you may ask, if the end product is the same? This patch ([2/2])
fixes an actual problem, while IIUC [1/2] is just an improvement, the
fix doesn't really depend on it. If -stable wants to integrate the
fix, they also need to pick the improvement (or resolve a conflict),
which might not be desired.

Otherwise this looks sane to me:

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
Lukasz Luba - Feb. 12, 2019, 11:03 a.m.
Hi Matthias,

On 2/11/19 10:36 PM, Matthias Kaehlcke wrote:
> Hi Lukasz,
> 
> On Mon, Feb 11, 2019 at 04:30:05PM +0100, Lukasz Luba wrote:
>> This patch changes deferred work to delayed work, which is now not missed
>> when timer is put on CPU that entered idle state.
>> The devfreq framework governor was not called, thus changing the device's
>> frequency did not happen.
>> Benchmarks for stressing Dynamic Memory Controller show x2 (in edge cases
>> even x5) performance boost with this patch when 'simpleondemand_governor'
>> is responsible for monitoring the device load and frequency changes.
>>
>> With this patch, the delayed work is done no mater CPUs' idle.
>> All of the drivers in devfreq which rely on periodic, guaranteed wakeup
>> intervals should benefit from it.
>>
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   drivers/devfreq/devfreq.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 882e717..c200b3c 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -400,7 +400,7 @@ static void devfreq_monitor(struct work_struct *work)
>>    */
>>   void devfreq_monitor_start(struct devfreq *devfreq)
>>   {
>> -	INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
>> +	INIT_DELAYED_WORK(&devfreq->work, devfreq_monitor);
>>   	if (devfreq->profile->polling_ms)
>>   		schedule_delayed_work(&devfreq->work,
>>   			msecs_to_jiffies(devfreq->profile->polling_ms));
> 
> I'd suggest to swap the order of the patches in this series.
> 
> Why, you may ask, if the end product is the same? This patch ([2/2])
> fixes an actual problem, while IIUC [1/2] is just an improvement, the
> fix doesn't really depend on it. If -stable wants to integrate the
> fix, they also need to pick the improvement (or resolve a conflict),
> which might not be desired.
Good point, I will reorder them.
> 
> Otherwise this looks sane to me:
> 
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> 
>
Thank you for the review.

Regards,
Lukasz

Patch

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 882e717..c200b3c 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -400,7 +400,7 @@  static void devfreq_monitor(struct work_struct *work)
  */
 void devfreq_monitor_start(struct devfreq *devfreq)
 {
-	INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
+	INIT_DELAYED_WORK(&devfreq->work, devfreq_monitor);
 	if (devfreq->profile->polling_ms)
 		schedule_delayed_work(&devfreq->work,
 			msecs_to_jiffies(devfreq->profile->polling_ms));