On Tue, May 18, 2021 at 7:43 PM Mike Rapoport [off-list ref] wrote:

On Tue, May 18, 2021 at 06:25:28PM +0800, Dong Aisheng wrote:

quoted

On Tue, May 18, 2021 at 6:09 PM Mike Rapoport [off-list ref] wrote:

quoted

On Mon, May 17, 2021 at 07:20:41PM +0800, Dong Aisheng wrote:

quoted

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

quoted

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

quoted

quoted

                      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.