Messages in this thread | | | Date | Wed, 15 Feb 2012 15:44:16 +0900 | Subject | Re: [RFC PATCH] PM / QoS: add pm_qos_update_request_timeout API | From | MyungJoo Ham <> |
| |
2012/2/15 Rafael J. Wysocki <rjw@sisk.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |