[PATCH] fix bogus hotplug cpu warning

[PATCH] fix bogus hotplug cpu warning

[Hugh Dickins]

Fix bogus DEBUG_PREEMPT warning on x86_64, when cpu brought online after
bootup: current_is_keventd is right to note its use of smp_processor_id
is preempt-safe, but should use raw_smp_processor_id to avoid the warning.

Signed-off-by: Hugh Dickins <hugh@veritas.com>

--- 2.6.23-rc3-git10/kernel/workqueue.c	2007-07-26 19:49:58.000000000 +0100
+++ linux/kernel/workqueue.c	2007-08-26 18:59:16.000000000 +0100
@@ -635,7 +635,7 @@ int keventd_up(void)
 int current_is_keventd(void)
 {
 	struct cpu_workqueue_struct *cwq;
-	int cpu = smp_processor_id();	/* preempt-safe: keventd is per-cpu */
+	int cpu = raw_smp_processor_id(); /* preempt-safe: keventd is per-cpu */
 	int ret = 0;
 
 	BUG_ON(!keventd_wq);

Re: [PATCH] fix bogus hotplug cpu warning

[Andrew Morton]

On Mon, 27 Aug 2007 16:06:19 +0100 (BST)
Hugh Dickins <hugh@veritas.com> wrote:

> Fix bogus DEBUG_PREEMPT warning on x86_64, when cpu brought online after
> bootup: current_is_keventd is right to note its use of smp_processor_id
> is preempt-safe, but should use raw_smp_processor_id to avoid the warning.
> 
> Signed-off-by: Hugh Dickins <hugh@veritas.com>
> 
> --- 2.6.23-rc3-git10/kernel/workqueue.c	2007-07-26 19:49:58.000000000 +0100
> +++ linux/kernel/workqueue.c	2007-08-26 18:59:16.000000000 +0100
> @@ -635,7 +635,7 @@ int keventd_up(void)
>  int current_is_keventd(void)
>  {
>  	struct cpu_workqueue_struct *cwq;
> -	int cpu = smp_processor_id();	/* preempt-safe: keventd is per-cpu */
> +	int cpu = raw_smp_processor_id(); /* preempt-safe: keventd is per-cpu */
>  	int ret = 0;
>  
>  	BUG_ON(!keventd_wq);

But lib/smp_processor_id.c:debug_smp_processor_id() does

	/*
	 * Kernel threads bound to a single CPU can safely use
	 * smp_processor_id():
	 */
	this_mask = cpumask_of_cpu(this_cpu);

	if (cpus_equal(current->cpus_allowed, this_mask))
		goto out;

So I assume that this warning was triggering because some non-keventd,
non-pinned task is calling current_is_keventd()?


So I agree with the patch, but not with its description.

Re: [PATCH] fix bogus hotplug cpu warning

[Hugh Dickins]

On Mon, 27 Aug 2007, Andrew Morton wrote:
> On Mon, 27 Aug 2007 16:06:19 +0100 (BST)
> Hugh Dickins <hugh@veritas.com> wrote:
> 
> > Fix bogus DEBUG_PREEMPT warning on x86_64, when cpu brought online after
> > bootup: current_is_keventd is right to note its use of smp_processor_id
> > is preempt-safe, but should use raw_smp_processor_id to avoid the warning.
> > 
> > Signed-off-by: Hugh Dickins <hugh@veritas.com>
> > 
> > --- 2.6.23-rc3-git10/kernel/workqueue.c	2007-07-26 19:49:58.000000000 +0100
> > +++ linux/kernel/workqueue.c	2007-08-26 18:59:16.000000000 +0100
> > @@ -635,7 +635,7 @@ int keventd_up(void)
> >  int current_is_keventd(void)
> >  {
> >  	struct cpu_workqueue_struct *cwq;
> > -	int cpu = smp_processor_id();	/* preempt-safe: keventd is per-cpu */
> > +	int cpu = raw_smp_processor_id(); /* preempt-safe: keventd is per-cpu */
> >  	int ret = 0;
> >  
> >  	BUG_ON(!keventd_wq);
> 
> But lib/smp_processor_id.c:debug_smp_processor_id() does
> 
> 	/*
> 	 * Kernel threads bound to a single CPU can safely use
> 	 * smp_processor_id():
> 	 */
> 	this_mask = cpumask_of_cpu(this_cpu);
> 
> 	if (cpus_equal(current->cpus_allowed, this_mask))
> 		goto out;
> 
> So I assume that this warning was triggering because some non-keventd,
> non-pinned task is calling current_is_keventd()?

That's right, the x86_64 (and the ia64) do_boot_cpu() functions (called
when onlining a cpu) do a check on current_is_keventd(): commented thus
 * During cold boot process, keventd thread is not spun up yet.
 * When we do cpu hot-add, we create idle threads on the fly, we should
 * not acquire any attributes from the calling context. Hence the clean
 * way to create kernel_threads() is to do that from keventd().
 * We do the current_is_keventd() due to the fact that ACPI notifier
 * was also queuing to keventd() and when the caller is already running
 * in context of keventd(), we would end up with locking up the keventd
 * thread.
though I've not tried to think that through.

> So I agree with the patch, but not with its description.

I don't see which part of the description you disagree with, but please
do improve it if you can.  The actual trace (when powersaved was saving
power by onlining a cpu I'd tried to exclude with maxcpus=1 ;) was this:

 BUG: using smp_processor_id() in preemptible [00000001] code: powersaved/3368
 caller is current_is_keventd+0x9/0x3d
 
 Call Trace:
  [<ffffffff8031e5f5>] debug_smp_processor_id+0xc1/0xd4
  [<ffffffff802453f3>] current_is_keventd+0x9/0x3d
  [<ffffffff80219c9b>] do_boot_cpu+0x19e/0x3c9
  [<ffffffff80219ad4>] do_fork_idle+0x0/0x29
  [<ffffffff80219fb7>] __cpu_up+0xd4/0x107
  [<ffffffff80252932>] _cpu_up+0xd9/0x17a
  [<ffffffff80252a09>] cpu_up+0x36/0x4d
  [<ffffffff8038f7ca>] store_online+0x52/0x81
  [<ffffffff8038b575>] sysdev_store+0x3c/0x3e
  [<ffffffff802cfed4>] flush_write_buffer+0x63/0x81
  [<ffffffff802cff5b>] sysfs_write_file+0x69/0x8f
  [<ffffffff8046e5d9>] _spin_unlock_irqrestore+0x16/0x30
  [<ffffffff8028aea3>] vfs_write+0xc5/0x186
  [<ffffffff8024c180>] up_write+0xd/0xf
  [<ffffffff8028b02f>] sys_write+0x51/0x7a
  [<ffffffff8020bc5e>] system_call+0x7e/0x83

Hugh

Re: [PATCH] fix bogus hotplug cpu warning

[Andrew Morton]

On Mon, 27 Aug 2007 21:37:14 +0100 (BST)
Hugh Dickins <hugh@veritas.com> wrote:

> > So I agree with the patch, but not with its description.
> 
> I don't see which part of the description you disagree with, but please
> do improve it if you can.

I'd change the description to


current_is_keventd() is a stupid load of crap.  It does

int current_is_keventd(void)
{
	struct cpu_workqueue_struct *cwq;
	int cpu = smp_processor_id();	/* preempt-safe: keventd is per-cpu */

but that comment (and the code itself) only make sense when
current_is_keventd() is called by keventd.  In which case thers is no point
in the function even existing!

We need to use raw_smp_processor_id() so that non-keventd (or, more
specifically, non-pinned-to-one-cpu) callers won't generate "using
smp_processor_id() in preemptible" warnings.

Re: [PATCH] fix bogus hotplug cpu warning

[Hugh Dickins]

On Mon, 27 Aug 2007, Andrew Morton wrote:
> On Mon, 27 Aug 2007 21:37:14 +0100 (BST)
> Hugh Dickins <hugh@veritas.com> wrote:
> 
> > > So I agree with the patch, but not with its description.
> > 
> > I don't see which part of the description you disagree with, but please
> > do improve it if you can.
> 
> I'd change the description to
> 
> 
> current_is_keventd() is a stupid load of crap.  It does
> 
> int current_is_keventd(void)
> {
> 	struct cpu_workqueue_struct *cwq;
> 	int cpu = smp_processor_id();	/* preempt-safe: keventd is per-cpu */
> 
> but that comment (and the code itself) only make sense when
> current_is_keventd() is called by keventd.  In which case thers is no point
> in the function even existing!

I disagree.  The comment (your very own, I think?) and the code made
sense to me.  It's saying this is rather unusual, but safe against
preemption, because keventd is per-cpu (it might perhaps be better
if it said each keventd is bound to a cpu).

The code works in such a way that if it is a keventd, then it'll
correctly arrive at the answer yes; and if it's not a keventd,
then even if it's preempted to a different cpu in the middle,
the test won't match any keventd and so it'll correctly arrive
at the answer no.

But I'm content with the half-liner already there.

> 
> We need to use raw_smp_processor_id() so that non-keventd (or, more
> specifically, non-pinned-to-one-cpu) callers won't generate "using
> smp_processor_id() in preemptible" warnings.

Yes, that's true, raw_smp_processor_id() is needed to avoid the
warning; but it was already preempt-safe with smp_processor_id(),
just in danger of giving an inappropriate warning.

Hugh