Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1718129 > unrolled thread

Re: [GIT PULL] workqueue fixes for v4.13-rc3

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [GIT PULL] workqueue fixes for v4.13-rc3 Geert Uytterhoeven <geert@linux-m68k.org> - 2017-08-23 10:20 +0200
    Re: [GIT PULL] workqueue fixes for v4.13-rc3 Tejun Heo <tj@kernel.org> - 2017-08-23 16:30 +0200
      Re: [GIT PULL] workqueue fixes for v4.13-rc3 Geert Uytterhoeven <geert@linux-m68k.org> - 2017-08-23 16:50 +0200
        Re: [GIT PULL] workqueue fixes for v4.13-rc3 Tejun Heo <tj@kernel.org> - 2017-08-23 19:10 +0200
          Re: [GIT PULL] workqueue fixes for v4.13-rc3 Geert Uytterhoeven <geert@linux-m68k.org> - 2017-08-24 15:40 +0200
            Re: [GIT PULL] workqueue fixes for v4.13-rc3 Tejun Heo <tj@kernel.org> - 2017-08-24 16:40 +0200

#1718129 — Re: [GIT PULL] workqueue fixes for v4.13-rc3

Hi Tejun,

On Mon, Aug 7, 2017 at 7:06 PM, Tejun Heo <tj@kernel.org> wrote:
> On Mon, Aug 07, 2017 at 02:18:51PM +0200, Geert Uytterhoeven wrote:
>> This triggers on m68k, which doesn't have SMP.
>> Haven't tried it yet on any other system due to holidays.
>
> That's weird.  Can you please apply the following patch and report the
> messages?
>
> Thanks.
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index ca937b0..1b9d21b 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -3579,8 +3579,10 @@ static bool wq_calc_node_cpumask(const struct workqueue_attrs *attrs, int node,
>         cpumask_and(cpumask, attrs->cpumask, wq_numa_possible_cpumask[node]);
>
>         if (cpumask_empty(cpumask)) {
> -               pr_warn_once("WARNING: workqueue cpumask: online intersect > "
> -                               "possible intersect\n");
> +               pr_warn_once("WARNING: workqueue empty cpumask: node=%d cpu_going_down=%d cpumask=%*pb online=%*pb possible=%*pb\n",
> +                            node, cpu_going_down, cpumask_pr_args(attrs->cpumask),
> +                            cpumask_pr_args(cpumask_of_node(node)),
> +                            cpumask_pr_args(wq_numa_possible_cpumask[node]));

WARNING: workqueue empty cpumask: node=1 cpu_going_down=-1 cpumask=1
online=1 possible=0

>                 return false;
>         }
>
> @@ -5526,6 +5528,9 @@ static void __init wq_numa_init(void)
>
>         wq_numa_possible_cpumask = tbl;
>         wq_numa_enabled = true;
> +
> +       for_each_node(node)
> +               printk("XXX wq node[%d] %*pb\n", node, cpumask_pr_args(wq_numa_possible_cpumask[node]));

XXX wq node[0] 1
XXX wq node[1] 0
XXX wq node[2] 0
XXX wq node[3] 0
XXX wq node[4] 0
XXX wq node[5] 0
XXX wq node[6] 0
XXX wq node[7] 0

>  }

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [next] | [standalone]

#1718409

Hello, Geert.

Something is really fishy.

On Wed, Aug 23, 2017 at 10:10:54AM +0200, Geert Uytterhoeven wrote:
> > +               pr_warn_once("WARNING: workqueue empty cpumask: node=%d cpu_going_down=%d cpumask=%*pb online=%*pb possible=%*pb\n",
> > +                            node, cpu_going_down, cpumask_pr_args(attrs->cpumask),
> > +                            cpumask_pr_args(cpumask_of_node(node)),
> > +                            cpumask_pr_args(wq_numa_possible_cpumask[node]));
> 
> WARNING: workqueue empty cpumask: node=1 cpu_going_down=-1 cpumask=1
> online=1 possible=0

So, somehow cpu0 seems to be associated with node 1 instead of 0.  It
seems highly unlikely but does the system actually have multiple NUMA
nodes?

> > @@ -5526,6 +5528,9 @@ static void __init wq_numa_init(void)
> >
> >         wq_numa_possible_cpumask = tbl;
> >         wq_numa_enabled = true;
> > +
> > +       for_each_node(node)
> > +               printk("XXX wq node[%d] %*pb\n", node, cpumask_pr_args(wq_numa_possible_cpumask[node]));
> 
> XXX wq node[0] 1
> XXX wq node[1] 0
> XXX wq node[2] 0
> XXX wq node[3] 0
> XXX wq node[4] 0
> XXX wq node[5] 0
> XXX wq node[6] 0
> XXX wq node[7] 0

No idea why num_possible_cpus() is 8 on a non-SMP system but the
problem is that, during boot while wq_numa_init() was running, cpu0
reported that it's associated with node 0, but later it reports that
it's associated node 1.  It looks like NUMA setup is screwed up.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1718422

Hi Tejun,

On Wed, Aug 23, 2017 at 4:24 PM, Tejun Heo <tj@kernel.org> wrote:
> On Wed, Aug 23, 2017 at 10:10:54AM +0200, Geert Uytterhoeven wrote:
>> > +               pr_warn_once("WARNING: workqueue empty cpumask: node=%d cpu_going_down=%d cpumask=%*pb online=%*pb possible=%*pb\n",
>> > +                            node, cpu_going_down, cpumask_pr_args(attrs->cpumask),
>> > +                            cpumask_pr_args(cpumask_of_node(node)),
>> > +                            cpumask_pr_args(wq_numa_possible_cpumask[node]));
>>
>> WARNING: workqueue empty cpumask: node=1 cpu_going_down=-1 cpumask=1
>> online=1 possible=0
>
> So, somehow cpu0 seems to be associated with node 1 instead of 0.  It
> seems highly unlikely but does the system actually have multiple NUMA
> nodes?
>
>> > @@ -5526,6 +5528,9 @@ static void __init wq_numa_init(void)
>> >
>> >         wq_numa_possible_cpumask = tbl;
>> >         wq_numa_enabled = true;
>> > +
>> > +       (node)
>> > +               printk("XXX wq node[%d] %*pb\n", node, cpumask_pr_args(wq_numa_possible_cpumask[node]));
>>
>> XXX wq node[0] 1
>> XXX wq node[1] 0
>> XXX wq node[2] 0
>> XXX wq node[3] 0
>> XXX wq node[4] 0
>> XXX wq node[5] 0
>> XXX wq node[6] 0
>> XXX wq node[7] 0
>
> No idea why num_possible_cpus() is 8 on a non-SMP system but the
> problem is that, during boot while wq_numa_init() was running, cpu0
> reported that it's associated with node 0, but later it reports that
> it's associated node 1.  It looks like NUMA setup is screwed up.

Some code is mixing up multiple memory nodes with multiple cpu nodes.
M68k uses DISCONTIGMEM, but  not NUMA (no SMP):

    config NEED_MULTIPLE_NODES
        def_bool y
        depends on DISCONTIGMEM || NUMA

The (virtual) Atari has 2 memory nodes:
  - node0: start 0x00000000 size 0x00e00000
  - node1: start 0x01000000 size 0x10000000
Both are tied to the same (single) CPU node, of course.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1718530

Hello, Geert.

On Wed, Aug 23, 2017 at 04:47:41PM +0200, Geert Uytterhoeven wrote:
> Some code is mixing up multiple memory nodes with multiple cpu nodes.
> M68k uses DISCONTIGMEM, but  not NUMA (no SMP):
> 
>     config NEED_MULTIPLE_NODES
>         def_bool y
>         depends on DISCONTIGMEM || NUMA
> 
> The (virtual) Atari has 2 memory nodes:
>   - node0: start 0x00000000 size 0x00e00000
>   - node1: start 0x01000000 size 0x10000000
> Both are tied to the same (single) CPU node, of course.

Ah, okay, so it has multiple nodes but not NUMA.  The generic numa
topology code assumes that there's only one node if !NUMA and reports
all online cpus regardless of the node number, which makes the same
CPUs to be reported for all nodes on the system.  I think something
like the following (completely untested) should work.

Thanks.

diff --git a/include/asm-generic/topology.h b/include/asm-generic/topology.h
index fc824e2..b31e84a 100644
--- a/include/asm-generic/topology.h
+++ b/include/asm-generic/topology.h
@@ -48,7 +48,11 @@
 #define parent_node(node)	((void)(node),0)
 #endif
 #ifndef cpumask_of_node
-#define cpumask_of_node(node)	((void)node, cpu_online_mask)
+  #ifdef CONFIG_NEED_MULTIPLE_NODES
+    #define cpumask_of_node(node)	((node) == 0 ? cpu_online_mask : cpu_empty_mask)
+  #else
+    #define cpumask_of_node(node)	((void)node, cpu_online_mask)
+  #endif
 #endif
 #ifndef pcibus_to_node
 #define pcibus_to_node(bus)	((void)(bus), -1)
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 4bf4479..4d41d3b 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -89,10 +89,12 @@ extern struct cpumask __cpu_possible_mask;
 extern struct cpumask __cpu_online_mask;
 extern struct cpumask __cpu_present_mask;
 extern struct cpumask __cpu_active_mask;
+extern struct cpumask __cpu_empty_mask;
 #define cpu_possible_mask ((const struct cpumask *)&__cpu_possible_mask)
 #define cpu_online_mask   ((const struct cpumask *)&__cpu_online_mask)
 #define cpu_present_mask  ((const struct cpumask *)&__cpu_present_mask)
 #define cpu_active_mask   ((const struct cpumask *)&__cpu_active_mask)
+#define cpu_empty_mask    ((const struct cpumask *)&__cpu_empty_mask)
 
 #if NR_CPUS > 1
 #define num_online_cpus()	cpumask_weight(cpu_online_mask)
diff --git a/kernel/cpu.c b/kernel/cpu.c
index ab86045..eeed945 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1741,6 +1741,9 @@ EXPORT_SYMBOL(__cpu_present_mask);
 struct cpumask __cpu_active_mask __read_mostly;
 EXPORT_SYMBOL(__cpu_active_mask);
 
+struct cpumask __cpu_empty_mask __read_mostly;
+EXPORT_SYMBOL(__cpu_empty_mask);
+
 void init_cpu_present(const struct cpumask *src)
 {
 	cpumask_copy(&__cpu_present_mask, src);

[toc] | [prev] | [next] | [standalone]

#1719240

Hi Tejun,

On Wed, Aug 23, 2017 at 7:08 PM, Tejun Heo <tj@kernel.org> wrote:
> On Wed, Aug 23, 2017 at 04:47:41PM +0200, Geert Uytterhoeven wrote:
>> Some code is mixing up multiple memory nodes with multiple cpu nodes.
>> M68k uses DISCONTIGMEM, but  not NUMA (no SMP):
>>
>>     config NEED_MULTIPLE_NODES
>>         def_bool y
>>         depends on DISCONTIGMEM || NUMA
>>
>> The (virtual) Atari has 2 memory nodes:
>>   - node0: start 0x00000000 size 0x00e00000
>>   - node1: start 0x01000000 size 0x10000000
>> Both are tied to the same (single) CPU node, of course.
>
> Ah, okay, so it has multiple nodes but not NUMA.  The generic numa
> topology code assumes that there's only one node if !NUMA and reports
> all online cpus regardless of the node number, which makes the same
> CPUs to be reported for all nodes on the system.  I think something
> like the following (completely untested) should work.

Thank you, that got rid of the warning.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1719320

Hello,

On Thu, Aug 24, 2017 at 03:32:04PM +0200, Geert Uytterhoeven wrote:
> > Ah, okay, so it has multiple nodes but not NUMA.  The generic numa
> > topology code assumes that there's only one node if !NUMA and reports
> > all online cpus regardless of the node number, which makes the same
> > CPUs to be reported for all nodes on the system.  I think something
> > like the following (completely untested) should work.
> 
> Thank you, that got rid of the warning.

Great, it turns out we already have cpu_none_mask.  Can you please
test the following works too?

Thanks.

diff --git a/include/asm-generic/topology.h b/include/asm-generic/topology.h
index fc824e2..5d2add1 100644
--- a/include/asm-generic/topology.h
+++ b/include/asm-generic/topology.h
@@ -48,7 +48,11 @@
 #define parent_node(node)	((void)(node),0)
 #endif
 #ifndef cpumask_of_node
-#define cpumask_of_node(node)	((void)node, cpu_online_mask)
+  #ifdef CONFIG_NEED_MULTIPLE_NODES
+    #define cpumask_of_node(node)	((node) == 0 ? cpu_online_mask : cpu_none_mask)
+  #else
+    #define cpumask_of_node(node)	((void)node, cpu_online_mask)
+  #endif
 #endif
 #ifndef pcibus_to_node
 #define pcibus_to_node(bus)	((void)(bus), -1)

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web