Re: [PATCH] mm/sparse: Make sparse_init_one_section void and remove check

Re: [PATCH] mm/sparse: Make sparse_init_one_section void and remove check

[Ross Zwisler]

On Mon, Jul 2, 2018 at 12:48 PM Pavel Tatashin
<pasha.tatashin@oracle.com> wrote:
>
> On Mon, Jul 2, 2018 at 11:43 AM <osalvador@techadventures.net> wrote:
> >
> > From: Oscar Salvador <osalvador@suse.de>
> >
> > sparse_init_one_section() is being called from two sites:
> > sparse_init() and sparse_add_one_section().
> > The former calls it from a for_each_present_section_nr() loop,
> > and the latter marks the section as present before calling it.
> > This means that when sparse_init_one_section() gets called, we already know
> > that the section is present.
> > So there is no point to double check that in the function.
> >
> > This removes the check and makes the function void.
> >
> > Signed-off-by: Oscar Salvador <osalvador@suse.de>
>
> Thank you Oscar.
>
> Reviewed-by: Pavel Tatashin <pasha.tatashin@oracle.com>

It looks like this change breaks "fsdax" mode namespaces in
next-20180705.  The offending commit is:

commit 054620849110 ("mm/sparse.c: make sparse_init_one_section void
and remove check")

Here is the stack trace I get when converting a raw mode namespace to
fsdax mode, and from then on during each reboot as the namespace is
being initialized:

[    6.067166] BUG: unable to handle kernel paging request at ffffea0005000080
[    6.068084] PGD 13ffdd067 P4D 13ffdd067 PUD 13ffdc067 PMD 0
[    6.068771] Oops: 0002 [#1] PREEMPT SMP PTI
[    6.069262] CPU: 11 PID: 180 Comm: kworker/u24:2 Not tainted
4.18.0-rc3-00193-g054620849110 #1
[    6.070440] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
BIOS rel-1.11.1-0-g0551a4be2c-prebuilt.qemu-project.org 04/01/2014
[    6.071689] Workqueue: events_unbound async_run_entry_fn
[    6.072261] RIP: 0010:memmap_init_zone+0x154/0x1cf
[    6.072882] Code: 48 89 c3 48 c1 eb 0c e9 82 00 00 00 48 89 da 48
b8 00 00 00 00 00 ea ff ff b9 10 00 00 00 48 c1 e2 06 48 01 c2 31 c0
48 89 d7 <f3> ab 48 b8 ff ff ff ff ff ff 7f 00 48 23 45 c0 c7 42 34 01
00 00
[    6.075396] RSP: 0018:ffffc900024bfa70 EFLAGS: 00010246
[    6.076052] RAX: 0000000000000000 RBX: 0000000000140002 RCX: 0000000000000010
[    6.076845] RDX: ffffea0005000080 RSI: 0000000000000000 RDI: ffffea0005000080
[    6.077604] RBP: ffffc900024bfab0 R08: 0000000000000001 R09: ffff88010eb50d38
[    6.078394] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
[    6.079331] R13: 0000000000000004 R14: 0000000000000001 R15: 0000000000000000
[    6.080274] FS:  0000000000000000(0000) GS:ffff880115a00000(0000)
knlGS:0000000000000000
[    6.081337] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    6.082092] CR2: ffffea0005000080 CR3: 0000000002824000 CR4: 00000000000006e0
[    6.083032] Call Trace:
[    6.083371]  move_pfn_range_to_zone+0x168/0x180
[    6.083965]  devm_memremap_pages+0x29b/0x480
[    6.084550]  pmem_attach_disk+0x1ae/0x6c0 [nd_pmem]
[    6.085204]  ? devm_memremap+0x79/0xb0
[    6.085714]  nd_pmem_probe+0x7e/0xa0 [nd_pmem]
[    6.086320]  nvdimm_bus_probe+0x6e/0x160 [libnvdimm]
[    6.086977]  driver_probe_device+0x310/0x480
[    6.087543]  __device_attach_driver+0x86/0x100
[    6.088136]  ? __driver_attach+0x110/0x110
[    6.088681]  bus_for_each_drv+0x6e/0xb0
[    6.089190]  __device_attach+0xe2/0x160
[    6.089705]  device_initial_probe+0x13/0x20
[    6.090266]  bus_probe_device+0xa6/0xc0
[    6.090772]  device_add+0x41b/0x660
[    6.091249]  ? lock_acquire+0xa3/0x210
[    6.091743]  nd_async_device_register+0x12/0x40 [libnvdimm]
[    6.092398]  async_run_entry_fn+0x3e/0x170
[    6.092921]  process_one_work+0x230/0x680
[    6.093455]  worker_thread+0x3f/0x3b0
[    6.093930]  kthread+0x12f/0x150
[    6.094362]  ? process_one_work+0x680/0x680
[    6.094903]  ? kthread_create_worker_on_cpu+0x70/0x70
[    6.095574]  ret_from_fork+0x3a/0x50
[    6.096069] Modules linked in: nd_pmem nd_btt dax_pmem device_dax
nfit libnvdimm
[    6.097179] CR2: ffffea0005000080
[    6.097764] ---[ end trace a5b8bd6a5500b68c ]---

- Ross
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH] mm/sparse.c: fix error path in sparse_add_one_section

[Ross Zwisler]

The following commit in -next:

commit 054620849110 ("mm/sparse.c: make sparse_init_one_section void and
remove check")

changed how the error handling in sparse_add_one_section() works.

Previously sparse_index_init() could return -EEXIST, and the function would
continue on happily.  'ret' would get unconditionally overwritten by the
result from sparse_init_one_section() and the error code after the 'out:'
label wouldn't be triggered.

With the above referenced commit, though, an -EEXIST error return from
sparse_index_init() now takes us through the function and into the error
case after 'out:'.  This eventually causes a kernel BUG, probably because
we've just freed a memory section that we successfully set up and marked as
present:

  BUG: unable to handle kernel paging request at ffffea0005000080
  RIP: 0010:memmap_init_zone+0x154/0x1cf

  Call Trace:
   move_pfn_range_to_zone+0x168/0x180
   devm_memremap_pages+0x29b/0x480
   pmem_attach_disk+0x1ae/0x6c0 [nd_pmem]
   ? devm_memremap+0x79/0xb0
   nd_pmem_probe+0x7e/0xa0 [nd_pmem]
   nvdimm_bus_probe+0x6e/0x160 [libnvdimm]
   driver_probe_device+0x310/0x480
   __device_attach_driver+0x86/0x100
   ? __driver_attach+0x110/0x110
   bus_for_each_drv+0x6e/0xb0
   __device_attach+0xe2/0x160
   device_initial_probe+0x13/0x20
   bus_probe_device+0xa6/0xc0
   device_add+0x41b/0x660
   ? lock_acquire+0xa3/0x210
   nd_async_device_register+0x12/0x40 [libnvdimm]
   async_run_entry_fn+0x3e/0x170
   process_one_work+0x230/0x680
   worker_thread+0x3f/0x3b0
   kthread+0x12f/0x150
   ? process_one_work+0x680/0x680
   ? kthread_create_worker_on_cpu+0x70/0x70
   ret_from_fork+0x3a/0x50

Fix this by clearing 'ret' back to 0 if sparse_index_init() returns
-EEXIST.  This restores the previous behavior.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 mm/sparse.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 9574113fc745..d254bd2d3289 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -753,8 +753,12 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
 	 * plus, it does a kmalloc
 	 */
 	ret = sparse_index_init(section_nr, pgdat->node_id);
-	if (ret < 0 && ret != -EEXIST)
-		return ret;
+	if (ret < 0) {
+		if (ret == -EEXIST)
+			ret = 0;
+		else
+			return ret;
+	}
 	memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, altmap);
 	if (!memmap)
 		return -ENOMEM;
-- 
2.14.4

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] mm/sparse.c: fix error path in sparse_add_one_section

[Oscar Salvador]

On Fri, Jul 06, 2018 at 01:06:58PM -0600, Ross Zwisler wrote:
> The following commit in -next:
> 
> commit 054620849110 ("mm/sparse.c: make sparse_init_one_section void and
> remove check")
> 
> changed how the error handling in sparse_add_one_section() works.
> 
> Previously sparse_index_init() could return -EEXIST, and the function would
> continue on happily.  'ret' would get unconditionally overwritten by the
> result from sparse_init_one_section() and the error code after the 'out:'
> label wouldn't be triggered.

My bad, I missed that.

> diff --git a/mm/sparse.c b/mm/sparse.c
> index 9574113fc745..d254bd2d3289 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -753,8 +753,12 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  	 * plus, it does a kmalloc
>  	 */
>  	ret = sparse_index_init(section_nr, pgdat->node_id);
> -	if (ret < 0 && ret != -EEXIST)
> -		return ret;
> +	if (ret < 0) {
> +		if (ret == -EEXIST)
> +			ret = 0;
> +		else
> +			return ret;
> +	}

sparse_index_init() can return:

-ENOMEM, -EEXIST or 0.

So what about this?:

diff --git a/mm/sparse.c b/mm/sparse.c
index f55e79fda03e..eb188eb6b82d 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -770,6 +770,7 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
        ret = sparse_index_init(section_nr, pgdat->node_id);
        if (ret < 0 && ret != -EEXIST)
                return ret;
+       ret = 0;

Does this look more clean?
-- 
Oscar Salvador
SUSE L3
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] mm/sparse.c: fix error path in sparse_add_one_section

[Ross Zwisler]

On Fri, Jul 06, 2018 at 11:23:27PM +0200, Oscar Salvador wrote:
> On Fri, Jul 06, 2018 at 01:06:58PM -0600, Ross Zwisler wrote:
> > The following commit in -next:
> > 
> > commit 054620849110 ("mm/sparse.c: make sparse_init_one_section void and
> > remove check")
> > 
> > changed how the error handling in sparse_add_one_section() works.
> > 
> > Previously sparse_index_init() could return -EEXIST, and the function would
> > continue on happily.  'ret' would get unconditionally overwritten by the
> > result from sparse_init_one_section() and the error code after the 'out:'
> > label wouldn't be triggered.
> 
> My bad, I missed that.
> 
> > diff --git a/mm/sparse.c b/mm/sparse.c
> > index 9574113fc745..d254bd2d3289 100644
> > --- a/mm/sparse.c
> > +++ b/mm/sparse.c
> > @@ -753,8 +753,12 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
> >  	 * plus, it does a kmalloc
> >  	 */
> >  	ret = sparse_index_init(section_nr, pgdat->node_id);
> > -	if (ret < 0 && ret != -EEXIST)
> > -		return ret;
> > +	if (ret < 0) {
> > +		if (ret == -EEXIST)
> > +			ret = 0;
> > +		else
> > +			return ret;
> > +	}
> 
> sparse_index_init() can return:
> 
> -ENOMEM, -EEXIST or 0.
> 
> So what about this?:
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index f55e79fda03e..eb188eb6b82d 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -770,6 +770,7 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>         ret = sparse_index_init(section_nr, pgdat->node_id);
>         if (ret < 0 && ret != -EEXIST)
>                 return ret;
> +       ret = 0;
> 
> Does this look more clean?

Sure, that's probably better.

Andrew, what's the easiest way forward?  I can send out a v2, you can fold
this into his previous patch, or something else?
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] mm/sparse.c: fix error path in sparse_add_one_section

[Andrew Morton]

On Fri, 6 Jul 2018 15:54:37 -0600 Ross Zwisler <ross.zwisler@linux.intel.com> wrote:

> On Fri, Jul 06, 2018 at 11:23:27PM +0200, Oscar Salvador wrote:
> > On Fri, Jul 06, 2018 at 01:06:58PM -0600, Ross Zwisler wrote:
> > > The following commit in -next:
> > > 
> > > commit 054620849110 ("mm/sparse.c: make sparse_init_one_section void and
> > > remove check")
> > > 
> > > changed how the error handling in sparse_add_one_section() works.
> > > 
> > > Previously sparse_index_init() could return -EEXIST, and the function would
> > > continue on happily.  'ret' would get unconditionally overwritten by the
> > > result from sparse_init_one_section() and the error code after the 'out:'
> > > label wouldn't be triggered.
> > 
> > My bad, I missed that.
> > 
> > > diff --git a/mm/sparse.c b/mm/sparse.c
> > > index 9574113fc745..d254bd2d3289 100644
> > > --- a/mm/sparse.c
> > > +++ b/mm/sparse.c
> > > @@ -753,8 +753,12 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
> > >  	 * plus, it does a kmalloc
> > >  	 */
> > >  	ret = sparse_index_init(section_nr, pgdat->node_id);
> > > -	if (ret < 0 && ret != -EEXIST)
> > > -		return ret;
> > > +	if (ret < 0) {
> > > +		if (ret == -EEXIST)
> > > +			ret = 0;
> > > +		else
> > > +			return ret;
> > > +	}
> > 
> > sparse_index_init() can return:
> > 
> > -ENOMEM, -EEXIST or 0.
> > 
> > So what about this?:
> > 
> > diff --git a/mm/sparse.c b/mm/sparse.c
> > index f55e79fda03e..eb188eb6b82d 100644
> > --- a/mm/sparse.c
> > +++ b/mm/sparse.c
> > @@ -770,6 +770,7 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
> >         ret = sparse_index_init(section_nr, pgdat->node_id);
> >         if (ret < 0 && ret != -EEXIST)
> >                 return ret;
> > +       ret = 0;
> > 
> > Does this look more clean?
> 
> Sure, that's probably better.
> 
> Andrew, what's the easiest way forward?  I can send out a v2, you can fold
> this into his previous patch, or something else?

Whatever ;)  v2 works.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH] mm/sparse.c: fix error path in sparse_add_one_section

[Andrew Morton]

On Fri,  6 Jul 2018 13:06:58 -0600 Ross Zwisler <ross.zwisler@linux.intel.com> wrote:

> commit 054620849110 ("mm/sparse.c: make sparse_init_one_section void and
> remove check")
> 
> changed how the error handling in sparse_add_one_section() works.
> 
> Previously sparse_index_init() could return -EEXIST, and the function would
> continue on happily.  'ret' would get unconditionally overwritten by the
> result from sparse_init_one_section() and the error code after the 'out:'
> label wouldn't be triggered.
> 
> With the above referenced commit, though, an -EEXIST error return from
> sparse_index_init() now takes us through the function and into the error
> case after 'out:'.  This eventually causes a kernel BUG, probably because
> we've just freed a memory section that we successfully set up and marked as
> present:

Thanks.

And gee it would be nice if some of this code was commented.  I
*assume* what's happening with that -EEXIST is that
sparse_add_one_section() is discovering that the root mem_section was
already initialized so things are OK.  Maybe.  My mind-reading skills
aren't so good on Fridays.

And sparse_index_init() sure looks like it needs locking to avoid races
around mem_section[root].  Or perhaps we're known to be single-threaded
here.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

[PATCH v2] mm/sparse.c: fix error path in sparse_add_one_section

[Ross Zwisler]

The following commit in -next:

commit 054620849110 ("mm/sparse.c: make sparse_init_one_section void and
remove check")

changed how the error handling in sparse_add_one_section() works.

Previously sparse_index_init() could return -EEXIST, and the function would
continue on happily.  'ret' would get unconditionally overwritten by the
result from sparse_init_one_section() and the error code after the 'out:'
label wouldn't be triggered.

With the above referenced commit, though, an -EEXIST error return from
sparse_index_init() now takes us through the function and into the error
case after 'out:'.  This eventually causes a kernel BUG, probably because
we've just freed a memory section that we successfully set up and marked as
present:

  BUG: unable to handle kernel paging request at ffffea0005000080
  RIP: 0010:memmap_init_zone+0x154/0x1cf

  Call Trace:
   move_pfn_range_to_zone+0x168/0x180
   devm_memremap_pages+0x29b/0x480
   pmem_attach_disk+0x1ae/0x6c0 [nd_pmem]
   ? devm_memremap+0x79/0xb0
   nd_pmem_probe+0x7e/0xa0 [nd_pmem]
   nvdimm_bus_probe+0x6e/0x160 [libnvdimm]
   driver_probe_device+0x310/0x480
   __device_attach_driver+0x86/0x100
   ? __driver_attach+0x110/0x110
   bus_for_each_drv+0x6e/0xb0
   __device_attach+0xe2/0x160
   device_initial_probe+0x13/0x20
   bus_probe_device+0xa6/0xc0
   device_add+0x41b/0x660
   ? lock_acquire+0xa3/0x210
   nd_async_device_register+0x12/0x40 [libnvdimm]
   async_run_entry_fn+0x3e/0x170
   process_one_work+0x230/0x680
   worker_thread+0x3f/0x3b0
   kthread+0x12f/0x150
   ? process_one_work+0x680/0x680
   ? kthread_create_worker_on_cpu+0x70/0x70
   ret_from_fork+0x3a/0x50

Fix this by clearing 'ret' back to 0 if sparse_index_init() returns
-EEXIST.  This restores the previous behavior.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 mm/sparse.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/sparse.c b/mm/sparse.c
index f55e79fda03e..eb188eb6b82d 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -770,6 +770,7 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
 	ret = sparse_index_init(section_nr, pgdat->node_id);
 	if (ret < 0 && ret != -EEXIST)
 		return ret;
+	ret = 0;
 	memmap = kmalloc_section_memmap(section_nr, pgdat->node_id, altmap);
 	if (!memmap)
 		return -ENOMEM;
-- 
2.14.4

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v2] mm/sparse.c: fix error path in sparse_add_one_section

[Oscar Salvador]

On Fri, Jul 06, 2018 at 04:33:58PM -0600, Ross Zwisler wrote:
> The following commit in -next:
> 
> commit 054620849110 ("mm/sparse.c: make sparse_init_one_section void and
> remove check")
> 
> changed how the error handling in sparse_add_one_section() works.
> 
> Previously sparse_index_init() could return -EEXIST, and the function would
> continue on happily.  'ret' would get unconditionally overwritten by the
> result from sparse_init_one_section() and the error code after the 'out:'
> label wouldn't be triggered.
> 
> With the above referenced commit, though, an -EEXIST error return from
> sparse_index_init() now takes us through the function and into the error
> case after 'out:'.  This eventually causes a kernel BUG, probably because
> we've just freed a memory section that we successfully set up and marked as
> present:
> 
>   BUG: unable to handle kernel paging request at ffffea0005000080
>   RIP: 0010:memmap_init_zone+0x154/0x1cf
> 
>   Call Trace:
>    move_pfn_range_to_zone+0x168/0x180
>    devm_memremap_pages+0x29b/0x480
>    pmem_attach_disk+0x1ae/0x6c0 [nd_pmem]
>    ? devm_memremap+0x79/0xb0
>    nd_pmem_probe+0x7e/0xa0 [nd_pmem]
>    nvdimm_bus_probe+0x6e/0x160 [libnvdimm]
>    driver_probe_device+0x310/0x480
>    __device_attach_driver+0x86/0x100
>    ? __driver_attach+0x110/0x110
>    bus_for_each_drv+0x6e/0xb0
>    __device_attach+0xe2/0x160
>    device_initial_probe+0x13/0x20
>    bus_probe_device+0xa6/0xc0
>    device_add+0x41b/0x660
>    ? lock_acquire+0xa3/0x210
>    nd_async_device_register+0x12/0x40 [libnvdimm]
>    async_run_entry_fn+0x3e/0x170
>    process_one_work+0x230/0x680
>    worker_thread+0x3f/0x3b0
>    kthread+0x12f/0x150
>    ? process_one_work+0x680/0x680
>    ? kthread_create_worker_on_cpu+0x70/0x70
>    ret_from_fork+0x3a/0x50
> 
> Fix this by clearing 'ret' back to 0 if sparse_index_init() returns
> -EEXIST.  This restores the previous behavior.
> 
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>

Reviewed-by: Oscar Salvador <osalvador@suse.de>
-- 
Oscar Salvador
SUSE L3
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm