Re: [PATCH] Force processes to non-realtime before mm_exit

Re: [PATCH] Force processes to non-realtime before mm_exit

[Brian Silverman]

Sebastian had some questions about this patch when I first sent it to rt-users.

On Wed, May 25, 2016 at 12:54 PM, Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
> On 05/25/2016 08:00 PM, Brian Silverman wrote:
>>> Why can't the application drop the RT priority before its exit? Wouldn't
>>> that be appropriate?
>>
>> If it crashes or gets killed, it doesn't have a chance to drop priority.
>
> That is correct. The task with the highest priority is usually one of
> the most important ones. Usually if that task crashes or gets killed by
> the OOM killer while in production you have usually bigger problems
> than this.

I sometimes use high priority for low latency rather than because a
process is important. Processes like that are designed to always run
quickly, but they need to run with low latency, so they're high
priority. However, they should never cause high latencies for other
processes, even if they crash.

Also, when something bad does happen and an important process crashes,
it's nice to have it not cause problems for other processes too.
Dumping core or freeing memory pages is never more important than
continuing to run the rest of the system.

> I'm neither pro nor against this patch. This patch can go actually
> upstream if accepted since it is not RT specific. If you have a good
> use case please submit please post it upstream and CC me. Once accepted
> I would pull it in -RT as well.

Done

Brian

Re: [PATCH] Force processes to non-realtime before mm_exit

[Thomas Gleixner]

On Thu, 14 Jul 2016, Peter Zijlstra wrote:
> On Fri, Jun 03, 2016 at 04:18:44PM -0700, Brian Silverman wrote:
> > Without this, a realtime process which has called mlockall exiting
> > causes large latencies for other realtime processes at the same or
> > lower priorities. This seems like a fairly common use case too, because
> > realtime processes generally want their memory locked into RAM.
> 
> So I'm not too sure..  SCHED_FIFO/RR are a complete trainwreck and
> provide absolutely no isolation from badly behaving tasks what so ever,
> so I'm not too inclined to protect them from exit either, its just one
> more way in which they can cause pain.
> 
> But aside from the, the patch has issues..
> 
> > +++ b/kernel/exit.c
> > @@ -730,6 +730,12 @@ void do_exit(long code)
> >  	tsk->exit_code = code;
> >  	taskstats_exit(tsk, group_dead);
> >  
> > +	if (tsk->policy == SCHED_FIFO || tsk->policy == SCHED_RR) {
> > +		struct sched_param param = { .sched_priority = 0 };
> > +
> > +		sched_setscheduler_nocheck(current, SCHED_NORMAL, &param);
> > +	}
> > +
> >  	exit_mm(tsk);
> 
> That only does half a job. You forget about SCHED_DEADLINE and negative
> nice tasks.
> 
> Something like the below perhaps... But yeah, unconvinced.

I agree that FIFO/RR can cause pain, but running exit_mm() with RT priority
or consuming DL time is silly.
 
FWIW: Acked-by-me

Re: [PATCH] Force processes to non-realtime before mm_exit

[Peter Zijlstra]

On Fri, Jun 03, 2016 at 04:18:44PM -0700, Brian Silverman wrote:
> Without this, a realtime process which has called mlockall exiting
> causes large latencies for other realtime processes at the same or
> lower priorities. This seems like a fairly common use case too, because
> realtime processes generally want their memory locked into RAM.

So I'm not too sure..  SCHED_FIFO/RR are a complete trainwreck and
provide absolutely no isolation from badly behaving tasks what so ever,
so I'm not too inclined to protect them from exit either, its just one
more way in which they can cause pain.

But aside from the, the patch has issues..

> +++ b/kernel/exit.c
> @@ -730,6 +730,12 @@ void do_exit(long code)
>  	tsk->exit_code = code;
>  	taskstats_exit(tsk, group_dead);
>  
> +	if (tsk->policy == SCHED_FIFO || tsk->policy == SCHED_RR) {
> +		struct sched_param param = { .sched_priority = 0 };
> +
> +		sched_setscheduler_nocheck(current, SCHED_NORMAL, &param);
> +	}
> +
>  	exit_mm(tsk);

That only does half a job. You forget about SCHED_DEADLINE and negative
nice tasks.

Something like the below perhaps... But yeah, unconvinced.


diff --git a/kernel/exit.c b/kernel/exit.c
index 84ae830234f8..25da16bcfb9b 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -812,6 +812,13 @@ void do_exit(long code)
 	tsk->exit_code = code;
 	taskstats_exit(tsk, group_dead);
 
+	/*
+	 * Drop all scheduler priority before exit_mm() as that
+	 * can involve a lot of work and we don't want a dying
+	 * task to interfere with healthy/running tasks.
+	 */
+	sched_normalize_task(tsk);
+
 	exit_mm(tsk);
 
 	if (group_dead)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index d7f5376cfaac..14e1945c62e8 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7585,14 +7585,29 @@ void ___might_sleep(const char *file, int line, int preempt_offset)
 EXPORT_SYMBOL(___might_sleep);
 #endif
 
-#ifdef CONFIG_MAGIC_SYSRQ
-void normalize_rt_tasks(void)
+void sched_normalize_task(struct task_struct *p)
 {
-	struct task_struct *g, *p;
 	struct sched_attr attr = {
 		.sched_policy = SCHED_NORMAL,
 	};
 
+	if (!dl_task(p) && !rt_task(p)) {
+		/*
+		 * Renice negative nice level tasks.
+		 */
+		if (task_nice(p) < 0)
+			set_user_nice(p, 0);
+		return;
+	}
+
+	__sched_setscheduler(p, &attr, false, false);
+}
+
+#ifdef CONFIG_MAGIC_SYSRQ
+void normalize_rt_tasks(void)
+{
+	struct task_struct *g, *p;
+
 	read_lock(&tasklist_lock);
 	for_each_process_thread(g, p) {
 		/*
@@ -7608,21 +7623,10 @@ void normalize_rt_tasks(void)
 		p->se.statistics.block_start	= 0;
 #endif
 
-		if (!dl_task(p) && !rt_task(p)) {
-			/*
-			 * Renice negative nice level userspace
-			 * tasks back to 0:
-			 */
-			if (task_nice(p) < 0)
-				set_user_nice(p, 0);
-			continue;
-		}
-
-		__sched_setscheduler(p, &attr, false, false);
+		sched_normalize_task(p);
 	}
 	read_unlock(&tasklist_lock);
 }
-
 #endif /* CONFIG_MAGIC_SYSRQ */
 
 #if defined(CONFIG_IA64) || defined(CONFIG_KGDB_KDB)

Re: [PATCH] Force processes to non-realtime before mm_exit

[Corey Minyard]

On 06/03/2016 06:18 PM, Brian Silverman wrote:
> Without this, a realtime process which has called mlockall exiting
> causes large latencies for other realtime processes at the same or
> lower priorities. This seems like a fairly common use case too, because
> realtime processes generally want their memory locked into RAM.

Could this cause a subtle priority inversion for a process waiting
on this process to die?  I'm thinking that if this is a critical process,
it crashes, and the system is very busy with other RT processes,
it could take a long time before the process gets restarted when
it is expected to happen quickly.

I don't have another solution for you, and beyond speeding up the
memory reclamation process (which may not be possible or easy)
I'm not sure there is.  I'm just pointing out a possible side effect.

-corey

> Signed-off-by: Brian Silverman <brian@peloton-tech.com>
> ---
>   kernel/exit.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/kernel/exit.c b/kernel/exit.c
> index a0cf72b..68a97df 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -730,6 +730,12 @@ void do_exit(long code)
>   	tsk->exit_code = code;
>   	taskstats_exit(tsk, group_dead);
>   
> +	if (tsk->policy == SCHED_FIFO || tsk->policy == SCHED_RR) {
> +		struct sched_param param = { .sched_priority = 0 };
> +
> +		sched_setscheduler_nocheck(current, SCHED_NORMAL, &param);
> +	}
> +
>   	exit_mm(tsk);
>   
>   	if (group_dead)

[PATCH] Force processes to non-realtime before mm_exit

[Brian Silverman]

Without this, a realtime process which has called mlockall exiting
causes large latencies for other realtime processes at the same or
lower priorities. This seems like a fairly common use case too, because
realtime processes generally want their memory locked into RAM.

Signed-off-by: Brian Silverman <brian@peloton-tech.com>
---
 kernel/exit.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/exit.c b/kernel/exit.c
index a0cf72b..68a97df 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -730,6 +730,12 @@ void do_exit(long code)
 	tsk->exit_code = code;
 	taskstats_exit(tsk, group_dead);
 
+	if (tsk->policy == SCHED_FIFO || tsk->policy == SCHED_RR) {
+		struct sched_param param = { .sched_priority = 0 };
+
+		sched_setscheduler_nocheck(current, SCHED_NORMAL, &param);
+	}
+
 	exit_mm(tsk);
 
 	if (group_dead)
-- 
2.1.4