linux-kernel - Re: [RFC] rtmutex: Do not boost fair tasks each other

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.02.1405032041460.6261@ionos.tec.linutronix.de>
Date:	Sat, 3 May 2014 20:54:08 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Kirill Tkhai <tkhai@...dex.ru>
cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...hat.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Paul Gortmaker <paul.gortmaker@...driver.com>
Subject: Re: [RFC] rtmutex: Do not boost fair tasks each other

On Thu, 1 May 2014, Kirill Tkhai wrote:
> 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.
 
> 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).

Thanks,

	tglx

> Kirill
> ---
>  kernel/locking/rtmutex.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/locking/rtmutex.c b/kernel/locking/rtmutex.c
> index aa4dff0..609a57e 100644
> --- a/kernel/locking/rtmutex.c
> +++ b/kernel/locking/rtmutex.c
> @@ -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)
> @@ -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;
>  }
> 
>  /*
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/