lkml.org 
[lkml]   [2021]   [Jun]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH] sched/uclamp: Fix getting unreasonable ucalmp_max when rq is idle
    Date

    +Cc Patrick's current address

    On 18/06/21 15:23, Xuewen Yan wrote:
    > From: Xuewen Yan <xuewen.yan@unisoc.com>
    >
    > Now in uclamp_rq_util_with(), when the task != NULL, the uclamp_max as following:
    > uc_rq_max = rq->uclamp[UCLAMP_MAX].value;
    > uc_eff_max = uclamp_eff_value(p, UCLAMP_MAX);
    > uclamp_max = max{uc_rq_max, uc_eff_max};
    >
    > Consider the following scenario:
    > (1)the rq is idle, the uc_rq_max is last task's UCLAMP_MAX;
    > (2)the p's uc_eff_max < uc_rq_max.
    >
    > The result is the uclamp_max = uc_rq_max instead of uc_eff_max, it is unreasonable.
    >
    > The scenario often happens in find_energy_efficient_cpu(), when the task has smaller UCLAMP_MAX.
    >
    > Inserts whether the rq is idle in the uclamp_rq_util_with().
    >
    > Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.com>
    > ---
    > kernel/sched/sched.h | 5 ++++-
    > 1 file changed, 4 insertions(+), 1 deletion(-)
    >
    > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
    > index a189bec13729..0feef6af89f2 100644
    > --- a/kernel/sched/sched.h
    > +++ b/kernel/sched/sched.h
    > @@ -2550,7 +2550,10 @@ unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
    >
    > if (p) {
    > min_util = max(min_util, uclamp_eff_value(p, UCLAMP_MIN));
    > - max_util = max(max_util, uclamp_eff_value(p, UCLAMP_MAX));
    > + if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
    > + max_util = uclamp_eff_value(p, UCLAMP_MAX);
    > + else
    > + max_util = max(max_util, uclamp_eff_value(p, UCLAMP_MAX));

    That makes sense to me - enqueuing the task will lift UCLAMP_FLAG_IDLE and
    set the rq clamp as the task's via uclamp_idle_reset().

    Does this want a

    Fixes: 9d20ad7dfc9a ("sched/uclamp: Add uclamp_util_with()")

    ?

    Also, when we have UCLAMP_FLAG_IDLE, we don't even need to read the rq max
    - and I'm pretty sure the same applies to the rq min. What about something like:

    ---
    diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
    index 6510f0a46789..a2c6f6ae6392 100644
    --- a/kernel/sched/sched.h
    +++ b/kernel/sched/sched.h
    @@ -2833,23 +2833,27 @@ static __always_inline
    unsigned long uclamp_rq_util_with(struct rq *rq, unsigned long util,
    struct task_struct *p)
    {
    - unsigned long min_util;
    - unsigned long max_util;
    + unsigned long min_util = 0;
    + unsigned long max_util = 0;

    if (!static_branch_likely(&sched_uclamp_used))
    return util;

    - min_util = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
    - max_util = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
    -
    if (p) {
    - min_util = max(min_util, uclamp_eff_value(p, UCLAMP_MIN));
    + min_util = uclamp_eff_value(p, UCLAMP_MIN);
    + max_util = uclamp_eff_value(p, UCLAMP_MAX);
    +
    + /*
    + * Ignore last runnable task's max clamp, as this task will
    + * reset it. Similarly, no need to read the rq's min clamp.
    + */
    if (rq->uclamp_flags & UCLAMP_FLAG_IDLE)
    - max_util = uclamp_eff_value(p, UCLAMP_MAX);
    - else
    - max_util = max(max_util, uclamp_eff_value(p, UCLAMP_MAX));
    + goto out;
    }

    + min_util = max_t(unsigned long, min_util, READ_ONCE(rq->uclamp[UCLAMP_MIN].value));
    + max_util = max_t(unsigned long, max_util, READ_ONCE(rq->uclamp[UCLAMP_MAX].value));
    +out:
    /*
    * Since CPU's {min,max}_util clamps are MAX aggregated considering
    * RUNNABLE tasks with _different_ clamps, we can end up with an
    \
     
     \ /
      Last update: 2021-06-29 15:51    [W:3.062 / U:0.304 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site