lkml.org 
[lkml]   [2022]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 17/22] sched/core: fix opencoded cpumask_any_but()
On Tue, May 10, 2022 at 08:47:45AM -0700, Yury Norov wrote:
> sched_core_cpu_starting() and sched_core_cpu_deactivate() implement
> opencoded cpumask_any_but(). Fix it.
>
> CC: Ben Segall <bsegall@google.com>
> CC: Daniel Bristot de Oliveira <bristot@redhat.com>
> CC: Dietmar Eggemann <dietmar.eggemann@arm.com>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Juri Lelli <juri.lelli@redhat.com>
> CC: Mel Gorman <mgorman@suse.de>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Steven Rostedt <rostedt@goodmis.org>
> CC: Valentin Schneider <vschneid@redhat.com>
> CC: Vincent Guittot <vincent.guittot@linaro.org>
> CC: linux-kernel@vger.kernel.org
> Signed-off-by: Yury Norov <yury.norov@gmail.com>
> ---
> kernel/sched/core.c | 33 +++++++++++++--------------------
> 1 file changed, 13 insertions(+), 20 deletions(-)
>
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index f5ebc392493d..9700001948d0 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -6125,7 +6125,7 @@ static void queue_core_balance(struct rq *rq)
> static void sched_core_cpu_starting(unsigned int cpu)
> {
> const struct cpumask *smt_mask = cpu_smt_mask(cpu);
> - struct rq *rq = cpu_rq(cpu), *core_rq = NULL;
> + struct rq *rq = cpu_rq(cpu), *core_rq;
> unsigned long flags;
> int t;
>
> @@ -6138,19 +6138,16 @@ static void sched_core_cpu_starting(unsigned int cpu)
> goto unlock;
>
> /* find the leader */
> - for_each_cpu(t, smt_mask) {
> - if (t == cpu)
> - continue;
> - rq = cpu_rq(t);
> - if (rq->core == rq) {
> - core_rq = rq;
> - break;
> - }
> - }
> + t = cpumask_any_but(smt_mask, cpu);
> + if (t >= nr_cpu_ids)
> + goto unlock;
>
> - if (WARN_ON_ONCE(!core_rq)) /* whoopsie */
> + rq = cpu_rq(t);
> + if (WARN_ON_ONCE(rq->core != rq)) /* whoopsie */
> goto unlock;
>
> + core_rq = rq;
> +
> /* install and validate core_rq */
> for_each_cpu(t, smt_mask) {
> rq = cpu_rq(t);

I don't think this is equivalent. Imagine SMT4, with:

rqN->core_rq = rq0

Now, further suppose smt0-2 are online and we're about to online smt3.
Then t above is free to be smt2, which then results in insta triggering:

+ if (WARN_ON_ONCE(rq->core != rq)) /* whoopsie */

You seem to have lost how the first loop searches for rq->core.

Please, be more careful. Also, all of this is super cold path don't
bother with optimizations. Much of the patches you have in this series
fall under that.

\
 
 \ /
  Last update: 2022-05-10 18:40    [W:1.051 / U:0.268 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site