Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20180723150249.GD2683@e110439-lin>
References: <20180716082906.6061-1-patrick.bellasi@arm.com>
 <20180716082906.6061-8-patrick.bellasi@arm.com> <CAJuCfpH6CsDVG35Z3NV87mUgJW9TmoCuOs8GeW2TEqkP0K0f9g@mail.gmail.com>
 <20180723150249.GD2683@e110439-lin>
From:   Suren Baghdasaryan <surenb@google.com>
Date:   Mon, 23 Jul 2018 09:40:08 -0700
Message-ID: <CAJuCfpG7vmOJZOuZBqy7G3b-ovXeLmPVB9pBaj=XKviOZ7PWzA@mail.gmail.com>
Subject: Re: [PATCH v2 07/12] sched/core: uclamp: enforce last task UCLAMP_MAX
To:     Patrick Bellasi <patrick.bellasi@arm.com>
Cc:     linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Tejun Heo <tj@kernel.org>,
        "Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
        Viresh Kumar <viresh.kumar@linaro.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Paul Turner <pjt@google.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Morten Rasmussen <morten.rasmussen@arm.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        Todd Kjos <tkjos@google.com>,
        Joel Fernandes <joelaf@google.com>,
        Steve Muckle <smuckle@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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