linux-kernel - Re: [PATCH v2 1/2] drivers: devfreq: change devfreq workqueue mechanism

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <01644c1c-a419-9864-3b34-0837a114b799@partner.samsung.com>
Date:   Tue, 12 Feb 2019 12:20:42 +0100
From:   Lukasz Luba <l.luba@...tner.samsung.com>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        b.zolnierkie@...sung.com, myungjoo.ham@...sung.com,
        cw00.choi@...sung.com, kyungmin.park@...sung.com,
        m.szyprowski@...sung.com, s.nawrocki@...sung.com, tkjos@...gle.com,
        joel@...lfernandes.org, chris.diamand@....com
Subject: Re: [PATCH v2 1/2] drivers: devfreq: change devfreq workqueue
 mechanism

Hi Matthias,

On 2/11/19 10:42 PM, Matthias Kaehlcke wrote:
> Hi Lukasz,
> 
> On Mon, Feb 11, 2019 at 04:30:04PM +0100, Lukasz Luba wrote:
>> There is no need for creating another workqueue in the system,
>> the existing one should meet the requirements.
>> This patch removes devfreq's custom workqueue and uses system one.
>> It switches from queue_delayed_work() to schedule_delayed_work().
>> It also does not wake up the system when it enters suspend (this
>> functionality stays the same).
>>
>> Signed-off-by: Lukasz Luba <l.luba@...tner.samsung.com>
>> ---
>>   drivers/devfreq/devfreq.c | 25 ++++++-------------------
>>   1 file changed, 6 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 0ae3de7..882e717 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -31,13 +31,6 @@
>>   
>>   static struct class *devfreq_class;
>>   
>> -/*
>> - * devfreq core provides delayed work based load monitoring helper
>> - * functions. Governors can use these or can implement their own
>> - * monitoring mechanism.
>> - */
>> -static struct workqueue_struct *devfreq_wq;
>> -
>>   /* The list of all device-devfreq governors */
>>   static LIST_HEAD(devfreq_governor_list);
>>   /* The list of all device-devfreq */
>> @@ -391,8 +384,8 @@ static void devfreq_monitor(struct work_struct *work)
>>   	if (err)
>>   		dev_err(&devfreq->dev, "dvfs failed with (%d) error\n", err);
>>   
>> -	queue_delayed_work(devfreq_wq, &devfreq->work,
>> -				msecs_to_jiffies(devfreq->profile->polling_ms));
>> +	schedule_delayed_work(&devfreq->work,
>> +			      msecs_to_jiffies(devfreq->profile->polling_ms));
>>   	mutex_unlock(&devfreq->lock);
>>   }
>>   
>> @@ -409,7 +402,7 @@ void devfreq_monitor_start(struct devfreq *devfreq)
>>   {
>>   	INIT_DEFERRABLE_WORK(&devfreq->work, devfreq_monitor);
>>   	if (devfreq->profile->polling_ms)
>> -		queue_delayed_work(devfreq_wq, &devfreq->work,
>> +		schedule_delayed_work(&devfreq->work,
>>   			msecs_to_jiffies(devfreq->profile->polling_ms));
>>   }
>>   EXPORT_SYMBOL(devfreq_monitor_start);
>> @@ -473,7 +466,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
>>   
>>   	if (!delayed_work_pending(&devfreq->work) &&
>>   			devfreq->profile->polling_ms)
>> -		queue_delayed_work(devfreq_wq, &devfreq->work,
>> +		schedule_delayed_work(&devfreq->work,
>>   			msecs_to_jiffies(devfreq->profile->polling_ms));
>>   
>>   	devfreq->last_stat_updated = jiffies;
>> @@ -516,7 +509,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
>>   
>>   	/* if current delay is zero, start polling with new delay */
>>   	if (!cur_delay) {
>> -		queue_delayed_work(devfreq_wq, &devfreq->work,
>> +		schedule_delayed_work(&devfreq->work,
>>   			msecs_to_jiffies(devfreq->profile->polling_ms));
>>   		goto out;
>>   	}
>> @@ -527,7 +520,7 @@ void devfreq_interval_update(struct devfreq *devfreq, unsigned int *delay)
>>   		cancel_delayed_work_sync(&devfreq->work);
>>   		mutex_lock(&devfreq->lock);
>>   		if (!devfreq->stop_polling)
>> -			queue_delayed_work(devfreq_wq, &devfreq->work,
>> +			schedule_delayed_work(&devfreq->work,
>>   			      msecs_to_jiffies(devfreq->profile->polling_ms));
>>   	}
>>   out:
>> @@ -1430,12 +1423,6 @@ static int __init devfreq_init(void)
>>   		return PTR_ERR(devfreq_class);
>>   	}
>>   
>> -	devfreq_wq = create_freezable_workqueue("devfreq_wq");
>> -	if (!devfreq_wq) {
>> -		class_destroy(devfreq_class);
>> -		pr_err("%s: couldn't create workqueue\n", __FILE__);
>> -		return -ENOMEM;
>> -	}
>>   	devfreq_class->dev_groups = devfreq_groups;
>>   
>>   	return 0;
> 
> As commented on v1, the change from a custom to a system workqueue
> seems reasonable to me. However this patch also changes from a
> freezable workqueue to a non-freezable one. C&P of my comments on v1:
> 
> ``WQ_FREEZABLE``
>      A freezable wq participates in the freeze phase of the system
>      suspend operations.  Work items on the wq are drained and no
>      new work item starts execution until thawed.
> 
> I'm not entirely sure what the impact of this is.
> 
> I imagine suspend is potentially quicker because the wq isn't drained,
> but could works that execute during the suspend phase be a problem?
The devfreq supports suspend from v4.20-rc6, which picks OPP for a
device based on its DT 'opp-suspend'. For the devices which do not
choose the suspend OPP it is possible to enter that state with any
frequency. Queuing work for calling governor during suspend which
calculates the device's frequency for the next period is IMO not needed,
The 'next period' is actually suspend and is not related to
'predicted' load by the governor.

You are right, the suspend will be faster.

Regards,
Lukasz
> 
> Cheers
> 
> Matthias
> 
>