| 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: Wed, 15 Feb 2012 15:44:16 +0900
From: MyungJoo Ham <myungjoo.ham@...sung.com>
To: "Rafael J. Wysocki" <rjw@...k.pl>
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
Kevin Hilman <khilman@...com>, Jean Pihet <j-pihet@...com>,
markgross <markgross@...gnar.org>, kyungmin.park@...sung.com
Subject: Re: [RFC PATCH] PM / QoS: add pm_qos_update_request_timeout API
2012/2/15 Rafael J. Wysocki <rjw@...k.pl>:
> Hi,
>
> On Tuesday, February 14, 2012, MyungJoo Ham wrote:
>> The new API, pm_qos_update_request_timeout() is to provide a timeout
>> with pm_qos_update_request.
>
> I need to know what the other people in the CC list think about that. It would
> definitely help if you described a use case.
Use case (current implementation in local repositories) w/ user inputs
(touch screen)
Run DVFS mechanism (both CPUfreq and Devfreq) or devices faster when a
user starts an input.
(default: polling every 100ms. with user inputs: polling every 10ms)
or (depending on projects)
(default: 200MHz ~ 1.5GHz. with user inputs: 800MHz ~ 1.5GHz)
Run DVFS mechanism faster (shorter polling interval) when the
userspace expects a sudden and temporal utilization changes or
fluctuations soon (but the utilization may and may not rise much, so
DVFS mechanism needs to react faster, not to increase frequency
unconditionally.): a new application is being loaded or a bulky task
is being loaded.
>
>> For example, pm_qos_update_request_timeout(req, 100, 1000), means that
>> QoS request on req with value 100 will be active for 1000 jiffies.
>
> Could that be a different time unit instead of jiffies?
Um.. yes.. I guess usec would fit here.
>
[]
>>
>> #define PM_QOS_RESERVED 0
>> #define PM_QOS_CPU_DMA_LATENCY 1
>> @@ -29,6 +30,8 @@
>> struct pm_qos_request {
>> struct plist_node node;
>> int pm_qos_class;
>> + s32 value; /* the back-up value for pm_qos_update_request_timeout */
>
> Well, what about "saved_value"?
Yes.. it looks better.
>
>> /**
>> + * pm_qos_timeout - the timeout handler of pm_qos_update_request_timeout
>> + * @work: work struct for the delayed work (timeout)
>> + *
>> + * This cancels the timeout request and rolls the request value back.
>> + */
>> +static void pm_qos_timeout(struct work_struct *work)
>
> I'd call it pm_qos_work_fn(), so that it's clear what it is.
Ok.. I'll modify it.. How about pm_qos_timeout_work_fn()?
>
[]
>>
>> - if (new_value != req->node.prio)
>> + if (delayed_work_pending(&req->work))
>> + cancel_delayed_work_sync(&req->work);
>> +
>
> That may result in setting the target value to an unwanted one temporarily.
> Namely, if pm_qos_timeout() is already running, it will switch back to the
> "original" target value before the new one is set. The PM QoS users may see
> the "restored" value and use it in their decision making. Do we care?
If pm_qos_timeout is already running at this point, QoS-value will be
restored and PM-QoS users (handlers) will react based on the restored
value. And then, these PM-QoS users (handlers) will again react to the
new_value below.
Having one additional and unnecessary PM-QoS users/handers' side
reaction is surely an overhead; however, it happens only if
pm_qos_timeout is already running at the point of this function call.
It pm_qos_timeout is awaiting for "timeout", then it will simply be
canceled.
If we do cancel_delayed_work rather than cancel_delayed_work_sync
here, we then may have serialization problem; the value set by
pm_qos_update_request() is overriden by pm_qos_timeout().
The purpose of modification here is to guarantee that the
pm_qos_timeout(), which is roll-back capability of
pm_qos_update_request_timeout(), won't override QoS request values set
by another pm_qos_update_request() calls.
Or, we may need to add mutexes in request-updating functions and let
the functions serialized.
>
>> + req->value = new_value;
>> + if (new_value != req->node.prio) {
>> pm_qos_update_target(
>> pm_qos_array[req->pm_qos_class]->constraints,
>> &req->node, PM_QOS_UPDATE_REQ, new_value);
>> + }
>
> Adding brances here doesn't belong to this patch.
Oops.. I'll remove these braces.
>
>> }
>> EXPORT_SYMBOL_GPL(pm_qos_update_request);
>>
>> /**
>> + * pm_qos_update_request_timeout - modifies an existing qos request temporarily.
>> + * @req : handle to list element holding a pm_qos request to use
>> + * @new_value: defines the temporal qos request
>> + * @timeout: the effective duration of this qos request in jiffies
>> + *
>> + * After timeout, this qos request is cancelled automatically.
>> + */
>> +void pm_qos_update_request_timeout(struct pm_qos_request *req, s32 new_value,
>> + unsigned long timeout)
>> +{
>> + if (!req)
>> + return;
>> + if (WARN(!pm_qos_request_active(req),
>> + "pm_qos_update_request_timeout() called for unknown object."))
>
> Please use __func__ in messages like this.
Oh.. yes, I'll. In fact, because it is WARN(), the function name
itself won't be necessary.
>
>> + return;
>> +
>> + if (new_value != req->node.prio) {
>> + pm_qos_update_target(
>> + pm_qos_array[req->pm_qos_class]->constraints,
>> + &req->node, PM_QOS_UPDATE_REQ, new_value);
>> + if (delayed_work_pending(&req->work))
>> + cancel_delayed_work_sync(&req->work);
>
> It seems that if pm_qos_timeout() has been started already, it will overwrite
> the value that we've just set, or am I missing something?
No, you are not.
I should've put cancel_delayed_work_sync before pm_qos_update_target().
>
>> + schedule_delayed_work(&req->work, timeout);
>> + }
>> +}
>> +
>> +/**
>> * pm_qos_remove_request - modifies an existing qos request
>> * @req: handle to request list element
>> *
>> @@ -334,6 +383,9 @@ void pm_qos_remove_request(struct pm_qos_request *req)
>> return;
>> }
>>
>> + if (delayed_work_pending(&req->work))
>> + cancel_delayed_work_sync(&req->work);
>> +
>> pm_qos_update_target(pm_qos_array[req->pm_qos_class]->constraints,
>> &req->node, PM_QOS_REMOVE_REQ,
>> PM_QOS_DEFAULT_VALUE);
>>
>
> Thanks,
> Rafael
Thank you so much!
Cheers!
MyungJoo
--
MyungJoo Ham, Ph.D.
Mobile Software Platform Lab, DMC Business, Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists