| Message ID | 20180702154325.12196-1-osalvador@techadventures.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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(-)
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
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); }