Re: + work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand.patch added to -mm tree

Re: + work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand.patch added to -mm tree

[Ingo Molnar]

* akpm@linux-foundation.org <akpm@linux-foundation.org> wrote:

> ------------------------------------------------------
> Subject: work_on_cpu(): rewrite it to create a kernel thread on demand
> From: Andrew Morton <akpm@linux-foundation.org>
> 
> The various implemetnations and proposed implemetnations of work_on_cpu()
> are vulnerable to various deadlocks because they all used queues of some
> form.
> 
> Unrelated pieces of kernel code thus gained dependencies wherein if one
> work_on_cpu() caller holds a lock which some other work_on_cpu() callback
> also takes, the kernel could rarely deadlock.
> 
> Fix this by creating a short-lived kernel thread for each work_on_cpu()
> invokation.
> 
> This is not terribly fast, but the only current caller of work_on_cpu() is
> pci_call_probe().

hm, it's quite ugly as well, and wasteful with resources.

	Ingo

Re: + work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand.patch added to -mm tree

[Frédéric Weisbecker]

2009/2/3 Ingo Molnar <mingo@elte.hu>:
>
> * akpm@linux-foundation.org <akpm@linux-foundation.org> wrote:
>
>> ------------------------------------------------------
>> Subject: work_on_cpu(): rewrite it to create a kernel thread on demand
>> From: Andrew Morton <akpm@linux-foundation.org>
>>
>> The various implemetnations and proposed implemetnations of work_on_cpu()
>> are vulnerable to various deadlocks because they all used queues of some
>> form.
>>
>> Unrelated pieces of kernel code thus gained dependencies wherein if one
>> work_on_cpu() caller holds a lock which some other work_on_cpu() callback
>> also takes, the kernel could rarely deadlock.
>>
>> Fix this by creating a short-lived kernel thread for each work_on_cpu()
>> invokation.
>>
>> This is not terribly fast, but the only current caller of work_on_cpu() is
>> pci_call_probe().
>
> hm, it's quite ugly as well, and wasteful with resources.


Sorry I don't see the patch but only the changelog.
So perhaps my answer will be a bit out of sync.

But if pci_call_probe() is the only caller, so it is supposed to be
called only on boot.
Perhaps the work_on_cpu thread can be killed after boot up and then
become a thread created
on the fly after that if needed....

Or perhaps it's too much complex.....

Re: + work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand.patch added to -mm tree

[Andrew Morton]

On Tue, 3 Feb 2009 17:58:13 +0100
Fr__d__ric Weisbecker <fweisbec@gmail.com> wrote:

> 2009/2/3 Ingo Molnar <mingo@elte.hu>:
> >
> > * akpm@linux-foundation.org <akpm@linux-foundation.org> wrote:
> >
> >> ------------------------------------------------------
> >> Subject: work_on_cpu(): rewrite it to create a kernel thread on demand
> >> From: Andrew Morton <akpm@linux-foundation.org>
> >>
> >> The various implemetnations and proposed implemetnations of work_on_cpu()
> >> are vulnerable to various deadlocks because they all used queues of some
> >> form.
> >>
> >> Unrelated pieces of kernel code thus gained dependencies wherein if one
> >> work_on_cpu() caller holds a lock which some other work_on_cpu() callback
> >> also takes, the kernel could rarely deadlock.
> >>
> >> Fix this by creating a short-lived kernel thread for each work_on_cpu()
> >> invokation.
> >>
> >> This is not terribly fast, but the only current caller of work_on_cpu() is
> >> pci_call_probe().
> >
> > hm, it's quite ugly as well

No it isn't.

It's no less ugly than the current code.

It's less buggy than the current code.

>, and wasteful with resources.

The current code consumes about 10kbytes per cpu and one kernel thread
per cpu.  This code fixes that.

(ie: since when did you guys care about consuming resources?)

> Sorry I don't see the patch but only the changelog.
> So perhaps my answer will be a bit out of sync.
> 
> But if pci_call_probe() is the only caller, so it is supposed to be
> called only on boot.
> Perhaps the work_on_cpu thread can be killed after boot up and then
> become a thread created
> on the fly after that if needed....
> 
> Or perhaps it's too much complex.....

Series of four patches:

- switch cstate.c frmo work_on_cpu to smp_call_function_single()

- ditto acpi-cpufreq.c

- ditto mce_amd_64.c

The final work_on_cpu() caller is pci_call_probe().  I'd like to find a
way of removing that callsite as well, so we can finally remove this
turkey but for now, just fix the bugs in it:



From: Andrew Morton <akpm@linux-foundation.org>

The various implementations and proposed implementations of work_on_cpu()
are vulnerable to various deadlocks because they all use queues of some
form.

Unrelated pieces of kernel code thus gained dependencies wherein if one
work_on_cpu() caller holds a lock which some other work_on_cpu() callback
also takes, the kernel could rarely deadlock.

Also, the present work_on_cpu() implementation creates yet another kernel
thread per CPU.

Fix this by creating a short-lived kernel thread for each work_on_cpu()
invokation.

This is not terribly fast, but the only current caller of work_on_cpu() is
pci_call_probe().

It would be nice to find some other way of doing the node-local
allocations in the PCI probe code so that we can zap work_on_cpu()
altogether.  The code there is rather nasty.  I can't think of anything
simple at this time...

Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/workqueue.c |   36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff -puN kernel/workqueue.c~work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand kernel/workqueue.c
--- a/kernel/workqueue.c~work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand
+++ a/kernel/workqueue.c
@@ -971,20 +971,20 @@ undo:
 }
 
 #ifdef CONFIG_SMP
-static struct workqueue_struct *work_on_cpu_wq __read_mostly;
 
 struct work_for_cpu {
-	struct work_struct work;
+	struct completion completion;
 	long (*fn)(void *);
 	void *arg;
 	long ret;
 };
 
-static void do_work_for_cpu(struct work_struct *w)
+static int do_work_for_cpu(void *_wfc)
 {
-	struct work_for_cpu *wfc = container_of(w, struct work_for_cpu, work);
-
+	struct work_for_cpu *wfc = _wfc;
 	wfc->ret = wfc->fn(wfc->arg);
+	complete(&wfc->completion);
+	return 0;
 }
 
 /**
@@ -995,17 +995,23 @@ static void do_work_for_cpu(struct work_
  *
  * This will return the value @fn returns.
  * It is up to the caller to ensure that the cpu doesn't go offline.
+ * The caller must not hold any locks which would prevent @fn from completing.
  */
 long work_on_cpu(unsigned int cpu, long (*fn)(void *), void *arg)
 {
-	struct work_for_cpu wfc;
-
-	INIT_WORK(&wfc.work, do_work_for_cpu);
-	wfc.fn = fn;
-	wfc.arg = arg;
-	queue_work_on(cpu, work_on_cpu_wq, &wfc.work);
-	flush_work(&wfc.work);
-
+	struct task_struct *sub_thread;
+	struct work_for_cpu wfc = {
+		.completion = COMPLETION_INITIALIZER_ONSTACK(wfc.completion),
+		.fn = fn,
+		.arg = arg,
+	};
+
+	sub_thread = kthread_create(do_work_for_cpu, &wfc, "work_for_cpu");
+	if (IS_ERR(sub_thread))
+		return PTR_ERR(sub_thread);
+	kthread_bind(sub_thread, cpu);
+	wake_up_process(sub_thread);
+	wait_for_completion(&wfc.completion);
 	return wfc.ret;
 }
 EXPORT_SYMBOL_GPL(work_on_cpu);
@@ -1021,8 +1027,4 @@ void __init init_workqueues(void)
 	hotcpu_notifier(workqueue_cpu_callback, 0);
 	keventd_wq = create_workqueue("events");
 	BUG_ON(!keventd_wq);
-#ifdef CONFIG_SMP
-	work_on_cpu_wq = create_workqueue("work_on_cpu");
-	BUG_ON(!work_on_cpu_wq);
-#endif
 }
_

Re: + work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand.patch added to -mm tree

[Rusty Russell]

On Wednesday 04 February 2009 05:55:29 Andrew Morton wrote:
> On Tue, 3 Feb 2009 17:58:13 +0100
> Fr__d__ric Weisbecker <fweisbec@gmail.com> wrote:
> 
> > 2009/2/3 Ingo Molnar <mingo@elte.hu>:
> > >
> > > * akpm@linux-foundation.org <akpm@linux-foundation.org> wrote:
> > >
> > >> ------------------------------------------------------
> > >> Subject: work_on_cpu(): rewrite it to create a kernel thread on demand
> > >> From: Andrew Morton <akpm@linux-foundation.org>
> > >>
> > >> The various implemetnations and proposed implemetnations of work_on_cpu()
> > >> are vulnerable to various deadlocks because they all used queues of some
> > >> form.
> > >>
> > >> Unrelated pieces of kernel code thus gained dependencies wherein if one
> > >> work_on_cpu() caller holds a lock which some other work_on_cpu() callback
> > >> also takes, the kernel could rarely deadlock.
> > >>
> > >> Fix this by creating a short-lived kernel thread for each work_on_cpu()
> > >> invokation.
> > >>
> > >> This is not terribly fast, but the only current caller of work_on_cpu() is
> > >> pci_call_probe().
> > >
> > > hm, it's quite ugly as well
> 
> No it isn't.
> 
> It's no less ugly than the current code.
> 
> It's less buggy than the current code.

Whatever, I like your version.

Tho making it a series of 5 and exposing rdmsr_on_cpu/wrmsr_on_cpu for other
uses would be even better.

Thanks,
Rusty.

Re: + work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand.patch added to -mm tree

[Andrew Morton]

On Wed, 4 Feb 2009 14:28:11 +1030 Rusty Russell <rusty@rustcorp.com.au> wrote:

> On Wednesday 04 February 2009 05:55:29 Andrew Morton wrote:
> > On Tue, 3 Feb 2009 17:58:13 +0100
> > Fr__d__ric Weisbecker <fweisbec@gmail.com> wrote:
> > 
> > > 2009/2/3 Ingo Molnar <mingo@elte.hu>:
> > > >
> > > > * akpm@linux-foundation.org <akpm@linux-foundation.org> wrote:
> > > >
> > > >> ------------------------------------------------------
> > > >> Subject: work_on_cpu(): rewrite it to create a kernel thread on demand
> > > >> From: Andrew Morton <akpm@linux-foundation.org>
> > > >>
> > > >> The various implemetnations and proposed implemetnations of work_on_cpu()
> > > >> are vulnerable to various deadlocks because they all used queues of some
> > > >> form.
> > > >>
> > > >> Unrelated pieces of kernel code thus gained dependencies wherein if one
> > > >> work_on_cpu() caller holds a lock which some other work_on_cpu() callback
> > > >> also takes, the kernel could rarely deadlock.
> > > >>
> > > >> Fix this by creating a short-lived kernel thread for each work_on_cpu()
> > > >> invokation.
> > > >>
> > > >> This is not terribly fast, but the only current caller of work_on_cpu() is
> > > >> pci_call_probe().
> > > >
> > > > hm, it's quite ugly as well
> > 
> > No it isn't.
> > 
> > It's no less ugly than the current code.
> > 
> > It's less buggy than the current code.
> 
> Whatever, I like your version.

Careful, or you'll own it ;)

> Tho making it a series of 5 and exposing rdmsr_on_cpu/wrmsr_on_cpu for other
> uses would be even better.

These:

int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);
int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h);
int rdmsr_safe_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);
int wrmsr_safe_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h);

already exist.  I don't think anything else needs to be done here?

Re: + work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand.patch added to -mm tree

[Rusty Russell]

On Wednesday 04 February 2009 14:46:42 Andrew Morton wrote:
> These:
> 
> int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);
> int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h);
> int rdmsr_safe_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);
> int wrmsr_safe_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h);
> 
> already exist.  I don't think anything else needs to be done here?

Nope.  Well, it'd be nice if they just used a u64, but we can't fix the whole
kernel.

Thanks!
Rusty.

Re: + work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand.patch added to -mm tree

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:
>
> Series of four patches:
>
> - switch cstate.c frmo work_on_cpu to smp_call_function_single()
>
> - ditto acpi-cpufreq.c
>
> - ditto mce_amd_64.c
>
> The final work_on_cpu() caller is pci_call_probe().  I'd like to find a
> way of removing that callsite as well, so we can finally remove this
> turkey but for now, just fix the bugs in it:

As far as I can tell when we are doing the probes we are in a function
that can sleep, so we should be able to just call set_cpus_allowed to
remove the need for work_on_cpu in pci_call_probe.  Possibly with a
save/restore of the allowed cpus.

Am I missing something?

Eric

Re: + work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand.patch added to -mm tree

[Andrew Morton]

On Thu, 12 Feb 2009 12:38:36 -0800
ebiederm@xmission.com (Eric W. Biederman) wrote:

> Andrew Morton <akpm@linux-foundation.org> writes:
> >
> > Series of four patches:
> >
> > - switch cstate.c frmo work_on_cpu to smp_call_function_single()
> >
> > - ditto acpi-cpufreq.c
> >
> > - ditto mce_amd_64.c
> >
> > The final work_on_cpu() caller is pci_call_probe().  I'd like to find a
> > way of removing that callsite as well, so we can finally remove this
> > turkey but for now, just fix the bugs in it:
> 
> As far as I can tell when we are doing the probes we are in a function
> that can sleep, so we should be able to just call set_cpus_allowed to
> remove the need for work_on_cpu in pci_call_probe.  Possibly with a
> save/restore of the allowed cpus.
> 
> Am I missing something?
> 

The problem with set_cpus_allowed() is that some other
suitably-privileged userspace process can come in from the side and
modify your cpus_allowed at any time.

Re: + work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand.patch added to -mm tree

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> The problem with set_cpus_allowed() is that some other
> suitably-privileged userspace process can come in from the side and
> modify your cpus_allowed at any time.

According to the comments the only reason we care is so that
we get the appropriate NUMA affinity by default.  I don't
think it would be fatal if userspace messed around and we
had a wrong value.

Does work_on_cpu prevent that?

Eric

Re: + work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand.patch added to -mm tree

[Eric W. Biederman]

I should follow up and say that the reason I care right now, is I am
digging into pci hotplug.  One of the issues I'm fighting is that
currently I appear to need a dedicated kernel thread for each pci
hotplug slot.  It gets easy to deadlock the kernel hotplugging
a hotplug controller otherwise.

Eric

Re: + work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand.patch added to -mm tree

[Andrew Morton]

On Thu, 12 Feb 2009 14:13:06 -0800
ebiederm@xmission.com (Eric W. Biederman) wrote:

> 
> I should follow up and say that the reason I care right now, is I am
> digging into pci hotplug.  One of the issues I'm fighting is that
> currently I appear to need a dedicated kernel thread for each pci
> hotplug slot.  It gets easy to deadlock the kernel hotplugging
> a hotplug controller otherwise.
> 

um, ok, if you say so...

I'd have thought that a short-lived kernel thread would be appropriate,
if poss.  Physical hotplug of a PCI device isn't a high-frequency
operation.

The new-fangled work_on_cpu() could do that, or maybe the new-fangled
kernel/async.c code.

Re: + work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand.patch added to -mm tree

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Thu, 12 Feb 2009 14:13:06 -0800
> ebiederm@xmission.com (Eric W. Biederman) wrote:
>
>> 
>> I should follow up and say that the reason I care right now, is I am
>> digging into pci hotplug.  One of the issues I'm fighting is that
>> currently I appear to need a dedicated kernel thread for each pci
>> hotplug slot.  It gets easy to deadlock the kernel hotplugging
>> a hotplug controller otherwise.
>> 
>
> um, ok, if you say so...
>
> I'd have thought that a short-lived kernel thread would be appropriate,
> if poss.  Physical hotplug of a PCI device isn't a high-frequency
> operation.

Oh.  I'm working to find a way to get there.  The trouble is I have
kick off all of this from interrupt context.

> The new-fangled work_on_cpu() could do that, or maybe the new-fangled
> kernel/async.c code.

I will have to look.  A shared workqueue threatens to deadlock when I
try and hotunplug a hotplug slot.  Running cancel_work_sync for work in
your current workqueue is the problem I had. Maybe some of the rest of the
solutions won't have that kind of problem.

I have this crazy thought that workqueues should just be fixed to fork
a short lived kernel thread for each request they process, and then we
don't have to worry about stuff blocking indefinitely.  I think that
will allow us to kill off explicitly named workqueues as well.

Eric

Re: + work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand.patch added to -mm tree

[Andrew Morton]

On Thu, 12 Feb 2009 14:08:09 -0800
ebiederm@xmission.com (Eric W. Biederman) wrote:

> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> > The problem with set_cpus_allowed() is that some other
> > suitably-privileged userspace process can come in from the side and
> > modify your cpus_allowed at any time.
> 
> According to the comments the only reason we care is so that
> we get the appropriate NUMA affinity by default.  I don't
> think it would be fatal if userspace messed around and we
> had a wrong value.

Right.  In this particular case, if you are fantastically unlucky and
hit the race window, all that will happen is that one particular device
will run a bit more slowly.

But at other codesites, the effects of a racing cpus_allowed rewrite
can be fatal.

> Does work_on_cpu prevent that?

Yup.

I think.

Nope.

I don't think there's actually anything which would prevent a
sufficiently stupid/malicious/unlucky administrator from moving the
work_on_cpu thread onto the wrong CPU at the wrong time.  hrm.

Another reason to use smp_call_function_single().

Re: + work_on_cpu-rewrite-it-to-create-a-kernel-thread-on-demand.patch added to -mm tree

[Rusty Russell]

On Friday 13 February 2009 07:08:36 Eric W. Biederman wrote:
> Andrew Morton <akpm@linux-foundation.org> writes:
> >
> > Series of four patches:
> >
> > - switch cstate.c frmo work_on_cpu to smp_call_function_single()
> >
> > - ditto acpi-cpufreq.c
> >
> > - ditto mce_amd_64.c
> >
> > The final work_on_cpu() caller is pci_call_probe().  I'd like to find a
> > way of removing that callsite as well, so we can finally remove this
> > turkey but for now, just fix the bugs in it:
> 
> As far as I can tell when we are doing the probes we are in a function
> that can sleep, so we should be able to just call set_cpus_allowed to
> remove the need for work_on_cpu in pci_call_probe.  Possibly with a
> save/restore of the allowed cpus.
> 
> Am I missing something?

Yes.  This is a questionable practice when it's a kernel thread (but not
really a problem), but a definite no-no on a real process.  Userspace is
allowed to change affinity on any process at any time; that's why we need
a real method to replace this meme.

(I'm also getting those cpumask's off the stack for core and x86 code, which
is why I'm hitting them all).

Thanks,
Rusty.