rtmutex: Do not boost fair tasks each other

Higher priority does not provide exclusive privilege of one fair task over the other. In this case priority boosting looks excess.

On RT patch with enabled PREEMPT_RT_FULL I see a lot of rt_mutex_setprio() actions like

	120 -> 118
	118 -> 120

They harm RT tasks.

RT patch has lazy preemtion feature, so if idea is we care about excess preemption inside fair class, we should care about excess priority inheritance too.

In case of vanila kernel the problem is the same, but there are no so many rt mutexes. Do I skip anything?

Kirill

---
 kernel/locking/rtmutex.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

kernel/locking/rtmutex.c +7 -4

index aa4dff0..609a57e 100644
@@ -197,11 +197,14 @@ rt_mutex_dequeue_pi(struct task_struct *task,
 struct rt_mutex_waiter *waiter)
  */
 int rt_mutex_getprio(struct task_struct *task)
 {
-	if (likely(!task_has_pi_waiters(task)))
-		return task->normal_prio;
+	if (unlikely(task_has_pi_waiters(task))) {
+		int prio = task_top_pi_waiter(task)->prio;
+
+		if (rt_prio(prio) || dl_prio(prio))
+			return min(prio, task->normal_prio);
+	}
 
-	return min(task_top_pi_waiter(task)->prio,
-		   task->normal_prio);
+	return task->normal_prio;
 }
 

struct task_struct *rt_mutex_get_top_task(struct task_struct *task)

kernel/locking/rtmutex.c +7 -3

@@ -218,10 +221,14 @@ struct task_struct *rt_mutex_get_top_task(struct
 task_struct *task)
  */
 int rt_mutex_check_prio(struct task_struct *task, int newprio)
 {
-	if (!task_has_pi_waiters(task))
-		return 0;
+	if (unlikely(task_has_pi_waiters(task))) {
+		int prio = task_top_pi_waiter(task)->task->prio;
 
-	return task_top_pi_waiter(task)->task->prio <= newprio;
+		if (rt_prio(prio) || dl_prio(prio))
+			return prio <= newprio;
+	}
+
+	return 0;
 }
 

/*

Quoted 11 lines · Kirill Tkhai

Higher priority does not provide exclusive privilege of one fair task over the other. In this case priority boosting looks excess.

On RT patch with enabled PREEMPT_RT_FULL I see a lot of rt_mutex_setprio() actions like

	120 -> 118
	118 -> 120

They harm RT tasks.

That's not the main problem. The point is that it is useless and therefor harming performace and throughput as well.

Quoted 6 lines

RT patch has lazy preemtion feature, so if idea is we care about excess preemption inside fair class, we should care about excess priority inheritance too.

In case of vanila kernel the problem is the same, but there are no so many rt mutexes. Do I skip anything?

Almost a decade ago we decided to do the boosting for everything including SCHED_OTHER due to the very simple reason that exercising that code path more is likely to trigger more bugs.

But yes in a production environment, it's pointless for SCHED_OTHER tasks.

Though exercising that code path as much as we can is not a bad thing either. So I'd like to see that made compile time conditional on one of the lock testing CONFIG items.

And the patch should be made against mainline, where we have the same issue (reduced to PI-futexes).

Quoted 34 lines · Thomas Gleixner

Quoted 11 lines · Kirill Tkhai

Higher priority does not provide exclusive privilege of one fair task over the other. In this case priority boosting looks excess.

On RT patch with enabled PREEMPT_RT_FULL I see a lot of rt_mutex_setprio() actions like

	120 -> 118
	118 -> 120

They harm RT tasks.

That's not the main problem. The point is that it is useless and therefor harming performace and throughput as well.

Quoted 6 lines

RT patch has lazy preemtion feature, so if idea is we care about excess preemption inside fair class, we should care about excess priority inheritance too.

In case of vanila kernel the problem is the same, but there are no so many rt mutexes. Do I skip anything?

Almost a decade ago we decided to do the boosting for everything including SCHED_OTHER due to the very simple reason that exercising that code path more is likely to trigger more bugs.

But yes in a production environment, it's pointless for SCHED_OTHER tasks.

Though exercising that code path as much as we can is not a bad thing either. So I'd like to see that made compile time conditional on one of the lock testing CONFIG items.

Quoted 2 lines

And the patch should be made against mainline, where we have the same issue (reduced to PI-futexes).

And of course, if we ever get to PEP, we very much want all the classes to participate :)

Quoted 11 lines

Higher priority does not provide exclusive privilege of one fair task over the other. In this case priority boosting looks excess.

On RT patch with enabled PREEMPT_RT_FULL I see a lot of rt_mutex_setprio() actions like

	120 -> 118
	118 -> 120

They harm RT tasks.

That's not the main problem. The point is that it is useless and therefor harming performace and throughput as well.

Quoted 6 lines

RT patch has lazy preemtion feature, so if idea is we care about excess preemption inside fair class, we should care about excess priority inheritance too.

In case of vanila kernel the problem is the same, but there are no so many rt mutexes. Do I skip anything?

Almost a decade ago we decided to do the boosting for everything including SCHED_OTHER due to the very simple reason that exercising that code path more is likely to trigger more bugs.

But yes in a production environment, it's pointless for SCHED_OTHER tasks.

Though exercising that code path as much as we can is not a bad thing either. So I'd like to see that made compile time conditional on one of the lock testing CONFIG items.

And the patch should be made against mainline, where we have the same issue (reduced to PI-futexes).

В Сб, 03/05/2014 в 20:54 +0200, Thomas Gleixner пишет:

Quoted 3 lines

Though exercising that code path as much as we can is not a bad thing either. So I'd like to see that made compile time conditional on one of the lock testing CONFIG items.

+#ifndef CONFIG_RT_MUTEX_BOOST_ALL

+#define heritable_prio(prio) (rt_prio(prio) || dl_prio(prio))

Quoted 5 lines

В Сб, 03/05/2014 в 20:54 +0200, Thomas Gleixner пишет:

Quoted 3 lines

Though exercising that code path as much as we can is not a bad thing either. So I'd like to see that made compile time conditional on one of the lock testing CONFIG items.

Quoted 1 line

+#ifndef CONFIG_RT_MUTEX_BOOST_ALL

No, not another pointless config option. Read what I said. What's wrong with using an existing config item, e.g DEBUG_RT_MUTEXES?

Quoted 1 line

+#define heritable_prio(prio) (rt_prio(prio) || dl_prio(prio))

inheritable please. It's not priority heritance and never will be.

В Ср, 28/05/2014 в 22:26 +0200, Thomas Gleixner пишет:

Quoted 10 lines · Kirill Tkhai

Quoted 5 lines

 В Сб, 03/05/2014 в 20:54 +0200, Thomas Gleixner пишет:

Quoted 3 lines

 Though exercising that code path as much as we can is not a bad thing either. So I'd like to see that made compile time conditional on one of the lock testing CONFIG items.

 +#ifndef CONFIG_RT_MUTEX_BOOST_ALL

 No, not another pointless config option. Read what I said. What's wrong with using an existing config item, e.g DEBUG_RT_MUTEXES?

Quoted 1 line

 +#define heritable_prio(prio) (rt_prio(prio) || dl_prio(prio))

 inheritable please. It's not priority heritance and never will be.

Thanks for comments. Here is new version.

[PATCH] rtmutex: Do not boost owner's prio if waiter is SCHED_OTHER

Higher priority does not provide exclusive privilege of one fair class task over the other. In this case priority boosting is pointless, and it may worsen performance.

This patch makes boosting, which is requested by fair class waiters, optional. It's disabled by default, but it's possible to enable it for debugging purposes to have more cases of priority inheritance.

Signed-off-by Kirill Tkhai <[email protected]>

 kernel/locking/rtmutex.c | 28 +++++++++++++++++++++-------

 1 file changed, 21 insertions(+), 7 deletions(-)