[PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd

[PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd

[Christoph Lameter]

This is a follow on patch related to the earlier
discussion about restricting the
spawning of kernel threads. See https://lkml.org/lkml/2013/9/5/426




usermodehelper() threads can currently run on all processors.
This is an issue for low latency cores.

Restrict usermodehelper() threads to the cores that kthreadd is
restricted to.

The default for kthreadd is to be allowed to run on an processors.
If the user restricts kthreadd then threads spawned by
usermodhelper() will similarly restricted.

Before this patch there is no way to limit the cpus that usermodehelper
can run on since the affinity is set when the thread is spawned to
all processors.

Signed-off-by: Christoph Lameter <cl@linux.com>

Index: linux/include/linux/kthread.h
===================================================================
--- linux.orig/include/linux/kthread.h	2013-10-10 11:00:34.167771996 -0500
+++ linux/include/linux/kthread.h	2013-10-15 13:57:52.859056676 -0500
@@ -51,6 +51,7 @@ void kthread_parkme(void);
 int kthreadd(void *unused);
 extern struct task_struct *kthreadd_task;
 extern int tsk_fork_get_node(struct task_struct *tsk);
+void set_kthreadd_affinity(void);

 /*
  * Simple work processor based on kthread.
Index: linux/kernel/kmod.c
===================================================================
--- linux.orig/kernel/kmod.c	2013-10-10 11:00:39.091771917 -0500
+++ linux/kernel/kmod.c	2013-10-15 14:02:01.904261324 -0500
@@ -40,6 +40,7 @@
 #include <linux/ptrace.h>
 #include <linux/async.h>
 #include <asm/uaccess.h>
+#include <linux/kthread.h>

 #include <trace/events/module.h>

@@ -209,8 +210,8 @@ static int ____call_usermodehelper(void
 	flush_signal_handlers(current, 1);
 	spin_unlock_irq(&current->sighand->siglock);

-	/* We can run anywhere, unlike our parent keventd(). */
-	set_cpus_allowed_ptr(current, cpu_all_mask);
+	/* We can run anywhere kthreadd can run */
+	set_kthreadd_affinity();

 	/*
 	 * Our parent is keventd, which runs with elevated scheduling priority.
Index: linux/kernel/kthread.c
===================================================================
--- linux.orig/kernel/kthread.c	2013-10-10 11:00:39.139771916 -0500
+++ linux/kernel/kthread.c	2013-10-15 14:04:36.636043744 -0500
@@ -136,6 +136,15 @@ void *kthread_data(struct task_struct *t
 	return to_kthread(task)->data;
 }

+/*
+ * Set the affinity of the calling task to be the same
+ * as the kthreadd affinities.
+ */
+void set_kthreadd_affinity(void)
+{
+	set_cpus_allowed(current, kthreadd_task->cpus_allowed);
+}
+
 /**
  * probe_kthread_data - speculative version of kthread_data()
  * @task: possible kthread task in question

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd

[Andrew Morton]

On Wed, 16 Oct 2013 14:44:28 +0000 Christoph Lameter <cl@linux.com> wrote:

> This is a follow on patch related to the earlier
> discussion about restricting the
> spawning of kernel threads. See https://lkml.org/lkml/2013/9/5/426
> 
> 
> 
> 
> usermodehelper() threads can currently run on all processors.
> This is an issue for low latency cores.

How much of an issue?  The severity of the problem is utterly unclear
from this description.

> Restrict usermodehelper() threads to the cores that kthreadd is
> restricted to.
> 
> The default for kthreadd is to be allowed to run on an processors.
> If the user restricts kthreadd then threads spawned by
> usermodhelper() will similarly restricted.
> 
> Before this patch there is no way to limit the cpus that usermodehelper
> can run on since the affinity is set when the thread is spawned to
> all processors.
> 
> ...
>
> --- linux.orig/include/linux/kthread.h	2013-10-10 11:00:34.167771996 -0500
> +++ linux/include/linux/kthread.h	2013-10-15 13:57:52.859056676 -0500
> @@ -51,6 +51,7 @@ void kthread_parkme(void);
>  int kthreadd(void *unused);
>  extern struct task_struct *kthreadd_task;
>  extern int tsk_fork_get_node(struct task_struct *tsk);
> +void set_kthreadd_affinity(void);
> 
>  /*
>   * Simple work processor based on kthread.
> Index: linux/kernel/kmod.c
> ===================================================================
> --- linux.orig/kernel/kmod.c	2013-10-10 11:00:39.091771917 -0500
> +++ linux/kernel/kmod.c	2013-10-15 14:02:01.904261324 -0500
> @@ -40,6 +40,7 @@
>  #include <linux/ptrace.h>
>  #include <linux/async.h>
>  #include <asm/uaccess.h>
> +#include <linux/kthread.h>
> 
>  #include <trace/events/module.h>
> 
> @@ -209,8 +210,8 @@ static int ____call_usermodehelper(void
>  	flush_signal_handlers(current, 1);
>  	spin_unlock_irq(&current->sighand->siglock);
> 
> -	/* We can run anywhere, unlike our parent keventd(). */
> -	set_cpus_allowed_ptr(current, cpu_all_mask);
> +	/* We can run anywhere kthreadd can run */

This is a poor comment - it explains "what" (which was utterly obvious)
but doesn't explain "why".  The reader will want to know *why*
call_usermodehelper() only runs on kthreadd CPUs, but we didn't tell
him.

> +	set_kthreadd_affinity();
> 

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd

[Christoph Lameter]

On Wed, 16 Oct 2013, Andrew Morton wrote:

> > usermodehelper() threads can currently run on all processors.
> > This is an issue for low latency cores.
>
> How much of an issue?  The severity of the problem is utterly unclear
> from this description.

This causes a context switch and processing to occur. Can take up to
microseconds which is a pretty long timeframe when decisions have to be
made in microseconds by active processing. This will cause a trade to
fail.

> > -	/* We can run anywhere, unlike our parent keventd(). */
> > -	set_cpus_allowed_ptr(current, cpu_all_mask);
> > +	/* We can run anywhere kthreadd can run */
>
> This is a poor comment - it explains "what" (which was utterly obvious)
> but doesn't explain "why".  The reader will want to know *why*
> call_usermodehelper() only runs on kthreadd CPUs, but we didn't tell
> him.

We'd like to have the ability to avoid running usermodehelper on certain
cpus to avoid cpu holdoff situations? Would that we an acceptable
explanation?

Or restricting kthreadd will also restrict usermodehelper spawning to
allow control for all spawned kernel threads?

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd

[Andrew Morton]

On Wed, 16 Oct 2013 22:37:52 +0000 Christoph Lameter <cl@linux.com> wrote:

> > > -	/* We can run anywhere, unlike our parent keventd(). */
> > > -	set_cpus_allowed_ptr(current, cpu_all_mask);
> > > +	/* We can run anywhere kthreadd can run */
> >
> > This is a poor comment - it explains "what" (which was utterly obvious)
> > but doesn't explain "why".  The reader will want to know *why*
> > call_usermodehelper() only runs on kthreadd CPUs, but we didn't tell
> > him.
> 
> We'd like to have the ability to avoid running usermodehelper on certain
> cpus to avoid cpu holdoff situations? Would that we an acceptable
> explanation?
> 
> Or restricting kthreadd will also restrict usermodehelper spawning to
> allow control for all spawned kernel threads?

Both, please ;)

[PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd V2

[Christoph Lameter]

usermodehelper() threads can currently run on all processors.
This is an issue for low latency cores. Spawnig a new thread causes
cpu holdoffs in the range of hundreds of microseconds to a few
milliseconds. Not good for cores on which processes run that need
to react as fast as possible.

kthreadd threads can be restricted using taskset to a limited set
of processors. Then the kernel thread pool will not fork processes
on those anymore thereby protecting those processors from additional
latencies.

Make usermodehelper() threads obey the limitations that kthreadd is
restricted to. Kthreadd is not the parent of usermodehelper threads
so we need to explicitly get the allowed processors for kthreadd.

Before this patch there is no way to limit the cpus that usermodehelper
can run on since the affinity is set when the thread is spawned to
all processors.

Signed-off-by: Christoph Lameter <cl@linux.com>

Index: linux/include/linux/kthread.h
===================================================================
--- linux.orig/include/linux/kthread.h	2013-11-07 10:31:46.667807582 -0600
+++ linux/include/linux/kthread.h	2013-11-07 10:31:46.663807693 -0600
@@ -51,6 +51,7 @@ void kthread_parkme(void);
 int kthreadd(void *unused);
 extern struct task_struct *kthreadd_task;
 extern int tsk_fork_get_node(struct task_struct *tsk);
+void set_kthreadd_affinity(void);

 /*
  * Simple work processor based on kthread.
Index: linux/kernel/kmod.c
===================================================================
--- linux.orig/kernel/kmod.c	2013-11-07 10:31:46.667807582 -0600
+++ linux/kernel/kmod.c	2013-11-07 10:35:28.825645008 -0600
@@ -40,6 +40,7 @@
 #include <linux/ptrace.h>
 #include <linux/async.h>
 #include <asm/uaccess.h>
+#include <linux/kthread.h>

 #include <trace/events/module.h>

@@ -209,8 +210,13 @@ static int ____call_usermodehelper(void
 	flush_signal_handlers(current, 1);
 	spin_unlock_irq(&current->sighand->siglock);

-	/* We can run anywhere, unlike our parent keventd(). */
-	set_cpus_allowed_ptr(current, cpu_all_mask);
+	/*
+	 * Kthreadd can be restricted to a set of processors if the user wants to
+	 * protect other processors from OS latencies. If that has happened then
+	 * we do not want to disturb the other processors here either so we start
+	 * the usermode helper threads only on the processors allowed for kthreadd.
+	 */
+	set_kthreadd_affinity();

 	/*
 	 * Our parent is keventd, which runs with elevated scheduling priority.
Index: linux/kernel/kthread.c
===================================================================
--- linux.orig/kernel/kthread.c	2013-11-07 10:31:46.667807582 -0600
+++ linux/kernel/kthread.c	2013-11-07 10:31:46.667807582 -0600
@@ -136,6 +136,15 @@ void *kthread_data(struct task_struct *t
 	return to_kthread(task)->data;
 }

+/*
+ * Set the affinity of the calling task to be the same
+ * as the kthreadd affinities.
+ */
+void set_kthreadd_affinity(void)
+{
+	set_cpus_allowed(current, kthreadd_task->cpus_allowed);
+}
+
 /**
  * probe_kthread_data - speculative version of kthread_data()
  * @task: possible kthread task in question

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd V2

[Frederic Weisbecker]

On Thu, Nov 07, 2013 at 04:43:11PM +0000, Christoph Lameter wrote:
> usermodehelper() threads can currently run on all processors.
> This is an issue for low latency cores. Spawnig a new thread causes
> cpu holdoffs in the range of hundreds of microseconds to a few
> milliseconds. Not good for cores on which processes run that need
> to react as fast as possible.
> 
> kthreadd threads can be restricted using taskset to a limited set
> of processors. Then the kernel thread pool will not fork processes
> on those anymore thereby protecting those processors from additional
> latencies.
> 
> Make usermodehelper() threads obey the limitations that kthreadd is
> restricted to. Kthreadd is not the parent of usermodehelper threads
> so we need to explicitly get the allowed processors for kthreadd.
> 
> Before this patch there is no way to limit the cpus that usermodehelper
> can run on since the affinity is set when the thread is spawned to
> all processors.
> 
> Signed-off-by: Christoph Lameter <cl@linux.com>
> 
> Index: linux/include/linux/kthread.h
> ===================================================================
> --- linux.orig/include/linux/kthread.h	2013-11-07 10:31:46.667807582 -0600
> +++ linux/include/linux/kthread.h	2013-11-07 10:31:46.663807693 -0600
> @@ -51,6 +51,7 @@ void kthread_parkme(void);
>  int kthreadd(void *unused);
>  extern struct task_struct *kthreadd_task;
>  extern int tsk_fork_get_node(struct task_struct *tsk);
> +void set_kthreadd_affinity(void);
> 
>  /*
>   * Simple work processor based on kthread.
> Index: linux/kernel/kmod.c
> ===================================================================
> --- linux.orig/kernel/kmod.c	2013-11-07 10:31:46.667807582 -0600
> +++ linux/kernel/kmod.c	2013-11-07 10:35:28.825645008 -0600
> @@ -40,6 +40,7 @@
>  #include <linux/ptrace.h>
>  #include <linux/async.h>
>  #include <asm/uaccess.h>
> +#include <linux/kthread.h>
> 
>  #include <trace/events/module.h>
> 
> @@ -209,8 +210,13 @@ static int ____call_usermodehelper(void
>  	flush_signal_handlers(current, 1);
>  	spin_unlock_irq(&current->sighand->siglock);
> 
> -	/* We can run anywhere, unlike our parent keventd(). */
> -	set_cpus_allowed_ptr(current, cpu_all_mask);
> +	/*
> +	 * Kthreadd can be restricted to a set of processors if the user wants to
> +	 * protect other processors from OS latencies. If that has happened then
> +	 * we do not want to disturb the other processors here either so we start
> +	 * the usermode helper threads only on the processors allowed for kthreadd.
> +	 */
> +	set_kthreadd_affinity();

I'm sorry to burden again on this but this looks too odd.

usermodehelper works are created via workqueues, right? And workqueues are an issue as
well for those who want CPU isolation.

So this looks like a more general problem than just call_usermodehelper.

Last time I checked, it seemed to me that this is an unbound workqueue? If so can't we tune
the affinity of this workqueue? If not perhaps that's something we want to solve and which
could be useful for all the users who don't want their CPU to be disturbed.

Thanks.

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd V2

[Christoph Lameter]

On Thu, 7 Nov 2013, Frederic Weisbecker wrote:

> usermodehelper works are created via workqueues, right? And workqueues are an issue as
> well for those who want CPU isolation.

AFAICT usermodehelper can be called from a variety of contexts.

> So this looks like a more general problem than just call_usermodehelper.

Well the code explicitly sets the the affinity mask to all cpus which
creates a problem for low latency processors. It does not inherit the
affinity from any calling process.

> Last time I checked, it seemed to me that this is an unbound workqueue? If so can't we tune
> the affinity of this workqueue? If not perhaps that's something we want to solve and which
> could be useful for all the users who don't want their CPU to be disturbed.

There are various contexts from which usermodehelper can be called.
Drivers use it etc.

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd V2

[Frederic Weisbecker]

On Fri, Nov 08, 2013 at 03:06:59PM +0000, Christoph Lameter wrote:
> On Thu, 7 Nov 2013, Frederic Weisbecker wrote:
> 
> > usermodehelper works are created via workqueues, right? And workqueues are an issue as
> > well for those who want CPU isolation.
> 
> AFAICT usermodehelper can be called from a variety of contexts.

But it looks like it always end up calling a workqueue. May be I missed something though.

Now we can argue that this workqueue seem to create kernel threads, which in turn create other kernel thread (uhh?)
and I don't know if those inherit the kworker affinity. But from a quick look, it seems to me that
this is what we want.

BTW the use of kernel_thread is justified in this comment: "/* Keventd can't block, but this (a child) can. */"
Do these kernel_threads exist because kworker can't block? Actually the new workqueue subsystem can have blocking
worklets now, may be something can be simplified there although I haven't checked the details. But perhaps
we can spare one level of kernel threads.

Can't we use kthreads there btw? Or may be we need a task->mm to do the user things.

> 
> > So this looks like a more general problem than just call_usermodehelper.
> 
> Well the code explicitly sets the the affinity mask to all cpus which
> creates a problem for low latency processors. It does not inherit the
> affinity from any calling process.

But how is that an argument in favour of changing this to inheriting the affinity from
the workqueue thread rather than kthread?

> 
> > Last time I checked, it seemed to me that this is an unbound workqueue? If so can't we tune
> > the affinity of this workqueue? If not perhaps that's something we want to solve and which
> > could be useful for all the users who don't want their CPU to be disturbed.
> 
> There are various contexts from which usermodehelper can be called.
> Drivers use it etc.

But they all converge to the workqueue, or I'm missing other code paths?

Thanks.

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd V2

[Christoph Lameter]

On Fri, 8 Nov 2013, Frederic Weisbecker wrote:

> But it looks like it always end up calling a workqueue. May be I missed something though.
>
> Now we can argue that this workqueue seem to create kernel threads, which in turn create other kernel thread (uhh?)
> and I don't know if those inherit the kworker affinity. But from a quick look, it seems to me that
> this is what we want.

Right. The problem is that the affinity cannot be inherited since
usermodehelper may be called from a workqueue or other restricted kernel
context. The main point of the usermodehelper logic is to work itself out
of the restrictions of the context in which is was called to be able to
fork off a kernel thread that then can call a userspace helper program.

I want to restrict on which processors this working out of the limiting
context can occur. It should not occur on low latency processors nor
should user space stuff be run there.

Without this patch we see various processes being sprinkled over all the
processors in the system when usermodehelper is invoked.

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd V2

[Frederic Weisbecker]

On Fri, Nov 08, 2013 at 05:05:35PM +0000, Christoph Lameter wrote:
> On Fri, 8 Nov 2013, Frederic Weisbecker wrote:
> 
> > But it looks like it always end up calling a workqueue. May be I missed something though.
> >
> > Now we can argue that this workqueue seem to create kernel threads, which in turn create other kernel thread (uhh?)
> > and I don't know if those inherit the kworker affinity. But from a quick look, it seems to me that
> > this is what we want.
> 
> Right. The problem is that the affinity cannot be inherited since
> usermodehelper may be called from a workqueue or other restricted kernel
> context. The main point of the usermodehelper logic is to work itself out
> of the restrictions of the context in which is was called to be able to
> fork off a kernel thread that then can call a userspace helper program.

I understand, but why not solving that from the workqueue affinity? We want to
solve the issue of unbound workqueues in CPU isolation anyway.

> 
> I want to restrict on which processors this working out of the limiting
> context can occur. It should not occur on low latency processors nor
> should user space stuff be run there.
> 
> Without this patch we see various processes being sprinkled over all the
> processors in the system when usermodehelper is invoked.

I got that, and I too am interested in solving this problem. I'm not fighting
against the purpose of this patch but against the way we solve that.

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd V2

[Christoph Lameter]

On Fri, 8 Nov 2013, Frederic Weisbecker wrote:

> I understand, but why not solving that from the workqueue affinity? We want to
> solve the issue of unbound workqueues in CPU isolation anyway.

Sure if you can solve that with an unbound work queue then this patch is
not needed. Do you have a patch that addresses this issue in your
patchset? How far out is the merging of that patch?

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd V2

[Frederic Weisbecker]

On Fri, Nov 08, 2013 at 07:52:37PM +0000, Christoph Lameter wrote:
> On Fri, 8 Nov 2013, Frederic Weisbecker wrote:
> 
> > I understand, but why not solving that from the workqueue affinity? We want to
> > solve the issue of unbound workqueues in CPU isolation anyway.
> 
> Sure if you can solve that with an unbound work queue then this patch is
> not needed. Do you have a patch that addresses this issue in your
> patchset?

No. Sorry.

> How far out is the merging of that patch?

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd V2

[Andrew Morton]

On Fri, 8 Nov 2013 21:06:22 +0100 Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Fri, Nov 08, 2013 at 07:52:37PM +0000, Christoph Lameter wrote:
> > On Fri, 8 Nov 2013, Frederic Weisbecker wrote:
> > 
> > > I understand, but why not solving that from the workqueue affinity? We want to
> > > solve the issue of unbound workqueues in CPU isolation anyway.
> > 
> > Sure if you can solve that with an unbound work queue then this patch is
> > not needed. Do you have a patch that addresses this issue in your
> > patchset?
> 
> No. Sorry.
> 

Several months have passed and nothgin has happened.  Any thoughts on
what we should do with Christoph's patch?


From: Christoph Lameter <cl@linux.com>
Subject: kmod: run usermodehelpers only on cpus allowed for kthreadd V2

usermodehelper() threads can currently run on all processors.  This is an
issue for low latency cores.  Spawnig a new thread causes cpu holdoffs in
the range of hundreds of microseconds to a few milliseconds.  Not good for
cores on which processes run that need to react as fast as possible.

kthreadd threads can be restricted using taskset to a limited set of
processors.  Then the kernel thread pool will not fork processes on those
anymore thereby protecting those processors from additional latencies.

Make usermodehelper() threads obey the limitations that kthreadd is
restricted to.  Kthreadd is not the parent of usermodehelper threads so we
need to explicitly get the allowed processors for kthreadd.

Before this patch there is no way to limit the cpus that usermodehelper
can run on since the affinity is set when the thread is spawned to all
processors.

[akpm@linux-foundation.org: set_cpus_allowed() doesn't exist when CONFIG_CPUMASK_OFFSTACK=y]
[akpm@linux-foundation.org: coding-style fixes]
Signed-off-by: Christoph Lameter <cl@linux.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Mike Galbraith <bitbucket@online.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Gilad Ben-Yossef <gilad@benyossef.com>
Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 include/linux/kthread.h |    1 +
 kernel/kmod.c           |   11 +++++++++--
 kernel/kthread.c        |    9 +++++++++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff -puN include/linux/kthread.h~kmod-run-usermodehelpers-only-on-cpus-allowed-for-kthreadd-v2 include/linux/kthread.h
--- a/include/linux/kthread.h~kmod-run-usermodehelpers-only-on-cpus-allowed-for-kthreadd-v2
+++ a/include/linux/kthread.h
@@ -51,6 +51,7 @@ void kthread_parkme(void);
 int kthreadd(void *unused);
 extern struct task_struct *kthreadd_task;
 extern int tsk_fork_get_node(struct task_struct *tsk);
+void set_kthreadd_affinity(void);
 
 /*
  * Simple work processor based on kthread.
diff -puN kernel/kmod.c~kmod-run-usermodehelpers-only-on-cpus-allowed-for-kthreadd-v2 kernel/kmod.c
--- a/kernel/kmod.c~kmod-run-usermodehelpers-only-on-cpus-allowed-for-kthreadd-v2
+++ a/kernel/kmod.c
@@ -40,6 +40,7 @@
 #include <linux/ptrace.h>
 #include <linux/async.h>
 #include <asm/uaccess.h>
+#include <linux/kthread.h>
 
 #include <trace/events/module.h>
 
@@ -209,8 +210,14 @@ static int ____call_usermodehelper(void
 	flush_signal_handlers(current, 1);
 	spin_unlock_irq(&current->sighand->siglock);
 
-	/* We can run anywhere, unlike our parent keventd(). */
-	set_cpus_allowed_ptr(current, cpu_all_mask);
+	/*
+	 * Kthreadd can be restricted to a set of processors if the user wants
+	 * to protect other processors from OS latencies. If that has happened
+	 * then we do not want to disturb the other processors here either so we
+	 * start the usermode helper threads only on the processors allowed for
+	 * kthreadd.
+	 */
+	set_kthreadd_affinity();
 
 	/*
 	 * Our parent is keventd, which runs with elevated scheduling priority.
diff -puN kernel/kthread.c~kmod-run-usermodehelpers-only-on-cpus-allowed-for-kthreadd-v2 kernel/kthread.c
--- a/kernel/kthread.c~kmod-run-usermodehelpers-only-on-cpus-allowed-for-kthreadd-v2
+++ a/kernel/kthread.c
@@ -136,6 +136,15 @@ void *kthread_data(struct task_struct *t
 	return to_kthread(task)->data;
 }
 
+/*
+ * Set the affinity of the calling task to be the same
+ * as the kthreadd affinities.
+ */
+void set_kthreadd_affinity(void)
+{
+	set_cpus_allowed_ptr(current, &kthreadd_task->cpus_allowed);
+}
+
 /**
  * probe_kthread_data - speculative version of kthread_data()
  * @task: possible kthread task in question
_

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd V2

[Frederic Weisbecker]

On Tue, Apr 08, 2014 at 01:57:12PM -0700, Andrew Morton wrote:
> On Fri, 8 Nov 2013 21:06:22 +0100 Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > On Fri, Nov 08, 2013 at 07:52:37PM +0000, Christoph Lameter wrote:
> > > On Fri, 8 Nov 2013, Frederic Weisbecker wrote:
> > > 
> > > > I understand, but why not solving that from the workqueue affinity? We want to
> > > > solve the issue of unbound workqueues in CPU isolation anyway.
> > > 
> > > Sure if you can solve that with an unbound work queue then this patch is
> > > not needed. Do you have a patch that addresses this issue in your
> > > patchset?
> > 
> > No. Sorry.
> > 
> 
> Several months have passed and nothgin has happened.  Any thoughts on
> what we should do with Christoph's patch?

We have taken another direction to solve the initial problem which is
that usermodehelper threads disturb CPUs on some workloads requiring isolation
(ie: workloads that want specific CPUs to not be disturbed by kernel noise)

We eventually agreed that setting the WQ_SYSFS flag to the usermodehelper
workqueue would allow us to control the affinity of these jobs through sysfs.

But eventually I took it one step further by considering that the real problem
is much broader and concerns all unbound workqueues. Not just usermodehelper.
Christoph finds usermodehelper workqueue disturbing on his workloads, but other
CPU isolation users runnning other workloads on other hardware, other IO protocols, etc..
may be disturbed by many other kind of unbound workqueues. Because unbound
workqueues are globally affined and this behaviour can't be tweaked sanely yet.

And there are a big lot of them. Singlethread workqueues for example has more
than 200 users in the kernel:

      $ git grep create_singlethread_workqueue | wc -l
        219

It's actually something that we wanted to fix for a while now. So I'm
currently working on a more general solution that allow users to affine all
unbound workqueues to the desired set of CPUs.

The last version of the patchset is https://lkml.org/lkml/2014/3/27/246
After some discussion with Tejun and others, I'm working on the next iteration.

So I strongly think we should drop Christoph's patch and focus on the
unbound workqueues sysfs' affinity interface patchset.

Ah there is also the problem of this explicit call in kmod:

       "set_cpus_allowed_ptr(current, cpu_all_mask);"

But it looks like something we can drop. The comment above says:

       /* We can run anywhere, unlike our parent keventd(). */

Looks like a stale issue that concerned the old workqueue times when workqueues
really mapped to named thread. But even then I don't see the reason for this
affinity overwrite.

Thanks.

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd

[Frederic Weisbecker]

On Wed, Oct 16, 2013 at 02:44:28PM +0000, Christoph Lameter wrote:
> This is a follow on patch related to the earlier
> discussion about restricting the
> spawning of kernel threads. See https://lkml.org/lkml/2013/9/5/426
> 
> 
> 
> 
> usermodehelper() threads can currently run on all processors.
> This is an issue for low latency cores.

This might be redundant with akpm's reply but here we go:

low latency is a very confusing term nowadays. It's used by real time
to describe the time it takes for a high prio task to take the CPU,
and it's used by you for extreme HPC mode for deterministic undisturbed
throughput.

The changelog needs to be expanded to describe what's needed, or people
will be confused when they will stare at that git blame line in one year from now :)

> 
> Restrict usermodehelper() threads to the cores that kthreadd is
> restricted to.
> 
> The default for kthreadd is to be allowed to run on an processors.

s/an/all ?

> If the user restricts kthreadd then threads spawned by
> usermodhelper() will similarly restricted.
> 
> Before this patch there is no way to limit the cpus that usermodehelper
> can run on since the affinity is set when the thread is spawned to
> all processors.
> 
> Signed-off-by: Christoph Lameter <cl@linux.com>
> 
> Index: linux/include/linux/kthread.h
> ===================================================================
> --- linux.orig/include/linux/kthread.h	2013-10-10 11:00:34.167771996 -0500
> +++ linux/include/linux/kthread.h	2013-10-15 13:57:52.859056676 -0500
> @@ -51,6 +51,7 @@ void kthread_parkme(void);
>  int kthreadd(void *unused);
>  extern struct task_struct *kthreadd_task;
>  extern int tsk_fork_get_node(struct task_struct *tsk);
> +void set_kthreadd_affinity(void);
> 
>  /*
>   * Simple work processor based on kthread.
> Index: linux/kernel/kmod.c
> ===================================================================
> --- linux.orig/kernel/kmod.c	2013-10-10 11:00:39.091771917 -0500
> +++ linux/kernel/kmod.c	2013-10-15 14:02:01.904261324 -0500
> @@ -40,6 +40,7 @@
>  #include <linux/ptrace.h>
>  #include <linux/async.h>
>  #include <asm/uaccess.h>
> +#include <linux/kthread.h>
> 
>  #include <trace/events/module.h>
> 
> @@ -209,8 +210,8 @@ static int ____call_usermodehelper(void
>  	flush_signal_handlers(current, 1);
>  	spin_unlock_irq(&current->sighand->siglock);
> 
> -	/* We can run anywhere, unlike our parent keventd(). */
> -	set_cpus_allowed_ptr(current, cpu_all_mask);
> +	/* We can run anywhere kthreadd can run */
> +	set_kthreadd_affinity();

If you really want your isolated CPUs to be undisturbed, you need this affinity to
be set before the creation of this usermode helper. Otherwise it's going to run to any random
place before you call the above function.

Is there now way to control the kworker affinity which create this usermode helpers threads
instead?

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd

[Christoph Lameter]

On Thu, 17 Oct 2013, Frederic Weisbecker wrote:

> > -	/* We can run anywhere, unlike our parent keventd(). */
> > -	set_cpus_allowed_ptr(current, cpu_all_mask);
> > +	/* We can run anywhere kthreadd can run */
> > +	set_kthreadd_affinity();
>
> If you really want your isolated CPUs to be undisturbed, you need this affinity to
> be set before the creation of this usermode helper. Otherwise it's going to run to any random
> place before you call the above function.
>
> Is there now way to control the kworker affinity which create this usermode helpers threads
> instead?

The invocation is coming from keventd so the affinity seems to be set to
a different cpu before we get the thread moved.

A tsc loop is running on the processor that gets hit by the usermodehelper spawns.

We see spawning by the tty notifier (when typing on a vt) as well as by a
mellanox driver (periodically).

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd

[Frederic Weisbecker]

On Thu, Oct 17, 2013 at 03:24:36PM +0000, Christoph Lameter wrote:
> On Thu, 17 Oct 2013, Frederic Weisbecker wrote:
> 
> > > -	/* We can run anywhere, unlike our parent keventd(). */
> > > -	set_cpus_allowed_ptr(current, cpu_all_mask);
> > > +	/* We can run anywhere kthreadd can run */
> > > +	set_kthreadd_affinity();
> >
> > If you really want your isolated CPUs to be undisturbed, you need this affinity to
> > be set before the creation of this usermode helper. Otherwise it's going to run to any random
> > place before you call the above function.
> >
> > Is there now way to control the kworker affinity which create this usermode helpers threads
> > instead?
> 
> The invocation is coming from keventd so the affinity seems to be set to
> a different cpu before we get the thread moved.

Looking at alloc_singlethread_workqueue(), it seems to create a kthread by using
kthread_create(). So I guess the resulting works have the affinity of that kthread.
And thus call_usermodehelper() initially has that affinity, until it spreads it explicitly
to all CPUs.

Couldn't we instead make kthread children (those created with kthread_create()) to inherit
kthread initial affinity? Currently kthread's children have cpu_all_mask. We could change
that behaviour. This way the initial kthread affinity could be inherited all along.

> 
> A tsc loop is running on the processor that gets hit by the usermodehelper spawns.
> 
> We see spawning by the tty notifier (when typing on a vt) as well as by a
> mellanox driver (periodically).> 

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd

[Andrew Morton]

On Thu, 17 Oct 2013 18:07:28 +0200 Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Couldn't we instead make kthread children (those created with kthread_create()) to inherit
> kthread initial affinity? Currently kthread's children have cpu_all_mask. We could change
> that behaviour. This way the initial kthread affinity could be inherited all along.

I'm wondering if it's clean/logical to tie usermodehelper affinity to
kthreadd affinity at all.  It's certainly convenient, but they're
distinct concepts.  What is the reason to not have a separate control
for usermodehelper cpus-allowed?

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd

[Christoph Lameter]

On Thu, 17 Oct 2013, Andrew Morton wrote:

> On Thu, 17 Oct 2013 18:07:28 +0200 Frederic Weisbecker <fweisbec@gmail.com> wrote:
>
> > Couldn't we instead make kthread children (those created with kthread_create()) to inherit
> > kthread initial affinity? Currently kthread's children have cpu_all_mask. We could change
> > that behaviour. This way the initial kthread affinity could be inherited all along.
>
> I'm wondering if it's clean/logical to tie usermodehelper affinity to
> kthreadd affinity at all.  It's certainly convenient, but they're
> distinct concepts.  What is the reason to not have a separate control
> for usermodehelper cpus-allowed?

It was suggested in the earlier discussion. We could do it separately.

We have to limit kthreadd anyways and the idea was that the setting would
be used to limit any threads spawned that can be limited to a set of
processors.

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd

[Frederic Weisbecker]

On Thu, Oct 17, 2013 at 10:50:26AM -0700, Andrew Morton wrote:
> On Thu, 17 Oct 2013 18:07:28 +0200 Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > Couldn't we instead make kthread children (those created with kthread_create()) to inherit
> > kthread initial affinity? Currently kthread's children have cpu_all_mask. We could change
> > that behaviour. This way the initial kthread affinity could be inherited all along.
> 
> I'm wondering if it's clean/logical to tie usermodehelper affinity to
> kthreadd affinity at all.  It's certainly convenient, but they're
> distinct concepts.  What is the reason to not have a separate control
> for usermodehelper cpus-allowed?

Makes sense yeah. In fact what I'm mostly concerned about is that we should
set the affinity of __call_usermodehelper threads through inheritance from
a parent rather than making it setting its affinity itself. Because in the latter case,
the usermodehelper thread can run anywhere until it sets its affinity. Whether
this little window of global affinity is short or not, this defeats the initial purpose
of this patch that is about isolating CPUs and having them undisturbed.

May be we can do that by setting the affinity of the "khelper" workqueue?

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd

[Christoph Lameter]

On Fri, 18 Oct 2013, Frederic Weisbecker wrote:

> Makes sense yeah. In fact what I'm mostly concerned about is that we should
> set the affinity of __call_usermodehelper threads through inheritance from
> a parent rather than making it setting its affinity itself. Because in the latter case,
> the usermodehelper thread can run anywhere until it sets its affinity. Whether
> this little window of global affinity is short or not, this defeats the initial purpose
> of this patch that is about isolating CPUs and having them undisturbed.
>
> May be we can do that by setting the affinity of the "khelper" workqueue?

The "parent" for usemodehelper in this case is keventd. The worker queue
item is triggered on a particular processor and thats fine because that is
the result of an OS action or a device irq action. These can already be
avoided. What is not ok is that a process makes a move onto a hardware
thread where we want to have the least OS holdoffs possible.

Setting it via khelper would be fine. Any objections?

Re: [PATCH] kmod: Run usermodehelpers only on cpus allowed for kthreadd

[Christoph Lameter]

On Thu, 17 Oct 2013, Frederic Weisbecker wrote:

> > The invocation is coming from keventd so the affinity seems to be set to
> > a different cpu before we get the thread moved.
>
> Looking at alloc_singlethread_workqueue(), it seems to create a kthread by using
> kthread_create(). So I guess the resulting works have the affinity of that kthread.
> And thus call_usermodehelper() initially has that affinity, until it spreads it explicitly
> to all CPUs.
>
> Couldn't we instead make kthread children (those created with kthread_create()) to inherit
> kthread initial affinity? Currently kthread's children have cpu_all_mask. We could change
> that behaviour. This way the initial kthread affinity could be inherited all along.

Some of the code relies on being able to set the affinities separately.