a few minor fixes and improvements
Dong Aisheng (5):
mm: correct SECTION_SHIFT name in code comments
mm/sparse: free section usage memory in case populate_section_memmap
failed
mm/sparse: move mem_sections allocation out of memory_present()
mm: rename the global section array to mem_sections
mm/page_alloc: improve memmap_pages and dma_reserve dbg msg
include/linux/mmzone.h | 12 +++++-----
include/linux/page-flags-layout.h | 3 +++
kernel/crash_core.c | 4 ++--
mm/page_alloc.c | 5 +++--
mm/sparse.c | 37 +++++++++++++++----------------
5 files changed, 31 insertions(+), 30 deletions(-)
--
2.25.1
Actually SECTIONS_SHIFT is used in the kernel code, fixed the code
comments. BTW, also moved the code comment to where it's defined.
Also fixed a checkpatch complain derived from the original code:
WARNING: please, no space before tabs
+ * SECTIONS_SHIFT ^I^I#bits space required to store a section #$
Cc: Andrew Morton <[email protected]>
Cc: Yu Zhao <[email protected]>
Cc: Andrey Konovalov <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Kees Cook <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
include/linux/mmzone.h | 2 --
include/linux/page-flags-layout.h | 3 +++
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 9cdc88d09f2b..fc23e36cb165 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -1198,8 +1198,6 @@ static inline struct zoneref *first_zones_zonelist(struct zonelist *zonelist,
#ifdef CONFIG_SPARSEMEM
/*
- * SECTION_SHIFT #bits space required to store a section #
- *
* PA_SECTION_SHIFT physical address to/from section number
* PFN_SECTION_SHIFT pfn to/from section number
*/
diff --git a/include/linux/page-flags-layout.h b/include/linux/page-flags-layout.h
index ef1e3e736e14..aff616855492 100644
--- a/include/linux/page-flags-layout.h
+++ b/include/linux/page-flags-layout.h
@@ -26,6 +26,9 @@
#define ZONES_WIDTH ZONES_SHIFT
+/*
+ * SECTIONS_SHIFT #bits space required to store a section #
+ */
#ifdef CONFIG_SPARSEMEM
#include <asm/sparsemem.h>
#define SECTIONS_SHIFT (MAX_PHYSMEM_BITS - SECTION_SIZE_BITS)
--
2.25.1
Make debug message more accurately.
Cc: Andrew Morton <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
mm/page_alloc.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3100fcb08500..16f494352f58 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -7263,14 +7263,15 @@ static void __init free_area_init_core(struct pglist_data *pgdat)
pr_debug(" %s zone: %lu pages used for memmap\n",
zone_names[j], memmap_pages);
} else
- pr_warn(" %s zone: %lu pages exceeds freesize %lu\n",
+ pr_warn(" %s zone: %lu memmap pages exceeds freesize %lu\n",
zone_names[j], memmap_pages, freesize);
}
/* Account for reserved pages */
if (j == 0 && freesize > dma_reserve) {
freesize -= dma_reserve;
- pr_debug(" %s zone: %lu pages reserved\n", zone_names[0], dma_reserve);
+ pr_debug(" %s zone: %lu pages reserved for dma\n",
+ zone_names[0], dma_reserve);
}
if (!is_highmem_idx(j))
--
2.25.1
In order to distinguish the struct mem_section for a better code
readability and align with kernel doc [1] name below, change the
global mem section name to 'mem_sections' from 'mem_section'.
[1] Documentation/vm/memory-model.rst
"The `mem_section` objects are arranged in a two-dimensional array
called `mem_sections`."
Cc: Andrew Morton <[email protected]>
Cc: Dave Young <[email protected]>
Cc: Baoquan He <[email protected]>
Cc: Vivek Goyal <[email protected]>
Cc: [email protected]
Signed-off-by: Dong Aisheng <[email protected]>
---
include/linux/mmzone.h | 10 +++++-----
kernel/crash_core.c | 4 ++--
mm/sparse.c | 16 ++++++++--------
3 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index fc23e36cb165..b348a06915c5 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -1297,9 +1297,9 @@ struct mem_section {
#define SECTION_ROOT_MASK (SECTIONS_PER_ROOT - 1)
#ifdef CONFIG_SPARSEMEM_EXTREME
-extern struct mem_section **mem_section;
+extern struct mem_section **mem_sections;
#else
-extern struct mem_section mem_section[NR_SECTION_ROOTS][SECTIONS_PER_ROOT];
+extern struct mem_section mem_sections[NR_SECTION_ROOTS][SECTIONS_PER_ROOT];
#endif
static inline unsigned long *section_to_usemap(struct mem_section *ms)
@@ -1310,12 +1310,12 @@ static inline unsigned long *section_to_usemap(struct mem_section *ms)
static inline struct mem_section *__nr_to_section(unsigned long nr)
{
#ifdef CONFIG_SPARSEMEM_EXTREME
- if (!mem_section)
+ if (!mem_sections)
return NULL;
#endif
- if (!mem_section[SECTION_NR_TO_ROOT(nr)])
+ if (!mem_sections[SECTION_NR_TO_ROOT(nr)])
return NULL;
- return &mem_section[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK];
+ return &mem_sections[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK];
}
extern unsigned long __section_nr(struct mem_section *ms);
extern size_t mem_section_usage_size(void);
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 29cc15398ee4..fb1180d81b5a 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -414,8 +414,8 @@ static int __init crash_save_vmcoreinfo_init(void)
VMCOREINFO_SYMBOL(contig_page_data);
#endif
#ifdef CONFIG_SPARSEMEM
- VMCOREINFO_SYMBOL_ARRAY(mem_section);
- VMCOREINFO_LENGTH(mem_section, NR_SECTION_ROOTS);
+ VMCOREINFO_SYMBOL_ARRAY(mem_sections);
+ VMCOREINFO_LENGTH(mem_sections, NR_SECTION_ROOTS);
VMCOREINFO_STRUCT_SIZE(mem_section);
VMCOREINFO_OFFSET(mem_section, section_mem_map);
VMCOREINFO_NUMBER(MAX_PHYSMEM_BITS);
diff --git a/mm/sparse.c b/mm/sparse.c
index df4418c12f04..a96e7e65475f 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -24,12 +24,12 @@
* 1) mem_section - memory sections, mem_map's for valid memory
*/
#ifdef CONFIG_SPARSEMEM_EXTREME
-struct mem_section **mem_section;
+struct mem_section **mem_sections;
#else
-struct mem_section mem_section[NR_SECTION_ROOTS][SECTIONS_PER_ROOT]
+struct mem_section mem_sections[NR_SECTION_ROOTS][SECTIONS_PER_ROOT]
____cacheline_internodealigned_in_smp;
#endif
-EXPORT_SYMBOL(mem_section);
+EXPORT_SYMBOL(mem_sections);
#ifdef NODE_NOT_IN_PAGE_FLAGS
/*
@@ -91,14 +91,14 @@ static int __meminit sparse_index_init(unsigned long section_nr, int nid)
*
* The mem_hotplug_lock resolves the apparent race below.
*/
- if (mem_section[root])
+ if (mem_sections[root])
return 0;
section = sparse_index_alloc(nid);
if (!section)
return -ENOMEM;
- mem_section[root] = section;
+ mem_sections[root] = section;
return 0;
}
@@ -131,7 +131,7 @@ unsigned long __section_nr(struct mem_section *ms)
#else
unsigned long __section_nr(struct mem_section *ms)
{
- return (unsigned long)(ms - mem_section[0]);
+ return (unsigned long)(ms - mem_sections[0]);
}
#endif
@@ -286,8 +286,8 @@ static void __init memblocks_present(void)
#ifdef CONFIG_SPARSEMEM_EXTREME
size = sizeof(struct mem_section *) * NR_SECTION_ROOTS;
align = 1 << (INTERNODE_CACHE_SHIFT);
- mem_section = memblock_alloc(size, align);
- if (!mem_section)
+ mem_sections = memblock_alloc(size, align);
+ if (!mem_sections)
panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
__func__, size, align);
#endif
--
2.25.1
The only path to call memory_present() is from memblocks_present().
The struct mem_section **mem_section only needs to be initialized once,
so no need put the initialization/allocation code in memory_present()
which will be called multiple times for each section.
After moving, the 'unlikely' condition statement becomes to be
meaningless as it's only initialized one time, so dropped as well.
Cc: Andrew Morton <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
mm/sparse.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
diff --git a/mm/sparse.c b/mm/sparse.c
index 98bfacc763da..df4418c12f04 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -254,19 +254,6 @@ static void __init memory_present(int nid, unsigned long start, unsigned long en
{
unsigned long pfn;
-#ifdef CONFIG_SPARSEMEM_EXTREME
- if (unlikely(!mem_section)) {
- unsigned long size, align;
-
- size = sizeof(struct mem_section *) * NR_SECTION_ROOTS;
- align = 1 << (INTERNODE_CACHE_SHIFT);
- mem_section = memblock_alloc(size, align);
- if (!mem_section)
- panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
- __func__, size, align);
- }
-#endif
-
start &= PAGE_SECTION_MASK;
mminit_validate_memmodel_limits(&start, &end);
for (pfn = start; pfn < end; pfn += PAGES_PER_SECTION) {
@@ -292,9 +279,19 @@ static void __init memory_present(int nid, unsigned long start, unsigned long en
*/
static void __init memblocks_present(void)
{
+ unsigned long __maybe_unused size, align;
unsigned long start, end;
int i, nid;
+#ifdef CONFIG_SPARSEMEM_EXTREME
+ size = sizeof(struct mem_section *) * NR_SECTION_ROOTS;
+ align = 1 << (INTERNODE_CACHE_SHIFT);
+ mem_section = memblock_alloc(size, align);
+ if (!mem_section)
+ panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
+ __func__, size, align);
+#endif
+
for_each_mem_pfn_range(i, MAX_NUMNODES, &start, &end, &nid)
memory_present(nid, start, end);
}
--
2.25.1
Free section usage memory in case populate_section_memmap failed.
We use map_count to track the remain unused memory to be freed.
Cc: Andrew Morton <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
mm/sparse.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/mm/sparse.c b/mm/sparse.c
index 7ac481353b6b..98bfacc763da 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -549,12 +549,14 @@ static void __init sparse_init_nid(int nid, unsigned long pnum_begin,
__func__, nid);
pnum_begin = pnum;
sparse_buffer_fini();
+ memblock_free_early(__pa(usage), map_count * mem_section_usage_size());
goto failed;
}
check_usemap_section_nr(nid, usage);
sparse_init_one_section(__nr_to_section(pnum), pnum, map, usage,
SECTION_IS_EARLY);
usage = (void *) usage + mem_section_usage_size();
+ map_count--;
}
sparse_buffer_fini();
return;
--
2.25.1
On Mon, May 17, 2021 at 5:21 AM Dong Aisheng <[email protected]> wrote:
>
> Actually SECTIONS_SHIFT is used in the kernel code, fixed the code
> comments. BTW, also moved the code comment to where it's defined.
>
> Also fixed a checkpatch complain derived from the original code:
> WARNING: please, no space before tabs
> + * SECTIONS_SHIFT ^I^I#bits space required to store a section #$
>
> Cc: Andrew Morton <[email protected]>
> Cc: Yu Zhao <[email protected]>
> Cc: Andrey Konovalov <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Kees Cook <[email protected]>
> Signed-off-by: Dong Aisheng <[email protected]>
> ---
> include/linux/mmzone.h | 2 --
> include/linux/page-flags-layout.h | 3 +++
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 9cdc88d09f2b..fc23e36cb165 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -1198,8 +1198,6 @@ static inline struct zoneref *first_zones_zonelist(struct zonelist *zonelist,
> #ifdef CONFIG_SPARSEMEM
>
> /*
> - * SECTION_SHIFT #bits space required to store a section #
> - *
> * PA_SECTION_SHIFT physical address to/from section number
> * PFN_SECTION_SHIFT pfn to/from section number
> */
> diff --git a/include/linux/page-flags-layout.h b/include/linux/page-flags-layout.h
> index ef1e3e736e14..aff616855492 100644
> --- a/include/linux/page-flags-layout.h
> +++ b/include/linux/page-flags-layout.h
> @@ -26,6 +26,9 @@
>
> #define ZONES_WIDTH ZONES_SHIFT
>
> +/*
> + * SECTIONS_SHIFT #bits space required to store a section #
> + */
IMO, we should either make the original comment helpful or just remove
it. Moving it here doesn't improve readability because it's stating
the obvious.
> #ifdef CONFIG_SPARSEMEM
> #include <asm/sparsemem.h>
> #define SECTIONS_SHIFT (MAX_PHYSMEM_BITS - SECTION_SIZE_BITS)
> --
> 2.25.1
>
> From: Yu Zhao <[email protected]>
> Sent: Tuesday, May 18, 2021 1:18 AM
>
> On Mon, May 17, 2021 at 5:21 AM Dong Aisheng <[email protected]>
> wrote:
> >
> > Actually SECTIONS_SHIFT is used in the kernel code, fixed the code
> > comments. BTW, also moved the code comment to where it's defined.
> >
> > Also fixed a checkpatch complain derived from the original code:
> > WARNING: please, no space before tabs
> > + * SECTIONS_SHIFT ^I^I#bits space required to store a section #$
> >
> > Cc: Andrew Morton <[email protected]>
> > Cc: Yu Zhao <[email protected]>
> > Cc: Andrey Konovalov <[email protected]>
> > Cc: Catalin Marinas <[email protected]>
> > Cc: Kees Cook <[email protected]>
> > Signed-off-by: Dong Aisheng <[email protected]>
> > ---
> > include/linux/mmzone.h | 2 --
> > include/linux/page-flags-layout.h | 3 +++
> > 2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h index
> > 9cdc88d09f2b..fc23e36cb165 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -1198,8 +1198,6 @@ static inline struct zoneref
> > *first_zones_zonelist(struct zonelist *zonelist, #ifdef
> > CONFIG_SPARSEMEM
> >
> > /*
> > - * SECTION_SHIFT #bits space required to store a section #
> > - *
> > * PA_SECTION_SHIFT physical address to/from section
> number
> > * PFN_SECTION_SHIFT pfn to/from section number
> > */
> > diff --git a/include/linux/page-flags-layout.h
> > b/include/linux/page-flags-layout.h
> > index ef1e3e736e14..aff616855492 100644
> > --- a/include/linux/page-flags-layout.h
> > +++ b/include/linux/page-flags-layout.h
> > @@ -26,6 +26,9 @@
> >
> > #define ZONES_WIDTH ZONES_SHIFT
> >
> > +/*
> > + * SECTIONS_SHIFT #bits space required to store a section #
> > + */
>
> IMO, we should either make the original comment helpful or just remove it.
> Moving it here doesn't improve readability because it's stating the obvious.
>
Sounds good to me.
If not objections, I will remove it in v2 later.
Thanks for the suggestion.
Regards
Aisheng
> > #ifdef CONFIG_SPARSEMEM
> > #include <asm/sparsemem.h>
> > #define SECTIONS_SHIFT (MAX_PHYSMEM_BITS - SECTION_SIZE_BITS)
> > --
> > 2.25.1
> >
On Mon, May 17, 2021 at 07:20:42PM +0800, Dong Aisheng wrote:
> The only path to call memory_present() is from memblocks_present().
> The struct mem_section **mem_section only needs to be initialized once,
> so no need put the initialization/allocation code in memory_present()
> which will be called multiple times for each section.
>
> After moving, the 'unlikely' condition statement becomes to be
> meaningless as it's only initialized one time, so dropped as well.
>
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: Dong Aisheng <[email protected]>
> ---
> mm/sparse.c | 23 ++++++++++-------------
> 1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 98bfacc763da..df4418c12f04 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -254,19 +254,6 @@ static void __init memory_present(int nid, unsigned long start, unsigned long en
> {
> unsigned long pfn;
>
> -#ifdef CONFIG_SPARSEMEM_EXTREME
> - if (unlikely(!mem_section)) {
> - unsigned long size, align;
> -
> - size = sizeof(struct mem_section *) * NR_SECTION_ROOTS;
> - align = 1 << (INTERNODE_CACHE_SHIFT);
> - mem_section = memblock_alloc(size, align);
> - if (!mem_section)
> - panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> - __func__, size, align);
> - }
> -#endif
Maybe split this into a helper function and call it directly from
sparse_init()?
> -
> start &= PAGE_SECTION_MASK;
> mminit_validate_memmodel_limits(&start, &end);
> for (pfn = start; pfn < end; pfn += PAGES_PER_SECTION) {
> @@ -292,9 +279,19 @@ static void __init memory_present(int nid, unsigned long start, unsigned long en
> */
> static void __init memblocks_present(void)
> {
> + unsigned long __maybe_unused size, align;
> unsigned long start, end;
> int i, nid;
>
> +#ifdef CONFIG_SPARSEMEM_EXTREME
> + size = sizeof(struct mem_section *) * NR_SECTION_ROOTS;
> + align = 1 << (INTERNODE_CACHE_SHIFT);
> + mem_section = memblock_alloc(size, align);
> + if (!mem_section)
> + panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> + __func__, size, align);
> +#endif
> +
> for_each_mem_pfn_range(i, MAX_NUMNODES, &start, &end, &nid)
> memory_present(nid, start, end);
> }
> --
> 2.25.1
>
>
--
Sincerely yours,
Mike.
On Mon, May 17, 2021 at 07:20:41PM +0800, Dong Aisheng wrote:
> Free section usage memory in case populate_section_memmap failed.
> We use map_count to track the remain unused memory to be freed.
>
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: Dong Aisheng <[email protected]>
> ---
> mm/sparse.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 7ac481353b6b..98bfacc763da 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -549,12 +549,14 @@ static void __init sparse_init_nid(int nid, unsigned long pnum_begin,
> __func__, nid);
> pnum_begin = pnum;
> sparse_buffer_fini();
> + memblock_free_early(__pa(usage), map_count * mem_section_usage_size());
I'd move both sparse_buffer_fini() and freeing of 'usage' memory after the
failed label.
> goto failed;
> }
> check_usemap_section_nr(nid, usage);
> sparse_init_one_section(__nr_to_section(pnum), pnum, map, usage,
> SECTION_IS_EARLY);
> usage = (void *) usage + mem_section_usage_size();
> + map_count--;
> }
> sparse_buffer_fini();
> return;
> --
> 2.25.1
>
>
--
Sincerely yours,
Mike.
On Tue, May 18, 2021 at 6:09 PM Mike Rapoport <[email protected]> wrote:
>
> On Mon, May 17, 2021 at 07:20:41PM +0800, Dong Aisheng wrote:
> > Free section usage memory in case populate_section_memmap failed.
> > We use map_count to track the remain unused memory to be freed.
> >
> > Cc: Andrew Morton <[email protected]>
> > Signed-off-by: Dong Aisheng <[email protected]>
> > ---
> > mm/sparse.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/mm/sparse.c b/mm/sparse.c
> > index 7ac481353b6b..98bfacc763da 100644
> > --- a/mm/sparse.c
> > +++ b/mm/sparse.c
> > @@ -549,12 +549,14 @@ static void __init sparse_init_nid(int nid, unsigned long pnum_begin,
> > __func__, nid);
> > pnum_begin = pnum;
> > sparse_buffer_fini();
> > + memblock_free_early(__pa(usage), map_count * mem_section_usage_size());
>
> I'd move both sparse_buffer_fini() and freeing of 'usage' memory after the
> failed label.
>
Doing that needs to introduce another 'failed' label.
Do you think if it's necessary?
e.g.
diff --git a/mm/sparse.c b/mm/sparse.c
index 7ac481353b6b..408b737e168e 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -533,7 +533,7 @@ static void __init sparse_init_nid(int nid,
unsigned long pnum_begin,
mem_section_usage_size() * map_count);
if (!usage) {
pr_err("%s: node[%d] usemap allocation failed", __func__, nid);
- goto failed;
+ goto failed1;
}
sparse_buffer_init(map_count * section_map_size(), nid);
for_each_present_section_nr(pnum_begin, pnum) {
@@ -548,17 +548,20 @@ static void __init sparse_init_nid(int nid,
unsigned long pnum_begin,
pr_err("%s: node[%d] memory map backing
failed. Some memory will not be available.",
__func__, nid);
pnum_begin = pnum;
- sparse_buffer_fini();
- goto failed;
+ goto failed2;
}
check_usemap_section_nr(nid, usage);
sparse_init_one_section(__nr_to_section(pnum), pnum, map, usage,
SECTION_IS_EARLY);
usage = (void *) usage + mem_section_usage_size();
+ map_count--;
}
sparse_buffer_fini();
return;
-failed:
+failed2:
+ sparse_buffer_fini();
+ memblock_free_early(__pa(usage), map_count * mem_section_usage_size());
+failed1:
/* We failed to allocate, mark all the following pnums as not present */
for_each_present_section_nr(pnum_begin, pnum) {
struct mem_section *ms;
Regards
Aisheng
> > goto failed;
> > }
> > check_usemap_section_nr(nid, usage);
> > sparse_init_one_section(__nr_to_section(pnum), pnum, map, usage,
> > SECTION_IS_EARLY);
> > usage = (void *) usage + mem_section_usage_size();
> > + map_count--;
> > }
> > sparse_buffer_fini();
> > return;
> > --
> > 2.25.1
> >
> >
>
> --
> Sincerely yours,
> Mike.
On Tue, May 18, 2021 at 6:12 PM Mike Rapoport <[email protected]> wrote:
>
> On Mon, May 17, 2021 at 07:20:42PM +0800, Dong Aisheng wrote:
> > The only path to call memory_present() is from memblocks_present().
> > The struct mem_section **mem_section only needs to be initialized once,
> > so no need put the initialization/allocation code in memory_present()
> > which will be called multiple times for each section.
> >
> > After moving, the 'unlikely' condition statement becomes to be
> > meaningless as it's only initialized one time, so dropped as well.
> >
> > Cc: Andrew Morton <[email protected]>
> > Signed-off-by: Dong Aisheng <[email protected]>
> > ---
> > mm/sparse.c | 23 ++++++++++-------------
> > 1 file changed, 10 insertions(+), 13 deletions(-)
> >
> > diff --git a/mm/sparse.c b/mm/sparse.c
> > index 98bfacc763da..df4418c12f04 100644
> > --- a/mm/sparse.c
> > +++ b/mm/sparse.c
> > @@ -254,19 +254,6 @@ static void __init memory_present(int nid, unsigned long start, unsigned long en
> > {
> > unsigned long pfn;
> >
> > -#ifdef CONFIG_SPARSEMEM_EXTREME
> > - if (unlikely(!mem_section)) {
> > - unsigned long size, align;
> > -
> > - size = sizeof(struct mem_section *) * NR_SECTION_ROOTS;
> > - align = 1 << (INTERNODE_CACHE_SHIFT);
> > - mem_section = memblock_alloc(size, align);
> > - if (!mem_section)
> > - panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> > - __func__, size, align);
> > - }
> > -#endif
>
> Maybe split this into a helper function and call it directly from
> sparse_init()?
>
Sounds good to me
e.g. sparse_alloc_section_root() called directly from sparse_init().
Thanks for the suggestion
Regards
Aisheng
> > -
> > start &= PAGE_SECTION_MASK;
> > mminit_validate_memmodel_limits(&start, &end);
> > for (pfn = start; pfn < end; pfn += PAGES_PER_SECTION) {
> > @@ -292,9 +279,19 @@ static void __init memory_present(int nid, unsigned long start, unsigned long en
> > */
> > static void __init memblocks_present(void)
> > {
> > + unsigned long __maybe_unused size, align;
> > unsigned long start, end;
> > int i, nid;
> >
> > +#ifdef CONFIG_SPARSEMEM_EXTREME
> > + size = sizeof(struct mem_section *) * NR_SECTION_ROOTS;
> > + align = 1 << (INTERNODE_CACHE_SHIFT);
> > + mem_section = memblock_alloc(size, align);
> > + if (!mem_section)
> > + panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> > + __func__, size, align);
> > +#endif
> > +
> > for_each_mem_pfn_range(i, MAX_NUMNODES, &start, &end, &nid)
> > memory_present(nid, start, end);
> > }
> > --
> > 2.25.1
> >
> >
>
> --
> Sincerely yours,
> Mike.
On Tue, May 18, 2021 at 06:25:28PM +0800, Dong Aisheng wrote:
> On Tue, May 18, 2021 at 6:09 PM Mike Rapoport <[email protected]> wrote:
> >
> > On Mon, May 17, 2021 at 07:20:41PM +0800, Dong Aisheng wrote:
> > > Free section usage memory in case populate_section_memmap failed.
> > > We use map_count to track the remain unused memory to be freed.
> > >
> > > Cc: Andrew Morton <[email protected]>
> > > Signed-off-by: Dong Aisheng <[email protected]>
> > > ---
> > > mm/sparse.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/mm/sparse.c b/mm/sparse.c
> > > index 7ac481353b6b..98bfacc763da 100644
> > > --- a/mm/sparse.c
> > > +++ b/mm/sparse.c
> > > @@ -549,12 +549,14 @@ static void __init sparse_init_nid(int nid, unsigned long pnum_begin,
> > > __func__, nid);
> > > pnum_begin = pnum;
> > > sparse_buffer_fini();
> > > + memblock_free_early(__pa(usage), map_count * mem_section_usage_size());
> >
> > I'd move both sparse_buffer_fini() and freeing of 'usage' memory after the
> > failed label.
> >
>
> Doing that needs to introduce another 'failed' label.
> Do you think if it's necessary?
In general, it's preferred way of error handling:
https://www.kernel.org/doc/html/latest/process/coding-style.html#centralized-exiting-of-functions
> e.g.
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 7ac481353b6b..408b737e168e 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -533,7 +533,7 @@ static void __init sparse_init_nid(int nid,
> unsigned long pnum_begin,
> mem_section_usage_size() * map_count);
> if (!usage) {
> pr_err("%s: node[%d] usemap allocation failed", __func__, nid);
> - goto failed;
> + goto failed1;
> }
> sparse_buffer_init(map_count * section_map_size(), nid);
> for_each_present_section_nr(pnum_begin, pnum) {
> @@ -548,17 +548,20 @@ static void __init sparse_init_nid(int nid,
> unsigned long pnum_begin,
> pr_err("%s: node[%d] memory map backing
> failed. Some memory will not be available.",
> __func__, nid);
> pnum_begin = pnum;
> - sparse_buffer_fini();
> - goto failed;
> + goto failed2;
> }
> check_usemap_section_nr(nid, usage);
> sparse_init_one_section(__nr_to_section(pnum), pnum, map, usage,
> SECTION_IS_EARLY);
> usage = (void *) usage + mem_section_usage_size();
> + map_count--;
> }
> sparse_buffer_fini();
> return;
> -failed:
> +failed2:
> + sparse_buffer_fini();
> + memblock_free_early(__pa(usage), map_count * mem_section_usage_size());
> +failed1:
> /* We failed to allocate, mark all the following pnums as not present */
> for_each_present_section_nr(pnum_begin, pnum) {
> struct mem_section *ms;
>
> Regards
> Aisheng
> > > goto failed;
> > > }
> > > check_usemap_section_nr(nid, usage);
> > > sparse_init_one_section(__nr_to_section(pnum), pnum, map, usage,
> > > SECTION_IS_EARLY);
> > > usage = (void *) usage + mem_section_usage_size();
> > > + map_count--;
> > > }
> > > sparse_buffer_fini();
> > > return;
> > > --
> > > 2.25.1
> > >
> > >
> >
> > --
> > Sincerely yours,
> > Mike.
--
Sincerely yours,
Mike.
On Tue, May 18, 2021 at 7:43 PM Mike Rapoport <[email protected]> wrote:
>
> On Tue, May 18, 2021 at 06:25:28PM +0800, Dong Aisheng wrote:
> > On Tue, May 18, 2021 at 6:09 PM Mike Rapoport <[email protected]> wrote:
> > >
> > > On Mon, May 17, 2021 at 07:20:41PM +0800, Dong Aisheng wrote:
> > > > Free section usage memory in case populate_section_memmap failed.
> > > > We use map_count to track the remain unused memory to be freed.
> > > >
> > > > Cc: Andrew Morton <[email protected]>
> > > > Signed-off-by: Dong Aisheng <[email protected]>
> > > > ---
> > > > mm/sparse.c | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/mm/sparse.c b/mm/sparse.c
> > > > index 7ac481353b6b..98bfacc763da 100644
> > > > --- a/mm/sparse.c
> > > > +++ b/mm/sparse.c
> > > > @@ -549,12 +549,14 @@ static void __init sparse_init_nid(int nid, unsigned long pnum_begin,
> > > > __func__, nid);
> > > > pnum_begin = pnum;
> > > > sparse_buffer_fini();
> > > > + memblock_free_early(__pa(usage), map_count * mem_section_usage_size());
> > >
> > > I'd move both sparse_buffer_fini() and freeing of 'usage' memory after the
> > > failed label.
> > >
> >
> > Doing that needs to introduce another 'failed' label.
> > Do you think if it's necessary?
>
> In general, it's preferred way of error handling:
>
Okay, if no objections, i will do it in V2.
Thanks
Regards
Aisheng
> https://www.kernel.org/doc/html/latest/process/coding-style.html#centralized-exiting-of-functions
>
> > e.g.
> > diff --git a/mm/sparse.c b/mm/sparse.c
> > index 7ac481353b6b..408b737e168e 100644
> > --- a/mm/sparse.c
> > +++ b/mm/sparse.c
> > @@ -533,7 +533,7 @@ static void __init sparse_init_nid(int nid,
> > unsigned long pnum_begin,
> > mem_section_usage_size() * map_count);
> > if (!usage) {
> > pr_err("%s: node[%d] usemap allocation failed", __func__, nid);
> > - goto failed;
> > + goto failed1;
> > }
> > sparse_buffer_init(map_count * section_map_size(), nid);
> > for_each_present_section_nr(pnum_begin, pnum) {
> > @@ -548,17 +548,20 @@ static void __init sparse_init_nid(int nid,
> > unsigned long pnum_begin,
> > pr_err("%s: node[%d] memory map backing
> > failed. Some memory will not be available.",
> > __func__, nid);
> > pnum_begin = pnum;
> > - sparse_buffer_fini();
> > - goto failed;
> > + goto failed2;
> > }
> > check_usemap_section_nr(nid, usage);
> > sparse_init_one_section(__nr_to_section(pnum), pnum, map, usage,
> > SECTION_IS_EARLY);
> > usage = (void *) usage + mem_section_usage_size();
> > + map_count--;
> > }
> > sparse_buffer_fini();
> > return;
> > -failed:
> > +failed2:
> > + sparse_buffer_fini();
> > + memblock_free_early(__pa(usage), map_count * mem_section_usage_size());
> > +failed1:
> > /* We failed to allocate, mark all the following pnums as not present */
> > for_each_present_section_nr(pnum_begin, pnum) {
> > struct mem_section *ms;
> >
> > Regards
> > Aisheng
> > > > goto failed;
> > > > }
> > > > check_usemap_section_nr(nid, usage);
> > > > sparse_init_one_section(__nr_to_section(pnum), pnum, map, usage,
> > > > SECTION_IS_EARLY);
> > > > usage = (void *) usage + mem_section_usage_size();
> > > > + map_count--;
> > > > }
> > > > sparse_buffer_fini();
> > > > return;
> > > > --
> > > > 2.25.1
> > > >
> > > >
> > >
> > > --
> > > Sincerely yours,
> > > Mike.
>
> --
> Sincerely yours,
> Mike.
On 17.05.21 13:20, Dong Aisheng wrote:
> Free section usage memory in case populate_section_memmap failed.
> We use map_count to track the remain unused memory to be freed.
>
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: Dong Aisheng <[email protected]>
> ---
> mm/sparse.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 7ac481353b6b..98bfacc763da 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -549,12 +549,14 @@ static void __init sparse_init_nid(int nid, unsigned long pnum_begin,
> __func__, nid);
> pnum_begin = pnum;
> sparse_buffer_fini();
> + memblock_free_early(__pa(usage), map_count * mem_section_usage_size());
> goto failed;
> }
> check_usemap_section_nr(nid, usage);
> sparse_init_one_section(__nr_to_section(pnum), pnum, map, usage,
> SECTION_IS_EARLY);
> usage = (void *) usage + mem_section_usage_size();
> + map_count--;
> }
> sparse_buffer_fini();
> return;
>
Why care about optimizing something that literally never fails, and if
it would fail, leave the system in a sub-optimal, mostly unusable state?
--
Thanks,
David / dhildenb
On 17.05.21 13:20, Dong Aisheng wrote:
> Make debug message more accurately.
>
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: Dong Aisheng <[email protected]>
> ---
> mm/page_alloc.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 3100fcb08500..16f494352f58 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -7263,14 +7263,15 @@ static void __init free_area_init_core(struct pglist_data *pgdat)
> pr_debug(" %s zone: %lu pages used for memmap\n",
> zone_names[j], memmap_pages);
> } else
> - pr_warn(" %s zone: %lu pages exceeds freesize %lu\n",
> + pr_warn(" %s zone: %lu memmap pages exceeds freesize %lu\n",
> zone_names[j], memmap_pages, freesize);
> }
>
> /* Account for reserved pages */
> if (j == 0 && freesize > dma_reserve) {
> freesize -= dma_reserve;
> - pr_debug(" %s zone: %lu pages reserved\n", zone_names[0], dma_reserve);
> + pr_debug(" %s zone: %lu pages reserved for dma\n",
> + zone_names[0], dma_reserve);
... which is not really correct I think. See the comment above
set_dma_reserve(). It's called dma_reserve because it involves the first
zone -- where many unfreeable allocations like the kernel image end up.
Memory is not reserved for dma purposes ... and the zone name should be
sufficient.
--
Thanks,
David / dhildenb
On Tue, May 25, 2021 at 3:55 PM David Hildenbrand <[email protected]> wrote:
>
> On 17.05.21 13:20, Dong Aisheng wrote:
> > In order to distinguish the struct mem_section for a better code
> > readability and align with kernel doc [1] name below, change the
> > global mem section name to 'mem_sections' from 'mem_section'.
> >
> > [1] Documentation/vm/memory-model.rst
> > "The `mem_section` objects are arranged in a two-dimensional array
> > called `mem_sections`."
> >
> > Cc: Andrew Morton <[email protected]>
> > Cc: Dave Young <[email protected]>
> > Cc: Baoquan He <[email protected]>
> > Cc: Vivek Goyal <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Dong Aisheng <[email protected]>
> > ---
> > include/linux/mmzone.h | 10 +++++-----
> > kernel/crash_core.c | 4 ++--
> > mm/sparse.c | 16 ++++++++--------
> > 3 files changed, 15 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> > index fc23e36cb165..b348a06915c5 100644
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -1297,9 +1297,9 @@ struct mem_section {
> > #define SECTION_ROOT_MASK (SECTIONS_PER_ROOT - 1)
> >
> > #ifdef CONFIG_SPARSEMEM_EXTREME
> > -extern struct mem_section **mem_section;
> > +extern struct mem_section **mem_sections;
> > #else
> > -extern struct mem_section mem_section[NR_SECTION_ROOTS][SECTIONS_PER_ROOT];
> > +extern struct mem_section mem_sections[NR_SECTION_ROOTS][SECTIONS_PER_ROOT];
> > #endif
> >
> > static inline unsigned long *section_to_usemap(struct mem_section *ms)
> > @@ -1310,12 +1310,12 @@ static inline unsigned long *section_to_usemap(struct mem_section *ms)
> > static inline struct mem_section *__nr_to_section(unsigned long nr)
> > {
> > #ifdef CONFIG_SPARSEMEM_EXTREME
> > - if (!mem_section)
> > + if (!mem_sections)
> > return NULL;
> > #endif
> > - if (!mem_section[SECTION_NR_TO_ROOT(nr)])
> > + if (!mem_sections[SECTION_NR_TO_ROOT(nr)])
> > return NULL;
> > - return &mem_section[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK];
> > + return &mem_sections[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK];
> > }
> > extern unsigned long __section_nr(struct mem_section *ms);
> > extern size_t mem_section_usage_size(void);
> > diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> > index 29cc15398ee4..fb1180d81b5a 100644
> > --- a/kernel/crash_core.c
> > +++ b/kernel/crash_core.c
> > @@ -414,8 +414,8 @@ static int __init crash_save_vmcoreinfo_init(void)
> > VMCOREINFO_SYMBOL(contig_page_data);
> > #endif
> > #ifdef CONFIG_SPARSEMEM
> > - VMCOREINFO_SYMBOL_ARRAY(mem_section);
> > - VMCOREINFO_LENGTH(mem_section, NR_SECTION_ROOTS);
> > + VMCOREINFO_SYMBOL_ARRAY(mem_sections);
> > + VMCOREINFO_LENGTH(mem_sections, NR_SECTION_ROOTS);
> > VMCOREINFO_STRUCT_SIZE(mem_section);
> > VMCOREINFO_OFFSET(mem_section, section_mem_map);
> > VMCOREINFO_NUMBER(MAX_PHYSMEM_BITS);
> > diff --git a/mm/sparse.c b/mm/sparse.c
> > index df4418c12f04..a96e7e65475f 100644
> > --- a/mm/sparse.c
> > +++ b/mm/sparse.c
> > @@ -24,12 +24,12 @@
> > * 1) mem_section - memory sections, mem_map's for valid memory
> > */
> > #ifdef CONFIG_SPARSEMEM_EXTREME
> > -struct mem_section **mem_section;
> > +struct mem_section **mem_sections;
> > #else
> > -struct mem_section mem_section[NR_SECTION_ROOTS][SECTIONS_PER_ROOT]
> > +struct mem_section mem_sections[NR_SECTION_ROOTS][SECTIONS_PER_ROOT]
> > ____cacheline_internodealigned_in_smp;
> > #endif
> > -EXPORT_SYMBOL(mem_section);
> > +EXPORT_SYMBOL(mem_sections);
> >
> > #ifdef NODE_NOT_IN_PAGE_FLAGS
> > /*
> > @@ -91,14 +91,14 @@ static int __meminit sparse_index_init(unsigned long section_nr, int nid)
> > *
> > * The mem_hotplug_lock resolves the apparent race below.
> > */
> > - if (mem_section[root])
> > + if (mem_sections[root])
> > return 0;
> >
> > section = sparse_index_alloc(nid);
> > if (!section)
> > return -ENOMEM;
> >
> > - mem_section[root] = section;
> > + mem_sections[root] = section;
> >
> > return 0;
> > }
> > @@ -131,7 +131,7 @@ unsigned long __section_nr(struct mem_section *ms)
> > #else
> > unsigned long __section_nr(struct mem_section *ms)
> > {
> > - return (unsigned long)(ms - mem_section[0]);
> > + return (unsigned long)(ms - mem_sections[0]);
> > }
> > #endif
> >
> > @@ -286,8 +286,8 @@ static void __init memblocks_present(void)
> > #ifdef CONFIG_SPARSEMEM_EXTREME
> > size = sizeof(struct mem_section *) * NR_SECTION_ROOTS;
> > align = 1 << (INTERNODE_CACHE_SHIFT);
> > - mem_section = memblock_alloc(size, align);
> > - if (!mem_section)
> > + mem_sections = memblock_alloc(size, align);
> > + if (!mem_sections)
> > panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> > __func__, size, align);
> > #endif
> >
>
> Smells like unnecessary code churn. I'd rather just fix the doc because
> it doesn't really improve readability IMHO.
>
In practice I feel it does affect readability that the global
mem_section pointer is mixed
with struct mem_section names which results in:
1. hard to quickly jump between global mem_section operation in a
single source file.
e.g. by VIM hotkey of search
2. hard to quickly identify all global mem_section operations in
global source files
by tools, e.g. cscope, grep
This does cause troubles if users want to quickly go through and
understand the code.
Regards
Aisheng
> Anyhow, just my 2 cents.
>
> --
> Thanks,
>
> David / dhildenb
>
On 17.05.21 13:20, Dong Aisheng wrote:
> In order to distinguish the struct mem_section for a better code
> readability and align with kernel doc [1] name below, change the
> global mem section name to 'mem_sections' from 'mem_section'.
>
> [1] Documentation/vm/memory-model.rst
> "The `mem_section` objects are arranged in a two-dimensional array
> called `mem_sections`."
>
> Cc: Andrew Morton <[email protected]>
> Cc: Dave Young <[email protected]>
> Cc: Baoquan He <[email protected]>
> Cc: Vivek Goyal <[email protected]>
> Cc: [email protected]
> Signed-off-by: Dong Aisheng <[email protected]>
> ---
> include/linux/mmzone.h | 10 +++++-----
> kernel/crash_core.c | 4 ++--
> mm/sparse.c | 16 ++++++++--------
> 3 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index fc23e36cb165..b348a06915c5 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -1297,9 +1297,9 @@ struct mem_section {
> #define SECTION_ROOT_MASK (SECTIONS_PER_ROOT - 1)
>
> #ifdef CONFIG_SPARSEMEM_EXTREME
> -extern struct mem_section **mem_section;
> +extern struct mem_section **mem_sections;
> #else
> -extern struct mem_section mem_section[NR_SECTION_ROOTS][SECTIONS_PER_ROOT];
> +extern struct mem_section mem_sections[NR_SECTION_ROOTS][SECTIONS_PER_ROOT];
> #endif
>
> static inline unsigned long *section_to_usemap(struct mem_section *ms)
> @@ -1310,12 +1310,12 @@ static inline unsigned long *section_to_usemap(struct mem_section *ms)
> static inline struct mem_section *__nr_to_section(unsigned long nr)
> {
> #ifdef CONFIG_SPARSEMEM_EXTREME
> - if (!mem_section)
> + if (!mem_sections)
> return NULL;
> #endif
> - if (!mem_section[SECTION_NR_TO_ROOT(nr)])
> + if (!mem_sections[SECTION_NR_TO_ROOT(nr)])
> return NULL;
> - return &mem_section[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK];
> + return &mem_sections[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK];
> }
> extern unsigned long __section_nr(struct mem_section *ms);
> extern size_t mem_section_usage_size(void);
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 29cc15398ee4..fb1180d81b5a 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -414,8 +414,8 @@ static int __init crash_save_vmcoreinfo_init(void)
> VMCOREINFO_SYMBOL(contig_page_data);
> #endif
> #ifdef CONFIG_SPARSEMEM
> - VMCOREINFO_SYMBOL_ARRAY(mem_section);
> - VMCOREINFO_LENGTH(mem_section, NR_SECTION_ROOTS);
> + VMCOREINFO_SYMBOL_ARRAY(mem_sections);
> + VMCOREINFO_LENGTH(mem_sections, NR_SECTION_ROOTS);
> VMCOREINFO_STRUCT_SIZE(mem_section);
> VMCOREINFO_OFFSET(mem_section, section_mem_map);
> VMCOREINFO_NUMBER(MAX_PHYSMEM_BITS);
> diff --git a/mm/sparse.c b/mm/sparse.c
> index df4418c12f04..a96e7e65475f 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -24,12 +24,12 @@
> * 1) mem_section - memory sections, mem_map's for valid memory
> */
> #ifdef CONFIG_SPARSEMEM_EXTREME
> -struct mem_section **mem_section;
> +struct mem_section **mem_sections;
> #else
> -struct mem_section mem_section[NR_SECTION_ROOTS][SECTIONS_PER_ROOT]
> +struct mem_section mem_sections[NR_SECTION_ROOTS][SECTIONS_PER_ROOT]
> ____cacheline_internodealigned_in_smp;
> #endif
> -EXPORT_SYMBOL(mem_section);
> +EXPORT_SYMBOL(mem_sections);
>
> #ifdef NODE_NOT_IN_PAGE_FLAGS
> /*
> @@ -91,14 +91,14 @@ static int __meminit sparse_index_init(unsigned long section_nr, int nid)
> *
> * The mem_hotplug_lock resolves the apparent race below.
> */
> - if (mem_section[root])
> + if (mem_sections[root])
> return 0;
>
> section = sparse_index_alloc(nid);
> if (!section)
> return -ENOMEM;
>
> - mem_section[root] = section;
> + mem_sections[root] = section;
>
> return 0;
> }
> @@ -131,7 +131,7 @@ unsigned long __section_nr(struct mem_section *ms)
> #else
> unsigned long __section_nr(struct mem_section *ms)
> {
> - return (unsigned long)(ms - mem_section[0]);
> + return (unsigned long)(ms - mem_sections[0]);
> }
> #endif
>
> @@ -286,8 +286,8 @@ static void __init memblocks_present(void)
> #ifdef CONFIG_SPARSEMEM_EXTREME
> size = sizeof(struct mem_section *) * NR_SECTION_ROOTS;
> align = 1 << (INTERNODE_CACHE_SHIFT);
> - mem_section = memblock_alloc(size, align);
> - if (!mem_section)
> + mem_sections = memblock_alloc(size, align);
> + if (!mem_sections)
> panic("%s: Failed to allocate %lu bytes align=0x%lx\n",
> __func__, size, align);
> #endif
>
Smells like unnecessary code churn. I'd rather just fix the doc because
it doesn't really improve readability IMHO.
Anyhow, just my 2 cents.
--
Thanks,
David / dhildenb
On Tue, May 25, 2021 at 3:52 PM David Hildenbrand <[email protected]> wrote:
>
> On 17.05.21 13:20, Dong Aisheng wrote:
> > Free section usage memory in case populate_section_memmap failed.
> > We use map_count to track the remain unused memory to be freed.
> >
> > Cc: Andrew Morton <[email protected]>
> > Signed-off-by: Dong Aisheng <[email protected]>
> > ---
> > mm/sparse.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/mm/sparse.c b/mm/sparse.c
> > index 7ac481353b6b..98bfacc763da 100644
> > --- a/mm/sparse.c
> > +++ b/mm/sparse.c
> > @@ -549,12 +549,14 @@ static void __init sparse_init_nid(int nid, unsigned long pnum_begin,
> > __func__, nid);
> > pnum_begin = pnum;
> > sparse_buffer_fini();
> > + memblock_free_early(__pa(usage), map_count * mem_section_usage_size());
> > goto failed;
> > }
> > check_usemap_section_nr(nid, usage);
> > sparse_init_one_section(__nr_to_section(pnum), pnum, map, usage,
> > SECTION_IS_EARLY);
> > usage = (void *) usage + mem_section_usage_size();
> > + map_count--;
> > }
> > sparse_buffer_fini();
> > return;
> >
>
> Why care about optimizing something that literally never fails, and if
> it would fail, leave the system in a sub-optimal, mostly unusable state?
>
It just fixes the code logic.
I'm going to listen to more comments, if everyone thinks it's not needed.
I may drop in V2.
Regards
Aisheng
> --
> Thanks,
>
> David / dhildenb
>
On Tue, May 25, 2021 at 4:01 PM David Hildenbrand <[email protected]> wrote:
>
> On 17.05.21 13:20, Dong Aisheng wrote:
> > Make debug message more accurately.
> >
> > Cc: Andrew Morton <[email protected]>
> > Signed-off-by: Dong Aisheng <[email protected]>
> > ---
> > mm/page_alloc.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 3100fcb08500..16f494352f58 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -7263,14 +7263,15 @@ static void __init free_area_init_core(struct pglist_data *pgdat)
> > pr_debug(" %s zone: %lu pages used for memmap\n",
> > zone_names[j], memmap_pages);
> > } else
> > - pr_warn(" %s zone: %lu pages exceeds freesize %lu\n",
> > + pr_warn(" %s zone: %lu memmap pages exceeds freesize %lu\n",
> > zone_names[j], memmap_pages, freesize);
> > }
> >
> > /* Account for reserved pages */
> > if (j == 0 && freesize > dma_reserve) {
> > freesize -= dma_reserve;
> > - pr_debug(" %s zone: %lu pages reserved\n", zone_names[0], dma_reserve);
> > + pr_debug(" %s zone: %lu pages reserved for dma\n",
> > + zone_names[0], dma_reserve);
>
> ... which is not really correct I think. See the comment above
> set_dma_reserve(). It's called dma_reserve because it involves the first
> zone -- where many unfreeable allocations like the kernel image end up.
>
> Memory is not reserved for dma purposes ... and the zone name should be
> sufficient.
You're right. I will drop this line of change.
Thanks
Regards
Aisheng
>
> --
> Thanks,
>
> David / dhildenb
>
>