[PATCH 0/3] kmod: Make nohz-full friendly + cleanups

[PATCH 0/3] kmod: Make nohz-full friendly + cleanups

[Frederic Weisbecker]

Usermodehelpers kernel threads are nohz-full unfriendly because they
force some explicit CPU-wide affinity that happen to disturb user code
that don't want to see noise on isolated CPUs.

So I spent many time on that, despite the one-liner end result.
The out-of-date comments didn't help much, especially it wasn't clear
why we need khelper at all, probably it was first introduced to bring
necessary asynchronous code but I couldn't justify that with the current
code.

I wrote patches to remove it but finally realized we still need it to
inherit a system wide (or nohz-full housekeeping) CPU affinity on
usermodehelper kernel threads because some usermodehelper callers may
have a reduced affinity (eg: per cpu workqueues). All I needed eventually
was to improve the comments to highlight that purpose. And remove a
nohz-full unfriendly wide affinity forcing.

git://git.kernel.org/pub/scm/linux/kernel/git/frederic/linux-dynticks.git
	nohz/kmod

HEAD: 9a0ad418b7739fbbe4add684551aa5582a58718b

Thanks,
	Frederic
---

Frederic Weisbecker (3):
      kmod: Bunch of internal functions renames
      kmod: Add up-to-date explanations on the purpose of each asynchronous levels
      kmod: Remove unecessary explicit wide CPU affinity setting


 kernel/kmod.c | 61 +++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 43 insertions(+), 18 deletions(-)

[PATCH 1/3] kmod: Bunch of internal functions renames

[Frederic Weisbecker]

Underscores on function names aren't much verbose to explain the
purpose of a function. And kmod has interesting such flavours.

Lets rename the following functions:

* __call_usermodehelper -> call_usermodehelper_exec_work
* ____call_usermodehelper -> call_usermodehelper_exec_async
* wait_for_helper -> call_usermodehelper_exec_sync

Cc: Rik van Riel <riel@redhat.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Lameter <cl@linux.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/kmod.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index 2777f40..4682e91 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -213,7 +213,7 @@ static void umh_complete(struct subprocess_info *sub_info)
 /*
  * This is the task which runs the usermode application
  */
-static int ____call_usermodehelper(void *data)
+static int call_usermodehelper_exec_async(void *data)
 {
 	struct subprocess_info *sub_info = data;
 	struct cred *new;
@@ -258,7 +258,10 @@ static int ____call_usermodehelper(void *data)
 			   (const char __user *const __user *)sub_info->envp);
 out:
 	sub_info->retval = retval;
-	/* wait_for_helper() will call umh_complete if UHM_WAIT_PROC. */
+	/*
+	 * call_usermodehelper_exec_sync() will call umh_complete
+	 * if UHM_WAIT_PROC.
+	 */
 	if (!(sub_info->wait & UMH_WAIT_PROC))
 		umh_complete(sub_info);
 	if (!retval)
@@ -267,14 +270,14 @@ out:
 }
 
 /* Keventd can't block, but this (a child) can. */
-static int wait_for_helper(void *data)
+static int call_usermodehelper_exec_sync(void *data)
 {
 	struct subprocess_info *sub_info = data;
 	pid_t pid;
 
 	/* If SIGCLD is ignored sys_wait4 won't populate the status. */
 	kernel_sigaction(SIGCHLD, SIG_DFL);
-	pid = kernel_thread(____call_usermodehelper, sub_info, SIGCHLD);
+	pid = kernel_thread(call_usermodehelper_exec_async, sub_info, SIGCHLD);
 	if (pid < 0) {
 		sub_info->retval = pid;
 	} else {
@@ -282,17 +285,18 @@ static int wait_for_helper(void *data)
 		/*
 		 * Normally it is bogus to call wait4() from in-kernel because
 		 * wait4() wants to write the exit code to a userspace address.
-		 * But wait_for_helper() always runs as keventd, and put_user()
-		 * to a kernel address works OK for kernel threads, due to their
-		 * having an mm_segment_t which spans the entire address space.
+		 * But call_usermodehelper_exec_sync() always runs as keventd,
+		 * and put_user() to a kernel address works OK for kernel
+		 * threads, due to their having an mm_segment_t which spans the
+		 * entire address space.
 		 *
 		 * Thus the __user pointer cast is valid here.
 		 */
 		sys_wait4(pid, (int __user *)&ret, 0, NULL);
 
 		/*
-		 * If ret is 0, either ____call_usermodehelper failed and the
-		 * real error code is already in sub_info->retval or
+		 * If ret is 0, either call_usermodehelper_exec_async failed and
+		 * the real error code is already in sub_info->retval or
 		 * sub_info->retval is 0 anyway, so don't mess with it then.
 		 */
 		if (ret)
@@ -304,17 +308,17 @@ static int wait_for_helper(void *data)
 }
 
 /* This is run by khelper thread  */
-static void __call_usermodehelper(struct work_struct *work)
+static void call_usermodehelper_exec_work(struct work_struct *work)
 {
 	struct subprocess_info *sub_info =
 		container_of(work, struct subprocess_info, work);
 	pid_t pid;
 
 	if (sub_info->wait & UMH_WAIT_PROC)
-		pid = kernel_thread(wait_for_helper, sub_info,
+		pid = kernel_thread(call_usermodehelper_exec_sync, sub_info,
 				    CLONE_FS | CLONE_FILES | SIGCHLD);
 	else
-		pid = kernel_thread(____call_usermodehelper, sub_info,
+		pid = kernel_thread(call_usermodehelper_exec_async, sub_info,
 				    SIGCHLD);
 
 	if (pid < 0) {
@@ -509,7 +513,7 @@ struct subprocess_info *call_usermodehelper_setup(char *path, char **argv,
 	if (!sub_info)
 		goto out;
 
-	INIT_WORK(&sub_info->work, __call_usermodehelper);
+	INIT_WORK(&sub_info->work, call_usermodehelper_exec_work);
 	sub_info->path = path;
 	sub_info->argv = argv;
 	sub_info->envp = envp;
-- 
2.1.4

[PATCH 2/3] kmod: Add up-to-date explanations on the purpose of each asynchronous levels

[Frederic Weisbecker]

There seem to be quite some confusions on the comments, likely due to
changes that came after them.

Now since it's very non obvious why we have 3 levels of asynchronous
code to implement usermodehelpers, it's important to comment in detail
the reason of this layout.

Cc: Rik van Riel <riel@redhat.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Lameter <cl@linux.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/kmod.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index 4682e91..f940b21 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -269,7 +269,11 @@ out:
 	do_exit(0);
 }
 
-/* Keventd can't block, but this (a child) can. */
+/*
+ * We couldn't wait for usermodehelper completion from khelper without
+ * blocking other pending concurrent usermodehelper targets. This is why
+ * the UMH_WAIT_PROC flavour runs in its own thread.
+ */
 static int call_usermodehelper_exec_sync(void *data)
 {
 	struct subprocess_info *sub_info = data;
@@ -285,8 +289,8 @@ static int call_usermodehelper_exec_sync(void *data)
 		/*
 		 * Normally it is bogus to call wait4() from in-kernel because
 		 * wait4() wants to write the exit code to a userspace address.
-		 * But call_usermodehelper_exec_sync() always runs as keventd,
-		 * and put_user() to a kernel address works OK for kernel
+		 * But call_usermodehelper_exec_sync() always runs as kernel
+		 * thread and put_user() to a kernel address works OK for kernel
 		 * threads, due to their having an mm_segment_t which spans the
 		 * entire address space.
 		 *
@@ -307,7 +311,15 @@ static int call_usermodehelper_exec_sync(void *data)
 	do_exit(0);
 }
 
-/* This is run by khelper thread  */
+/*
+ * This function doesn't need to be called asynchronously. But we need to create
+ * the usermodehelper kernel threads from a task that is affine to all CPUs
+ * (or nohz housekeeping ones) such that they inherit a global affinity. Khelper
+ * workqueue simply provides that.
+ * call_usermodehelper() can be called from tasks with a reduced CPU
+ * affinity (eg: per-cpu workqueues) and we don't want usermodehelper targets to
+ * contend any busy CPU.
+ */
 static void call_usermodehelper_exec_work(struct work_struct *work)
 {
 	struct subprocess_info *sub_info =
@@ -693,6 +705,18 @@ struct ctl_table usermodehelper_table[] = {
 
 void __init usermodehelper_init(void)
 {
+	/*
+	 * The singlethread property here stands for the need of a workqueue
+	 * with wide CPUs affinity, in order to create usermodehelper kernel
+	 * threads inheriting this attribute irrespective of
+	 * call_usermodehelper() callers. Non-singlethread workqueues are
+	 * otherwise per-cpu and wouldn't produce the desired effect.
+	 *
+	 * The ordering guarantee as a side-effect isn't necessary but shouldn't
+	 * introduce performance issue. All we do is creating two kernel threads.
+	 * This should be fast enough not to block concurrent usermodehelper
+	 * callers.
+	 */
 	khelper_wq = create_singlethread_workqueue("khelper");
 	BUG_ON(!khelper_wq);
 }
-- 
2.1.4

[PATCH 3/3] kmod: Remove unecessary explicit wide CPU affinity setting

[Frederic Weisbecker]

The call_usermodehelper_exec_[a]sync() kernel threads are created by
khelper precisely because we want them to be affine to all CPUs,
irrespective of any call_usermodehelper() caller with reduced CPU
affinity. So this explicit all-CPUs wide affinity forcing is useless.

Not only useless it even breaks nohz full. The housekeeping work
(general kernel internal code that user doesn't care much about) is
handled by a reduced set of CPUs in nohz full, precisely those that are
not included by nohz_full= kernel parameters. For example unbound
workqueues are handled by housekeeping CPUs. And we want the
usermodehelper tasks to be handled by housekeeping CPUs because they are
kernel internals that user critical nohz full work don't want to be
disturbed by.

In nohz full configurations, khelper will naturally be affine to
housekeeping CPUs and this housekeeping affinity is then inherited by
usermodehelper kernel threads. But the explicit call to
set_cpus_allowed_ptr() breaks that.

Simply remove it.

Cc: Rik van Riel <riel@redhat.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Christoph Lameter <cl@linux.com>
Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/kmod.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index f940b21..b89342b 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -223,9 +223,6 @@ static int call_usermodehelper_exec_async(void *data)
 	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);
-
 	/*
 	 * Our parent is keventd, which runs with elevated scheduling priority.
 	 * Avoid propagating that into the userspace child.
-- 
2.1.4

Re: [PATCH 3/3] kmod: Remove unecessary explicit wide CPU affinity setting

[Oleg Nesterov]

On 07/06, Frederic Weisbecker wrote:
>
> The call_usermodehelper_exec_[a]sync() kernel threads are created by
> khelper precisely because

I think khelper should simply die. It doesn't make any sense today,
kmod.c should use some system wq instead. But see below.

> Not only useless it even breaks nohz full. The housekeeping work
> (general kernel internal code that user doesn't care much about) is
> handled by a reduced set of CPUs in nohz full, precisely those that are
> not included by nohz_full= kernel parameters. For example unbound
> workqueues are handled by housekeeping CPUs.

Confused... I do not see how workqueue_attrs->cpumask can depend on
tick_nohz_full_mask or housekeeping_mask. Could you explain?

> @@ -223,9 +223,6 @@ static int call_usermodehelper_exec_async(void *data)
>  	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);
> -

I think this is fine, ->no_numa is true for khelper.

But this means that after this change kmod.c can't use a system wq,
->no_numa is false by default. And khelper is no_numa only because
it is __WQ_ORDERED, but kmod.c doesn't need really need__WQ_ORDERED,
except, again, this implies ->no_numa == T.

So perhaps init_workqueues() should create another global
WQ_UNBOUND/no_numa workqueue_struct so that we could kill khelper_wq?
Or kmod.c can use system_unbound_wq, but then we need to keep this
set_cpus_allowed_ptr().

To me khelper_wq looks just annoying...

Oleg.

Re: [PATCH 3/3] kmod: Remove unecessary explicit wide CPU affinity setting

[Frederic Weisbecker]

On Tue, Jul 07, 2015 at 06:30:30PM +0200, Oleg Nesterov wrote:
> On 07/06, Frederic Weisbecker wrote:
> >
> > The call_usermodehelper_exec_[a]sync() kernel threads are created by
> > khelper precisely because
> 
> I think khelper should simply die. It doesn't make any sense today,
> kmod.c should use some system wq instead. But see below.
> 
> > Not only useless it even breaks nohz full. The housekeeping work
> > (general kernel internal code that user doesn't care much about) is
> > handled by a reduced set of CPUs in nohz full, precisely those that are
> > not included by nohz_full= kernel parameters. For example unbound
> > workqueues are handled by housekeeping CPUs.
> 
> Confused... I do not see how workqueue_attrs->cpumask can depend on
> tick_nohz_full_mask or housekeeping_mask. Could you explain?

People who want CPU isolation will likely write
/sys/devices/virtual/workqueue/cpumask to a reduced set of CPUs, typically
CPU 0 that is used for housekeeping in nohz full.

In fact we should add the code which initialize wq_unbound_cpumask
to housekeeping_mask automatically.

So this cpumask is inherited to khelper because it is a singlethread
workqueue.

> 
> > @@ -223,9 +223,6 @@ static int call_usermodehelper_exec_async(void *data)
> >  	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);
> > -
> 
> I think this is fine, ->no_numa is true for khelper.
> 
> But this means that after this change kmod.c can't use a system wq,
> ->no_numa is false by default. And khelper is no_numa only because
> it is __WQ_ORDERED, but kmod.c doesn't need really need__WQ_ORDERED,
> except, again, this implies ->no_numa == T.

I'm not sure what means no_numa in the context of workqueues, I guess
it's about having system workqueues bound to one CPU or several in the
same nodes. But indeed we can't use system workqueues because they are
per-cpu and we inherit that. And it's ridiculous to call set_cpus_allowed_ptr()
to fix that.

Hence why we use a singlethread, even though we don't care about ordering.
Ok I guess that's more or less what you just said :o)

> So perhaps init_workqueues() should create another global
> WQ_UNBOUND/no_numa workqueue_struct so that we could kill khelper_wq?
> Or kmod.c can use system_unbound_wq, but then we need to keep this
> set_cpus_allowed_ptr().
> 
> To me khelper_wq looks just annoying.

That's a good idea. I can do that!

Perhaps queuing there would be done through schedule_work_unbound() ?
Or schedule_work_no_numa()?

Re: [PATCH 3/3] kmod: Remove unecessary explicit wide CPU affinity setting

[Oleg Nesterov]

On 07/07, Frederic Weisbecker wrote:
>
> On Tue, Jul 07, 2015 at 06:30:30PM +0200, Oleg Nesterov wrote:
> >
> > > Not only useless it even breaks nohz full. The housekeeping work
> > > (general kernel internal code that user doesn't care much about) is
> > > handled by a reduced set of CPUs in nohz full, precisely those that are
> > > not included by nohz_full= kernel parameters. For example unbound
> > > workqueues are handled by housekeeping CPUs.
> >
> > Confused... I do not see how workqueue_attrs->cpumask can depend on
> > tick_nohz_full_mask or housekeeping_mask. Could you explain?
>
> People who want CPU isolation will likely write
> /sys/devices/virtual/workqueue/cpumask to a reduced set of CPUs, typically
> CPU 0 that is used for housekeeping in nohz full.

Well, khelper_wq is not WQ_SYSFS, so I am not sure this is possible.

But this doesn't really matter, people can change cpu affinity. But
"workqueues are handled by housekeeping CPUs" doesn't look right.

> In fact we should add the code which initialize wq_unbound_cpumask
> to housekeeping_mask automatically.

Perhaps, but until then the changelog above looks really confusing,
as if workqueue.c already does this automagically ;)

Oleg.

Re: [PATCH 3/3] kmod: Remove unecessary explicit wide CPU affinity setting

[Oleg Nesterov]

On 07/07, Oleg Nesterov wrote:
>
> On 07/07, Frederic Weisbecker wrote:
> >
> >
> > People who want CPU isolation will likely write
> > /sys/devices/virtual/workqueue/cpumask to a reduced set of CPUs, typically
> > CPU 0 that is used for housekeeping in nohz full.
>
> Well, khelper_wq is not WQ_SYSFS, so I am not sure this is possible.

Please ignore, I see the new "workqueue: Create low-level unbound
workqueues cpumask" commit...


> But this doesn't really matter, people can change cpu affinity. But
> "workqueues are handled by housekeeping CPUs" doesn't look right.
>
> > In fact we should add the code which initialize wq_unbound_cpumask
> > to housekeeping_mask automatically.
>
> Perhaps, but until then the changelog above looks really confusing,
> as if workqueue.c already does this automagically ;)

Yes.

Oleg.

Re: [PATCH 2/3] kmod: Add up-to-date explanations on the purpose of each asynchronous levels

[Andrew Morton]

On Mon,  6 Jul 2015 17:33:40 +0200 Frederic Weisbecker <fweisbec@gmail.com> wrote:

> There seem to be quite some confusions on the comments, likely due to
> changes that came after them.
> 
> Now since it's very non obvious why we have 3 levels of asynchronous
> code to implement usermodehelpers, it's important to comment in detail
> the reason of this layout.

There are still a few references to keventd in there.  One of them is
simply wrong: "runs as a child of keventd".  The userspace code is
actually a child of the khelper thread, yes?

I guess we should remove all kernel references to "keventd".  It got
renamed to "kworker".

Re: [PATCH 2/3] kmod: Add up-to-date explanations on the purpose of each asynchronous levels

[Frederic Weisbecker]

On Tue, Jul 07, 2015 at 04:07:58PM -0700, Andrew Morton wrote:
> On Mon,  6 Jul 2015 17:33:40 +0200 Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > There seem to be quite some confusions on the comments, likely due to
> > changes that came after them.
> > 
> > Now since it's very non obvious why we have 3 levels of asynchronous
> > code to implement usermodehelpers, it's important to comment in detail
> > the reason of this layout.
> 
> There are still a few references to keventd in there.  One of them is
> simply wrong: "runs as a child of keventd".  The userspace code is
> actually a child of the khelper thread, yes?
> 
> I guess we should remove all kernel references to "keventd".  It got
> renamed to "kworker".

Right, I think I missed them because I confused khelper with keventd.
In fact here they are all children of khelper, which is the singlethread
workqueue tied to kmod.

But I'm working on a new iteration that makes use of a global no numa
workqueue.