Message-ID: <10861916-F001-4AF7-B512-9FFD0D883941@nutanix.com>
Date: Tue, 11 Feb 2025 21:08:22 +0000
From: Harshit Agarwal <harshit@...anix.com>
To: Steven Rostedt <rostedt@...dmis.org>
CC: Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot
	<vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        Valentin
 Schneider <vschneid@...hat.com>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        Jon Kohler <jon@...anix.com>,
        Gauri
 Patwardhan <gauri.patwardhan@...anix.com>,
        Rahul Chunduru
	<rahul.chunduru@...anix.com>,
        Will Ton <william.ton@...anix.com>
Subject: Re: [PATCH] sched/rt: Fix race in push_rt_task



> On Feb 11, 2025, at 7:11 AM, Steven Rostedt <rostedt@...dmis.org> wrote:
> 
> On Tue, 11 Feb 2025 05:46:45 +0000
> Harshit Agarwal <harshit@...anix.com> wrote:
> 
>> Overview
>> ========
>> When a CPU chooses to call push_rt_task and picks a task to push to
>> another CPU's runqueue then it will call find_lock_lowest_rq method
>> which would take a double lock on both CPUs' runqueues. If one of the
>> locks aren't readily available, it may lead to dropping the current
>> runqueue lock and reacquiring both the locks at once. During this window
>> it is possible that the task is already migrated and is running on some
>> other CPU. These cases are already handled. However, if the task is
>> migrated and has already been executed and another CPU is now trying to
>> wake it up (ttwu) such that it is queued again on the runqeue
>> (on_rq is 1) and also if the task was run by the same CPU, then the
>> current checks will pass even though the task was migrated out and is no
>> longer in the pushable tasks list.
> 
> Nice catch! And nice analysis.
> 
>> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
>> index 4b8e33c615b1..d48a9cb9ac92 100644
>> --- a/kernel/sched/rt.c
>> +++ b/kernel/sched/rt.c
>> @@ -1885,6 +1885,27 @@ static int find_lowest_rq(struct task_struct *task)
>> return -1;
>> }
>> 
>> +static struct task_struct *pick_next_pushable_task(struct rq *rq)
>> +{
>> + struct task_struct *p;
>> +
>> + if (!has_pushable_tasks(rq))
>> + return NULL;
>> +
>> + p = plist_first_entry(&rq->rt.pushable_tasks,
>> +       struct task_struct, pushable_tasks);
>> +
>> + BUG_ON(rq->cpu != task_cpu(p));
>> + BUG_ON(task_current(rq, p));
>> + BUG_ON(task_current_donor(rq, p));
>> + BUG_ON(p->nr_cpus_allowed <= 1);
>> +
>> + BUG_ON(!task_on_rq_queued(p));
>> + BUG_ON(!rt_task(p));
>> +
>> + return p;
>> +}
>> +
>> /* Will lock the rq it finds */
>> static struct rq *find_lock_lowest_rq(struct task_struct *task, struct rq *rq)
>> {
>> @@ -1920,13 +1941,18 @@ static struct rq *find_lock_lowest_rq(struct task_struct *task, struct rq *rq)
>>  * It is possible the task was scheduled, set
>>  * "migrate_disabled" and then got preempted, so we must
>>  * check the task migration disable flag here too.
>> +  * Also, the task may have been dequeued and completed
>> +  * execution on the same CPU during this time, therefore
>> +  * check if the task is still at the head of the
>> +  * pushable tasks list.
>>  */
>> if (unlikely(task_rq(task) != rq ||
>>      !cpumask_test_cpu(lowest_rq->cpu, &task->cpus_mask) ||
>>      task_on_cpu(rq, task) ||
>>      !rt_task(task) ||
>>      is_migration_disabled(task) ||
>> -      !task_on_rq_queued(task))) {
>> +      !task_on_rq_queued(task) ||
>> +      task != pick_next_pushable_task(rq))) {
> 
> I'm wondering? Could we replace all of these checks with just:
> 
> task != pick_next_pushable_task(rq)
> ?
> 
> I mean, what check above would fail and not cause that one to fail?
> 
> Maybe the is_migration_disabled(), but I'd like to remove any of those
> checks that would be covered with the "pick_next_pushable_task()".
> 
> -- Steve
> 

Thanks Steve for taking a look. Yes we should ideally remove any conditions that are 
subsumed by task != pick_next_pushable_task condition, however I am not sure if anyone (say ttwu())
will try to modify p’s state without removing it from the pushable tasks list first. In such a case
pick_next_pushable_task will still pick p again and then it will match and will proceed to do the migration
while ttwu() is trying to wake it up. Most likely, some asserts like !task_on_rq_queued etc will be hit in
pick_next_pushable_task as soon as p is picked. If we can be sure that p’s state is modified by others
only after it being removed from pushable tasks list of this CPU then it should be safe to remove it
else not. 

Please me know what do you think.

Regards,
Harshit


> 
> 
>> 
>> double_unlock_balance(rq, lowest_rq);
>> lowest_rq = NULL;
>> @@ -1946,27 +1972,6 @@ static struct rq *find_lock_lowest_rq(struct task_struct *task, struct rq *rq)
>> return lowest_rq;
>> }
>> 
>> -static struct task_struct *pick_next_pushable_task(struct rq *rq)
>> -{
>> - struct task_struct *p;
>> -
>> - if (!has_pushable_tasks(rq))
>> - return NULL;
>> -
>> - p = plist_first_entry(&rq->rt.pushable_tasks,
>> -       struct task_struct, pushable_tasks);
>> -
>> - BUG_ON(rq->cpu != task_cpu(p));
>> - BUG_ON(task_current(rq, p));
>> - BUG_ON(task_current_donor(rq, p));
>> - BUG_ON(p->nr_cpus_allowed <= 1);
>> -
>> - BUG_ON(!task_on_rq_queued(p));
>> - BUG_ON(!rt_task(p));
>> -
>> - return p;
>> -}
>> -
>> /*
>>  * If the current CPU has more than one RT task, see if the non
>>  * running task can migrate over to a CPU that is running a task

Powered by blists - more mailing lists