Re: [PATCH v2 07/12] sched/core: uclamp: enforce last task UCLAMP_MAX

[Suren Baghdasaryan <surenb@google.com>]

On Mon, Jul 23, 2018 at 8:02 AM, Patrick Bellasi
<patrick.bellasi@arm.com> wrote:
> On 20-Jul 18:23, Suren Baghdasaryan wrote:
>> Hi Patrick,
>
> Hi Sure,
> thank!
>
>> On Mon, Jul 16, 2018 at 1:29 AM, Patrick Bellasi
>> <patrick.bellasi@arm.com> wrote:
>
> [...]
>
>> > @@ -977,13 +991,21 @@ static inline void uclamp_cpu_get_id(struct task_struct *p,
>> >         uc_grp = &rq->uclamp.group[clamp_id][0];
>> >         uc_grp[group_id].tasks += 1;
>> >
>> > +       /* Force clamp update on idle exit */
>> > +       uc_cpu = &rq->uclamp;
>> > +       clamp_value = p->uclamp[clamp_id].value;
>> > +       if (unlikely(uc_cpu->flags & UCLAMP_FLAG_IDLE)) {
>>
>> The condition below is not needed because UCLAMP_FLAG_IDLE is set only
>> for UCLAMP_MAX clamp_id, therefore the above condition already covers
>> the one below.
>
> Not really, this function is called two times, the first time to
> update UCLAMP_MIN and a second time to update UCLAMP_MAX.
>
> For both clamp_id we want to force update uc_cpu->value[clamp_id],
> thus the UCLAMP_FLAG_IDLE flag has to be cleared only the second time.
>
> Maybe I can had the following comment to better explain the reason of
> the check:
>
>                 /*
>                  * This function is called for both UCLAMP_MIN (before) and
>                  * UCLAMP_MAX (after). Let's reset the flag only the when
>                  * we know that UCLAMP_MIN has been already updated.
>                  */
>

Ah, my bad. I missed the fact that uc_cpu->flags is shared for both
UCLAMP_MIN and UCLAMP_MAX. It's fine the way it originally was. Thanks
for explanation!

>> > +               if (clamp_id == UCLAMP_MAX)
>> > +                       uc_cpu->flags &= ~UCLAMP_FLAG_IDLE;
>> > +               uc_cpu->value[clamp_id] = clamp_value;
>> > +               return;
>> > +       }
>
> [...]
>
> --
> #include <best/regards.h>
>
> Patrick Bellasi