lkml.org 
[lkml]   [2012]   [Feb]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [RFC PATCH] PM / QoS: add pm_qos_update_request_timeout API
From
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/

\
 
 \ /
  Last update: 2012-02-15 07:47    [W:0.113 / U:0.608 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site