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;
References: <20220828170303.171400-1-schspa@gmail.com>
 <20220828170303.171400-2-schspa@gmail.com>
 <xhsmhh6tt9qde.mognet@vschneid.remote.csb>
User-agent: mu4e 1.10.1; emacs 29.0.60
From:   Schspa Shi <schspa@gmail.com>
To:     Valentin Schneider <vschneid@redhat.com>
Cc:     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, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v8 2/2] sched/rt: Trying to push current task when
 target disable migrating
Date:   Wed, 12 Apr 2023 11:44:40 +0800
In-reply-to: <xhsmhh6tt9qde.mognet@vschneid.remote.csb>
Message-ID: <m2r0spior1.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


Valentin Schneider <vschneid@redhat.com> writes:

> On 29/08/22 01:03, Schspa Shi wrote:
>> When the task to push disable migration, retry to push the current
>> running task on this CPU away, instead doing nothing for this migrate
>> disabled task.
>>
>> CC: Valentin Schneider <vschneid@redhat.com>
>> Signed-off-by: Schspa Shi <schspa@gmail.com>
>> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
>> Reviewed-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
>> ---
>>  kernel/sched/core.c     | 13 ++++++++++++-
>>  kernel/sched/deadline.c |  9 +++++++++
>>  kernel/sched/rt.c       |  8 ++++++++
>>  3 files changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index ee28253c9ac0c..056b336c29e70 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -2503,8 +2503,19 @@ int push_cpu_stop(void *arg)
>>       if (p->sched_class->find_lock_rq)
>>               lowest_rq = p->sched_class->find_lock_rq(p, rq);
>>
>> -	if (!lowest_rq)
>> +	if (!lowest_rq) {
>> +		/*
>> +		 * The find_lock_rq function above could have released the rq
>> +		 * lock and allow p to schedule and be preempted again, and
>> +		 * that lowest_rq could be NULL because p now has the
>> +		 * migrate_disable flag set and not because it could not find
>> +		 * the lowest rq. So we must check task migration flag again.
>> +		 */
>> +		if (unlikely(is_migration_disabled(p)))
>> +			p->migration_flags |= MDF_PUSH;
>> +
>
> Given p has to be on this rq initially, this implies p being migrated away
> to become migration_disabled() (it *can't* be scheduled while the stopper
> is running), in which case it's not on this rq anymore, so do we care?
>

Yes, you are right, we have already have a correct handle for this.

>>               goto out_unlock;
>> +	}
>>
>>       // XXX validate p is still the highest prio task
>>       if (task_rq(p) == rq) {
>> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
>> index e7eea6cde5cb9..c8055b978dbc3 100644
>> --- a/kernel/sched/deadline.c
>> +++ b/kernel/sched/deadline.c
>> @@ -2340,6 +2340,15 @@ static int push_dl_task(struct rq *rq)
>>                */
>>               task = pick_next_pushable_dl_task(rq);
>>               if (task == next_task) {
>> +			/*
>> +			 * If next task has now disabled migrating, see if we
>> +			 * can do resched_curr().
>> +			 */
>> +			if (unlikely(is_migration_disabled(task))) {
>> +				put_task_struct(next_task);
>> +				goto retry;
>> +			}
>> +
>>                       /*
>>                        * The task is still there. We don't try
>>                        * again, some other CPU will pull it when ready.
>> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
>> index 57e8cd5c9c267..381ec05eb2701 100644
>> --- a/kernel/sched/rt.c
>> +++ b/kernel/sched/rt.c
>> @@ -2139,6 +2139,14 @@ static int push_rt_task(struct rq *rq, bool pull)
>>                */
>>               task = pick_next_pushable_task(rq);
>>               if (task == next_task) {
>> +			/*
>> +			 * If next task has now disabled migrating, see if we
>> +			 * can push the current task.
>> +			 */
>> +			if (unlikely(is_migration_disabled(task))) {
>> +				put_task_struct(next_task);
>> +				goto retry;
>> +			}
>
> Similarly here, if the task has been through a switch-in / switch-out
> cycle, then at least for RT we'd have
>
>   set_next_task_rt()
>   `\
>     rt_queue_push_tasks()
>
> which will take care of it.
>

Yes, it will take care of this.

> If the task is preempted by e.g. a DL task, then the retry would fail on
>
>   (next_task->prio < rq->curr->prio)
>

It may fail most of the time, but push_rt_task can run on a different
CPU (the rq != this_rq()), and the rq->curr can be changed. the retry
won't fail in this case. It is the same with the deadline.c.

> and I'm thinking the same logic applies to the deadline.c. IOW, it looks
> like we're already doing the right thing here when the task gets scheduled
> out, so I don't think we need any of this.
>
>>                       /*
>>                        * The task hasn't migrated, and is still the next
>>                        * eligible task, but we failed to find a run-queue
>> --
>> 2.37.2

-- 
BRs
Schspa Shi