Re: [PATCH] sched/rt: Plug rt_mutex_setprio() vs push_rt_task() race

[Valentin Schneider]

On 24/01/22 10:37, Dietmar Eggemann wrote:
> On 20/01/2022 20:40, Valentin Schneider wrote:
>
> [...]
>
>> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
>> index 7b4f4fbbb404..48fc8c04b038 100644
>> --- a/kernel/sched/rt.c
>> +++ b/kernel/sched/rt.c
>> @@ -2026,6 +2026,16 @@ static int push_rt_task(struct rq *rq, bool pull)
>>  		return 0;
>>  
>>  retry:
>> +	/*
>> +	 * It's possible that the next_task slipped in of
>> +	 * higher priority than current. If that's the case
>> +	 * just reschedule current.
>> +	 */
>> +	if (unlikely(next_task->prio < rq->curr->prio)) {
>> +		resched_curr(rq);
>> +		return 0;
>> +	}
>
> If we do this before `is_migration_disabled(next_task), shouldn't then
> the related condition in push_dl_task() also be moved up?
>
>   if (dl_task(rq->curr) &&
>     dl_time_before(next_task->dl.deadline, rq->curr->dl.deadline) &&
>     rq->curr->nr_cpus_allowed > 1)
>
> To enforce resched_curr(rq) in the `is_migration_disabled(next_task)`
> case there as well?
>

I'm not sure if we can hit the same issue with DL since DL doesn't have the
push irqwork. If there are DL tasks on the rq when current gets demoted,
switched_from_dl() won't queue pull_dl_task().

That said, if say we have DL tasks on the rq and demote the current DL task
to RT, do we currently have anything that will call resched_curr() (I'm
looking at the rt_mutex path)?
switched_to_fair() has a resched_curr() (which helps for the RT -> CFS
case), I don't see anything that would give us that in switched_from_dl() /
switched_to_rt(), or am I missing something?

>> +
>>  	if (is_migration_disabled(next_task)) {
>>  		struct task_struct *push_task = NULL;
>>  		int cpu;
>> @@ -2033,6 +2043,17 @@ static int push_rt_task(struct rq *rq, bool pull)
>>  		if (!pull || rq->push_busy)
>>  			return 0;
>>  
>> +		/*
>> +		 * Per the above priority check, curr is at least RT. If it's
>> +		 * of a higher class than RT, invoking find_lowest_rq() on it
>> +		 * doesn't make sense.
>> +		 *
>> +		 * Note that the stoppers are masqueraded as SCHED_FIFO
>> +		 * (cf. sched_set_stop_task()), so we can't rely on rt_task().
>> +		 */
>> +		if (rq->curr->sched_class != &rt_sched_class)
>
> s/ != / > / ... since the `unlikely(next_task->prio < rq->curr->prio)`
> already filters tasks from lower sched classes (CFS)?
>

!= points out we won't invoke find_lowest_rq() on anything that isn't RT,
which makes it a bit clearer IMO, and it's not like either of those
comparisons is more expensive than the other :)

>> +			return 0;
>> +
>
> [...]