Re: [patch 2/3] rtmutex: Propagate priority settings into PI lock chains

[Esben Nielsen <nielsen.esben@googlemail.com>]

On Thu, 22 Jun 2006, Steven Rostedt wrote:

>
> I've stated these comments on the -rt thread, but it's more important to
> repeat them here.
>
> On Thu, 22 Jun 2006, Thomas Gleixner wrote:
>
>>  /*
>> + * Recheck the pi chain, in case we got a priority setting
>> + *
>> + * Called from sched_setscheduler
>> + */
>> +void rt_mutex_adjust_pi(struct task_struct *task)
>> +{
>> +	struct rt_mutex_waiter *waiter;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&task->pi_lock, flags);
>> +
>> +	waiter = task->pi_blocked_on;
>
> Good to see you fixed the waiter race that I mentioned in the other
> thread.  You did it before I mentioned it, but I didn't read this yet ;)
>
>> +	if (!waiter || waiter->list_entry.prio == task->prio) {
>> +		spin_unlock_irqrestore(&task->pi_lock, flags);
>> +		return;
>> +	}
>> +
>> +	/* gets dropped in rt_mutex_adjust_prio_chain()! */
>> +	get_task_struct(task);
>> +	spin_unlock_irqrestore(&task->pi_lock, flags);
>> +
>> +	rt_mutex_adjust_prio_chain(task, 0, NULL, NULL, task);
>
> The above means that you cant ever call sched_setscheduler from a
> interrupt handler (or softirq).  The rt_mutex_adjust_prio_chain since that
> grabs wait_lock which is not for interrupt use.

Worse in RT context: It makes it unhealthy to call from a RT task as it 
doesn't have predictable runtime unless you know that the target task is 
not blocked on a deep locking tree.

I know this is very unlikely to happen very often in real life and this 
thread isn't about preempt-realtime, but I'll say it anyway: Hard realtime 
is about avoiding surprisingly long execution times - especially those 
which are  extremely unlikely to happen, but nevertheless are possible, 
because you are not very likely to see those situations in any tests, and 
therefore you can suddenly miss deadlines in the field without a clue what 
is happening.

Esben

>
>> +}
>> +
>> +/*
>>   * Slow path lock function:
>>   */
>>  static int __sched
>> @@ -633,6 +663,7 @@
>>  			if (unlikely(ret))
>>  				break;
>>  		}
>> +
>>  		spin_unlock(&lock->wait_lock);
>>
>>  		debug_rt_mutex_print_deadlock(&waiter);
>
> [...]
>
>>
>>  extern void set_user_nice(task_t *p, long nice);
>> Index: linux-2.6.17-mm/kernel/sched.c
>> ===================================================================
>> --- linux-2.6.17-mm.orig/kernel/sched.c	2006-06-22 10:26:11.000000000 +0200
>> +++ linux-2.6.17-mm/kernel/sched.c	2006-06-22 10:26:11.000000000 +0200
>
> Oh and Thomas...
>
>  export QUILT_DIFF_OPTS='-p'
>
>> @@ -4119,6 +4119,8 @@
>
> Can sched_setscheduler be called from interrupt context?
>
>>  	__task_rq_unlock(rq);
>>  	spin_unlock_irqrestore(&p->pi_lock, flags);
>>
>> +	rt_mutex_adjust_pi(p);
>> +
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(sched_setscheduler);
>
> I haven't found any place that this was called from interrupt context, but
> with this added, it can not be.  So it should be documented that
> sched_setscheduler grabs locks that are not to be called from interrupt
> context.
>
> -- Steve
>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
>
> Index: linux-2.6.17-rc4-mm1/kernel/sched.c
> ===================================================================
> --- linux-2.6.17-rc4-mm1.orig/kernel/sched.c	2006-06-22 10:13:50.000000000 -0400
> +++ linux-2.6.17-rc4-mm1/kernel/sched.c	2006-06-22 10:15:09.000000000 -0400
> @@ -4006,6 +4006,10 @@ static void __setscheduler(struct task_s
>  * @p: the task in question.
>  * @policy: new policy.
>  * @param: structure containing the new RT priority.
> + *
> + *  Do not call this from interrupt context.  If RT_MUTEXES is configured
> + *  then it can grab spin locks that are not protected with interrupts
> + *  disabled.
>  */
> int sched_setscheduler(struct task_struct *p, int policy,
> 		       struct sched_param *param)
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>