[PATCH 0/5] mm/sparse: a few minor fixes and improvements

[PATCH 0/5] mm/sparse: a few minor fixes and improvements

[Dong Aisheng]

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

[PATCH 1/5] mm: correct SECTION_SHIFT name in code comments

[Dong Aisheng]

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 <akpm@linux-foundation.org>
Cc: Yu Zhao <yuzhao@google.com>
Cc: Andrey Konovalov <andreyknvl@gmail.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 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

Re: [PATCH 1/5] mm: correct SECTION_SHIFT name in code comments

[Yu Zhao]

On Mon, May 17, 2021 at 5:21 AM Dong Aisheng <aisheng.dong@nxp.com> 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 <akpm@linux-foundation.org>
> Cc: Yu Zhao <yuzhao@google.com>
> Cc: Andrey Konovalov <andreyknvl@gmail.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  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
>

RE: [PATCH 1/5] mm: correct SECTION_SHIFT name in code comments

[Aisheng Dong]

> From: Yu Zhao <yuzhao@google.com>
> Sent: Tuesday, May 18, 2021 1:18 AM
> 
> On Mon, May 17, 2021 at 5:21 AM Dong Aisheng <aisheng.dong@nxp.com>
> 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 <akpm@linux-foundation.org>
> > Cc: Yu Zhao <yuzhao@google.com>
> > Cc: Andrey Konovalov <andreyknvl@gmail.com>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  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
> >

[PATCH 2/5] mm/sparse: free section usage memory in case populate_section_memmap failed

[Dong Aisheng]

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 <akpm@linux-foundation.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 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

Re: [PATCH 2/5] mm/sparse: free section usage memory in case populate_section_memmap failed

[Mike Rapoport]

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 <akpm@linux-foundation.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  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.

Re: [PATCH 2/5] mm/sparse: free section usage memory in case populate_section_memmap failed

[Dong Aisheng]

On Tue, May 18, 2021 at 6:09 PM Mike Rapoport <rppt@kernel.org> 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 <akpm@linux-foundation.org>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  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.

Re: [PATCH 2/5] mm/sparse: free section usage memory in case populate_section_memmap failed

[Mike Rapoport]

On Tue, May 18, 2021 at 06:25:28PM +0800, Dong Aisheng wrote:
> On Tue, May 18, 2021 at 6:09 PM Mike Rapoport <rppt@kernel.org> 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 <akpm@linux-foundation.org>
> > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > ---
> > >  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.

Re: [PATCH 2/5] mm/sparse: free section usage memory in case populate_section_memmap failed

[Dong Aisheng]

On Tue, May 18, 2021 at 7:43 PM Mike Rapoport <rppt@kernel.org> 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 <rppt@kernel.org> 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 <akpm@linux-foundation.org>
> > > > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > > > ---
> > > >  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.

Re: [PATCH 2/5] mm/sparse: free section usage memory in case populate_section_memmap failed

[David Hildenbrand]

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 <akpm@linux-foundation.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>   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

Re: [PATCH 2/5] mm/sparse: free section usage memory in case populate_section_memmap failed

[Dong Aisheng]

On Tue, May 25, 2021 at 3:52 PM David Hildenbrand <david@redhat.com> 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 <akpm@linux-foundation.org>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >   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
>

[PATCH 3/5] mm/sparse: move mem_sections allocation out of memory_present()

[Dong Aisheng]

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 <akpm@linux-foundation.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 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

Re: [PATCH 3/5] mm/sparse: move mem_sections allocation out of memory_present()

[Mike Rapoport]

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 <akpm@linux-foundation.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>  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.

Re: [PATCH 3/5] mm/sparse: move mem_sections allocation out of memory_present()

[Dong Aisheng]

On Tue, May 18, 2021 at 6:12 PM Mike Rapoport <rppt@kernel.org> 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 <akpm@linux-foundation.org>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >  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.

[PATCH 4/5] mm: rename the global section array to mem_sections

[Dong Aisheng]

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 <akpm@linux-foundation.org>
Cc: Dave Young <dyoung@redhat.com>
Cc: Baoquan He <bhe@redhat.com>
Cc: Vivek Goyal <vgoyal@redhat.com>
Cc: kexec@lists.infradead.org
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 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

Re: [PATCH 4/5] mm: rename the global section array to mem_sections

[David Hildenbrand]

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 <akpm@linux-foundation.org>
> Cc: Dave Young <dyoung@redhat.com>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Vivek Goyal <vgoyal@redhat.com>
> Cc: kexec@lists.infradead.org
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>   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

Re: [PATCH 4/5] mm: rename the global section array to mem_sections

[Dong Aisheng]

On Tue, May 25, 2021 at 3:55 PM David Hildenbrand <david@redhat.com> 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 <akpm@linux-foundation.org>
> > Cc: Dave Young <dyoung@redhat.com>
> > Cc: Baoquan He <bhe@redhat.com>
> > Cc: Vivek Goyal <vgoyal@redhat.com>
> > Cc: kexec@lists.infradead.org
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >   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
>

[PATCH 5/5] mm/page_alloc: improve memmap_pages and dma_reserve dbg msg

[Dong Aisheng]

Make debug message more accurately.

Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 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

Re: [PATCH 5/5] mm/page_alloc: improve memmap_pages and dma_reserve dbg msg

[David Hildenbrand]

On 17.05.21 13:20, Dong Aisheng wrote:
> Make debug message more accurately.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> ---
>   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

Re: [PATCH 5/5] mm/page_alloc: improve memmap_pages and dma_reserve dbg msg

[Dong Aisheng]

On Tue, May 25, 2021 at 4:01 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 17.05.21 13:20, Dong Aisheng wrote:
> > Make debug message more accurately.
> >
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> > ---
> >   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
>
>