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;
From:   Valentin Schneider <vschneid@redhat.com>
To:     Schspa Shi <schspa@gmail.com>, mingo@redhat.com,
        peterz@infradead.org, juri.lelli@redhat.com,
        vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
        rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
        bristot@redhat.com
Cc:     linux-kernel@vger.kernel.org, zhaohui.shi@horizon.ai,
        Schspa Shi <schspa@gmail.com>
Subject: Re: [PATCH v2] sched/rt: fix bad task migration for rt tasks
In-Reply-To: <20220627154051.92599-1-schspa@gmail.com>
References: <20220627154051.92599-1-schspa@gmail.com>
Date:   Fri, 01 Jul 2022 11:21:45 +0100
Message-ID: <xhsmhh7415e12.mognet@vschneid.remote.csb>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On 27/06/22 23:40, Schspa Shi wrote:
> @@ -2115,6 +2115,15 @@ static int push_rt_task(struct rq *rq, bool pull)
>       if (WARN_ON(next_task == rq->curr))
>               return 0;
>
> +	/*
> +	 * It is possible the task has running for a while, we need to check
> +	 * task migration disable flag again. If task migration is disabled,
> +	 * the retry code will retry to push the current running task on this
> +	 * CPU away.
> +	 */
> +	if (unlikely(is_migration_disabled(next_task)))
> +		goto retry;
> +

Can we ever hit this? The previous is_migration_disabled() check is in the
same rq->lock segment.

AFAIA this doesn't fix the problem v1 was fixing, which is next_task can
become migrate_disable() after push_rt_task() goes through
find_lock_lowest_rq().

For the task to still be in the pushable_tasks list after having made
itself migration disabled, it must no longer be current, which means we
enqueued a higher priority RT task, in which case we went through
set_next_task_rt() so we did rt_queue_push_tasks().

So I think what you had in v1 was actually what we needed.

>       /* We might release rq lock */
>       get_task_struct(next_task);
>
> --
> 2.24.3 (Apple Git-128)