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:3.746 / U:0.300 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site