Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 19 Jan 2021 15:35:56 +0000
From:   Quentin Perret <qperret@google.com>
To:     Qais Yousef <qais.yousef@arm.com>
Cc:     "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        linux-kernel@vger.kernel.org,
        Valentin Schneider <valentin.schneider@arm.com>,
        Morten Rasmussen <morten.rasmussen@arm.com>
Subject: Re: [PATCH] sched/eas: Don't update misfit status if the task is
 pinned
Message-ID: <YAb8XGyp3NtrHl+U@google.com>
References: <20210119120755.2425264-1-qais.yousef@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210119120755.2425264-1-qais.yousef@arm.com>
Precedence: bulk

On Tuesday 19 Jan 2021 at 12:07:55 (+0000), Qais Yousef wrote:
> If the task is pinned to a cpu, setting the misfit status means that
> we'll unnecessarily continuously attempt to migrate the task but fail.
> 
> This continuous failure will cause the balance_interval to increase to
> a high value, and eventually cause unnecessary significant delays in
> balancing the system when real imbalance happens.
> 
> Caught while testing uclamp where rt-app calibration loop was pinned to
> cpu 0, shortly after which we spawn another task with high util_clamp
> value. The task was failing to migrate after over 40ms of runtime due to
> balance_interval unnecessary expanded to a very high value from the
> calibration loop.
> 
> Not done here, but it could be useful to extend the check for pinning to
> verify that the affinity of the task has a cpu that fits. We could end
> up in a similar situation otherwise.

Do you mean failing the sched_setaffinity syscall if e.g. the task
has a min clamp that is higher than the capacity of the CPUs to which it
will be pinned? If so, I'm not sure if we really want that.

But this patch makes sense on its own for sure, so:

Reviewed-by: Quentin Perret <qperret@google.com>

Thanks,
Quentin