Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7c75e5d4-75a2-8ea5-64ad-13794a6036b6@arm.com>
Date:   Fri, 18 Mar 2022 17:28:43 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH] sched/fair: Refactor cpu_util_without()
Content-Language: en-US
To:     Vincent Guittot <vincent.guittot@linaro.org>
Cc:     Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Juri Lelli <juri.lelli@redhat.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mel Gorman <mgorman@suse.de>, Ben Segall <bsegall@google.com>,
        Patrick Bellasi <patrick.bellasi@matbug.net>,
        Vincent Donnefort <Vincent.Donnefort@arm.com>,
        linux-kernel@vger.kernel.org
References: <20220301171727.812157-1-dietmar.eggemann@arm.com>
 <CAKfTPtD6i7oYZuAMYFmxFnRHETUAgKYV1gxoMiMjSksBs6Z=rg@mail.gmail.com>
From:   Dietmar Eggemann <dietmar.eggemann@arm.com>
In-Reply-To: <CAKfTPtD6i7oYZuAMYFmxFnRHETUAgKYV1gxoMiMjSksBs6Z=rg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

- Valentin Schneider <Valentin.Schneider@arm.com>

On 02/03/2022 10:09, Vincent Guittot wrote:
> On Tue, 1 Mar 2022 at 18:17, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:

[...]

> I have only minor comment

Thanks for the review!

[...]

>> +static unsigned long cpu_util_next(int cpu, struct task_struct *p, int dst_cpu)
>> +{

[...]

>> +       if (sched_feat(UTIL_EST)) {
>> +               util_est = READ_ONCE(cfs_rq->avg.util_est.enqueued);
>> +
>> +               /*
>> +                * During wake-up, the task isn't enqueued yet and doesn't
>> +                * appear in the cfs_rq->avg.util_est.enqueued of any rq,
>> +                * so just add it (if needed) to "simulate" what will be
>> +                * cpu_util after the task has been enqueued.
>> +                */
>> +               if (dst_cpu == cpu)
>> +                       util_est += _task_util_est(p);
>> +
> 
> Could you add a comment that explains why the addition above will not
> be removed below by the lsub_positive below so it isn't worth trying
> to optimize such a case?

Yes. I rewored the comments in cpu_util_next() so they also apply when
called by cpu_util_without(). And I use a `if{}/else if{}` here too in v2.
>> +               /*
>> +                * Despite the following checks we still have a small window
>> +                * for a possible race, when an execl's select_task_rq_fair()
>> +                * races with LB's detach_task():
>> +                *
>> +                *   detach_task()
>> +                *     p->on_rq = TASK_ON_RQ_MIGRATING;
>> +                *     ---------------------------------- A
>> +                *     deactivate_task()                   \
>> +                *       dequeue_task()                     + RaceTime
>> +                *         util_est_dequeue()              /
>> +                *     ---------------------------------- B
>> +                *
>> +                * The additional check on "current == p" it's required to
>> +                * properly fix the execl regression and it helps in further
>> +                * reducing the chances for the above race.
>> +                */
>> +               if (unlikely(task_on_rq_queued(p) || current == p))
>> +                       lsub_positive(&util_est, _task_util_est(p));

I did a lot of testing on mainline & v4.20 and there wasn't one
occurrence of `p->on_rq == TASK_ON_RQ_MIGRATING` here. Not for WF_EXEC
tasks (p->on_rq = TASK_ON_RQ_QUEUED) and in case of v4.20 not for
WF_EXEC and WF_TTWU tasks (p->on_rq = 0). So I assume it's not needed. I
left it in v2 though and mentioned it in the additional comment section
of the patch.

[...]

>>  static unsigned long cpu_util_without(int cpu, struct task_struct *p)
>>  {

[...]

>>         /*
>>          * Covered cases:
>>          *
>> @@ -6560,82 +6609,8 @@ static unsigned long cpu_util_without(int cpu, struct task_struct *p)
>>          *    estimation of the spare capacity on that CPU, by just
>>          *    considering the expected utilization of tasks already
>>          *    runnable on that CPU.
> 
> The comment about the covered cases above should be moved in
> cpu_util_next() which is where the cases are covered now

Yes. I Incorporated it into the comments in cpu_util_next() in v2.

[...]