All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mm/sparse: Make sparse_init_one_section void and remove check
@ 2018-07-02 15:43 osalvador
  2018-07-02 16:05 ` Michal Hocko
  2018-07-02 18:47 ` Pavel Tatashin
  0 siblings, 2 replies; 19+ messages in thread
From: osalvador @ 2018-07-02 15:43 UTC (permalink / raw)
  To: akpm
  Cc: pasha.tatashin, mhocko, vbabka, bhe, kirill.shutemov,
	dave.hansen, linux-mm, linux-kernel, Oscar Salvador

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>
---
 mm/sparse.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index b2848cc6e32a..f55e79fda03e 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -264,19 +264,14 @@ struct page *sparse_decode_mem_map(unsigned long coded_mem_map, unsigned long pn
 	return ((struct page *)coded_mem_map) + section_nr_to_pfn(pnum);
 }
 
-static int __meminit sparse_init_one_section(struct mem_section *ms,
+static void __meminit sparse_init_one_section(struct mem_section *ms,
 		unsigned long pnum, struct page *mem_map,
 		unsigned long *pageblock_bitmap)
 {
-	if (!present_section(ms))
-		return -EINVAL;
-
 	ms->section_mem_map &= ~SECTION_MAP_MASK;
 	ms->section_mem_map |= sparse_encode_mem_map(mem_map, pnum) |
 							SECTION_HAS_MEM_MAP;
  	ms->pageblock_flags = pageblock_bitmap;
-
-	return 1;
 }
 
 unsigned long usemap_size(void)
@@ -801,12 +796,11 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
 #endif
 
 	section_mark_present(ms);
-
-	ret = sparse_init_one_section(ms, section_nr, memmap, usemap);
+	sparse_init_one_section(ms, section_nr, memmap, usemap);
 
 out:
 	pgdat_resize_unlock(pgdat, &flags);
-	if (ret <= 0) {
+	if (ret < 0) {
 		kfree(usemap);
 		__kfree_section_memmap(memmap, altmap);
 	}
-- 
2.13.6


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] mm/sparse: Make sparse_init_one_section void and remove check
  2018-07-02 15:43 [PATCH] mm/sparse: Make sparse_init_one_section void and remove check osalvador
@ 2018-07-02 16:05 ` Michal Hocko
  2018-07-02 18:47 ` Pavel Tatashin
  1 sibling, 0 replies; 19+ messages in thread
From: Michal Hocko @ 2018-07-02 16:05 UTC (permalink / raw)
  To: osalvador
  Cc: akpm, pasha.tatashin, vbabka, bhe, kirill.shutemov, dave.hansen,
	linux-mm, linux-kernel, Oscar Salvador

On Mon 02-07-18 17:43:25, 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.

Looks good.

> Signed-off-by: Oscar Salvador <osalvador@suse.de>

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/sparse.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index b2848cc6e32a..f55e79fda03e 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -264,19 +264,14 @@ struct page *sparse_decode_mem_map(unsigned long coded_mem_map, unsigned long pn
>  	return ((struct page *)coded_mem_map) + section_nr_to_pfn(pnum);
>  }
>  
> -static int __meminit sparse_init_one_section(struct mem_section *ms,
> +static void __meminit sparse_init_one_section(struct mem_section *ms,
>  		unsigned long pnum, struct page *mem_map,
>  		unsigned long *pageblock_bitmap)
>  {
> -	if (!present_section(ms))
> -		return -EINVAL;
> -
>  	ms->section_mem_map &= ~SECTION_MAP_MASK;
>  	ms->section_mem_map |= sparse_encode_mem_map(mem_map, pnum) |
>  							SECTION_HAS_MEM_MAP;
>   	ms->pageblock_flags = pageblock_bitmap;
> -
> -	return 1;
>  }
>  
>  unsigned long usemap_size(void)
> @@ -801,12 +796,11 @@ int __meminit sparse_add_one_section(struct pglist_data *pgdat,
>  #endif
>  
>  	section_mark_present(ms);
> -
> -	ret = sparse_init_one_section(ms, section_nr, memmap, usemap);
> +	sparse_init_one_section(ms, section_nr, memmap, usemap);
>  
>  out:
>  	pgdat_resize_unlock(pgdat, &flags);
> -	if (ret <= 0) {
> +	if (ret < 0) {
>  		kfree(usemap);
>  		__kfree_section_memmap(memmap, altmap);
>  	}
> -- 
> 2.13.6
> 

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] mm/sparse: Make sparse_init_one_section void and remove check
  2018-07-02 15:43 [PATCH] mm/sparse: Make sparse_init_one_section void and remove check osalvador
  2018-07-02 16:05 ` Michal Hocko
@ 2018-07-02 18:47 ` Pavel Tatashin
  2018-07-06 18:23     ` Ross Zwisler
  1 sibling, 1 reply; 19+ messages in thread
From: Pavel Tatashin @ 2018-07-02 18:47 UTC (permalink / raw)
  To: osalvador
  Cc: Andrew Morton, Michal Hocko, Vlastimil Babka, bhe,
	kirill.shutemov, dave.hansen, Linux Memory Management List, LKML,
	osalvador

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>

> ---
>  mm/sparse.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] mm/sparse: Make sparse_init_one_section void and remove check
  2018-07-02 18:47 ` Pavel Tatashin
@ 2018-07-06 18:23     ` Ross Zwisler
  0 siblings, 0 replies; 19+ messages in thread
From: Ross Zwisler @ 2018-07-06 18:23 UTC (permalink / raw)
  To: pasha.tatashin, linux-nvdimm
  Cc: osalvador, bhe, Dave Hansen, LKML, Linux MM, Michal Hocko,
	Vlastimil Babka, Andrew Morton, Kirill A. Shutemov, osalvador

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] mm/sparse: Make sparse_init_one_section void and remove check
@ 2018-07-06 18:23     ` Ross Zwisler
  0 siblings, 0 replies; 19+ messages in thread
From: Ross Zwisler @ 2018-07-06 18:23 UTC (permalink / raw)
  To: pasha.tatashin, linux-nvdimm
  Cc: osalvador, Andrew Morton, Michal Hocko, Vlastimil Babka, bhe,
	Kirill A. Shutemov, Dave Hansen, Linux MM, LKML, osalvador

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH] mm/sparse.c: fix error path in sparse_add_one_section
  2018-07-06 18:23     ` Ross Zwisler
@ 2018-07-06 19:06       ` Ross Zwisler
  -1 siblings, 0 replies; 19+ messages in thread
From: Ross Zwisler @ 2018-07-06 19:06 UTC (permalink / raw)
  To: pasha.tatashin, linux-nvdimm
  Cc: osalvador, bhe, Dave Hansen, LKML, Linux MM, Michal Hocko,
	Kirill A. Shutemov, Andrew Morton, Vlastimil Babka, osalvador

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH] mm/sparse.c: fix error path in sparse_add_one_section
@ 2018-07-06 19:06       ` Ross Zwisler
  0 siblings, 0 replies; 19+ messages in thread
From: Ross Zwisler @ 2018-07-06 19:06 UTC (permalink / raw)
  To: pasha.tatashin, linux-nvdimm
  Cc: Ross Zwisler, osalvador, bhe, Dave Hansen, LKML, Linux MM,
	Michal Hocko, Vlastimil Babka, Andrew Morton, Kirill A. Shutemov,
	osalvador

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] mm/sparse.c: fix error path in sparse_add_one_section
  2018-07-06 19:06       ` Ross Zwisler
@ 2018-07-06 21:23         ` Oscar Salvador
  -1 siblings, 0 replies; 19+ messages in thread
From: Oscar Salvador @ 2018-07-06 21:23 UTC (permalink / raw)
  To: Ross Zwisler
  Cc: Michal Hocko, bhe, linux-nvdimm, Dave Hansen, LKML,
	pasha.tatashin, Linux MM, Kirill A. Shutemov, Andrew Morton,
	Vlastimil Babka, osalvador

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] mm/sparse.c: fix error path in sparse_add_one_section
@ 2018-07-06 21:23         ` Oscar Salvador
  0 siblings, 0 replies; 19+ messages in thread
From: Oscar Salvador @ 2018-07-06 21:23 UTC (permalink / raw)
  To: Ross Zwisler
  Cc: pasha.tatashin, linux-nvdimm, bhe, Dave Hansen, LKML, Linux MM,
	Michal Hocko, Vlastimil Babka, Andrew Morton, Kirill A. Shutemov,
	osalvador

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] mm/sparse.c: fix error path in sparse_add_one_section
  2018-07-06 19:06       ` Ross Zwisler
@ 2018-07-06 21:32         ` Andrew Morton
  -1 siblings, 0 replies; 19+ messages in thread
From: Andrew Morton @ 2018-07-06 21:32 UTC (permalink / raw)
  To: Ross Zwisler
  Cc: osalvador, bhe, linux-nvdimm, Dave Hansen, LKML, pasha.tatashin,
	Linux MM, Michal Hocko, Kirill A. Shutemov, Vlastimil Babka,
	osalvador

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] mm/sparse.c: fix error path in sparse_add_one_section
@ 2018-07-06 21:32         ` Andrew Morton
  0 siblings, 0 replies; 19+ messages in thread
From: Andrew Morton @ 2018-07-06 21:32 UTC (permalink / raw)
  To: Ross Zwisler
  Cc: pasha.tatashin, linux-nvdimm, osalvador, bhe, Dave Hansen, LKML,
	Linux MM, Michal Hocko, Vlastimil Babka, Kirill A. Shutemov,
	osalvador

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] mm/sparse.c: fix error path in sparse_add_one_section
  2018-07-06 21:23         ` Oscar Salvador
@ 2018-07-06 21:54           ` Ross Zwisler
  -1 siblings, 0 replies; 19+ messages in thread
From: Ross Zwisler @ 2018-07-06 21:54 UTC (permalink / raw)
  To: Oscar Salvador
  Cc: Michal Hocko, bhe, linux-nvdimm, Dave Hansen, LKML,
	pasha.tatashin, Linux MM, Kirill A. Shutemov, Andrew Morton,
	Vlastimil Babka, osalvador

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] mm/sparse.c: fix error path in sparse_add_one_section
@ 2018-07-06 21:54           ` Ross Zwisler
  0 siblings, 0 replies; 19+ messages in thread
From: Ross Zwisler @ 2018-07-06 21:54 UTC (permalink / raw)
  To: Oscar Salvador
  Cc: Ross Zwisler, pasha.tatashin, linux-nvdimm, bhe, Dave Hansen,
	LKML, Linux MM, Michal Hocko, Vlastimil Babka, Andrew Morton,
	Kirill A. Shutemov, osalvador

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?

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] mm/sparse.c: fix error path in sparse_add_one_section
  2018-07-06 21:54           ` Ross Zwisler
@ 2018-07-06 21:58             ` Andrew Morton
  -1 siblings, 0 replies; 19+ messages in thread
From: Andrew Morton @ 2018-07-06 21:58 UTC (permalink / raw)
  To: Ross Zwisler
  Cc: Oscar Salvador, bhe, linux-nvdimm, Dave Hansen, LKML,
	pasha.tatashin, Linux MM, Michal Hocko, Kirill A. Shutemov,
	Vlastimil Babka, osalvador

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] mm/sparse.c: fix error path in sparse_add_one_section
@ 2018-07-06 21:58             ` Andrew Morton
  0 siblings, 0 replies; 19+ messages in thread
From: Andrew Morton @ 2018-07-06 21:58 UTC (permalink / raw)
  To: Ross Zwisler
  Cc: Oscar Salvador, pasha.tatashin, linux-nvdimm, bhe, Dave Hansen,
	LKML, Linux MM, Michal Hocko, Vlastimil Babka,
	Kirill A. Shutemov, osalvador

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2] mm/sparse.c: fix error path in sparse_add_one_section
  2018-07-06 19:06       ` Ross Zwisler
@ 2018-07-06 22:33         ` Ross Zwisler
  -1 siblings, 0 replies; 19+ messages in thread
From: Ross Zwisler @ 2018-07-06 22:33 UTC (permalink / raw)
  To: pasha.tatashin, linux-nvdimm
  Cc: osalvador, bhe, Dave Hansen, LKML, Linux MM, Michal Hocko,
	Kirill A. Shutemov, Andrew Morton, Vlastimil Babka, osalvador

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2] mm/sparse.c: fix error path in sparse_add_one_section
@ 2018-07-06 22:33         ` Ross Zwisler
  0 siblings, 0 replies; 19+ messages in thread
From: Ross Zwisler @ 2018-07-06 22:33 UTC (permalink / raw)
  To: pasha.tatashin, linux-nvdimm
  Cc: Ross Zwisler, osalvador, bhe, Dave Hansen, LKML, Linux MM,
	Michal Hocko, Vlastimil Babka, Andrew Morton, Kirill A. Shutemov,
	osalvador

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] mm/sparse.c: fix error path in sparse_add_one_section
  2018-07-06 22:33         ` Ross Zwisler
@ 2018-07-07  6:01           ` Oscar Salvador
  -1 siblings, 0 replies; 19+ messages in thread
From: Oscar Salvador @ 2018-07-07  6:01 UTC (permalink / raw)
  To: Ross Zwisler
  Cc: Michal Hocko, bhe, linux-nvdimm, Dave Hansen, LKML,
	pasha.tatashin, Linux MM, Kirill A. Shutemov, Andrew Morton,
	Vlastimil Babka, osalvador

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] mm/sparse.c: fix error path in sparse_add_one_section
@ 2018-07-07  6:01           ` Oscar Salvador
  0 siblings, 0 replies; 19+ messages in thread
From: Oscar Salvador @ 2018-07-07  6:01 UTC (permalink / raw)
  To: Ross Zwisler
  Cc: pasha.tatashin, linux-nvdimm, bhe, Dave Hansen, LKML, Linux MM,
	Michal Hocko, Vlastimil Babka, Andrew Morton, Kirill A. Shutemov,
	osalvador

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2018-07-07  6:02 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-02 15:43 [PATCH] mm/sparse: Make sparse_init_one_section void and remove check osalvador
2018-07-02 16:05 ` Michal Hocko
2018-07-02 18:47 ` Pavel Tatashin
2018-07-06 18:23   ` Ross Zwisler
2018-07-06 18:23     ` Ross Zwisler
2018-07-06 19:06     ` [PATCH] mm/sparse.c: fix error path in sparse_add_one_section Ross Zwisler
2018-07-06 19:06       ` Ross Zwisler
2018-07-06 21:23       ` Oscar Salvador
2018-07-06 21:23         ` Oscar Salvador
2018-07-06 21:54         ` Ross Zwisler
2018-07-06 21:54           ` Ross Zwisler
2018-07-06 21:58           ` Andrew Morton
2018-07-06 21:58             ` Andrew Morton
2018-07-06 21:32       ` Andrew Morton
2018-07-06 21:32         ` Andrew Morton
2018-07-06 22:33       ` [PATCH v2] " Ross Zwisler
2018-07-06 22:33         ` Ross Zwisler
2018-07-07  6:01         ` Oscar Salvador
2018-07-07  6:01           ` Oscar Salvador

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.