[PATCH v2 0/4] Clean up comments and codes in sparse_add_one_section()

[PATCH v2 0/4] Clean up comments and codes in sparse_add_one_section()

[Baoquan He]

This is v2 post. V1 is here:
http://lkml.kernel.org/r/20190320073540.12866-1-bhe@redhat.com

This patchset includes 4 patches. The first three patches are around
sparse_add_one_section(). The last one is a simple clean up patch when
review codes in hotplug path, carry it in this patchset.

Baoquan He (4):
  mm/sparse: Clean up the obsolete code comment
  mm/sparse: Optimize sparse_add_one_section()
  mm/sparse: Rename function related to section memmap allocation/free
  drivers/base/memory.c: Rename the misleading parameter

 drivers/base/memory.c |  6 ++---
 mm/sparse.c           | 58 ++++++++++++++++++++++---------------------
 2 files changed, 33 insertions(+), 31 deletions(-)

-- 
2.17.2

[PATCH v2 1/4] mm/sparse: Clean up the obsolete code comment

[Baoquan He]

The code comment above sparse_add_one_section() is obsolete and
incorrect, clean it up and write new one.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
v1-v2:
  Add comments to explain what the returned value means for
  each error code.

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

diff --git a/mm/sparse.c b/mm/sparse.c
index 69904aa6165b..b2111f996aa6 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -685,9 +685,18 @@ static void free_map_bootmem(struct page *memmap)
 #endif /* CONFIG_SPARSEMEM_VMEMMAP */
 
 /*
- * returns the number of sections whose mem_maps were properly
- * set.  If this is <=0, then that means that the passed-in
- * map was not consumed and must be freed.
+ * sparse_add_one_section - add a memory section
+ * @nid: The node to add section on
+ * @start_pfn: start pfn of the memory range
+ * @altmap: device page map
+ *
+ * This is only intended for hotplug.
+ *
+ * Returns:
+ *   0 on success.
+ *   Other error code on failure:
+ *     - -EEXIST - section has been present.
+ *     - -ENOMEM - out of memory.
  */
 int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
 				     struct vmem_altmap *altmap)
-- 
2.17.2

[PATCH v2 2/4] mm/sparse: Optimize sparse_add_one_section()

[Baoquan He]

Reorder the allocation of usemap and memmap since usemap allocation
is much simpler and easier. Otherwise hard work is done to make
memmap ready, then have to rollback just because of usemap allocation
failure.

And also check if section is present earlier. Then don't bother to
allocate usemap and memmap if yes.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
v1->v2:
  Do section existence checking earlier to further optimize code.

 mm/sparse.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index b2111f996aa6..f4f34d69131e 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -714,20 +714,18 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
 	ret = sparse_index_init(section_nr, nid);
 	if (ret < 0 && ret != -EEXIST)
 		return ret;
-	ret = 0;
-	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
-	if (!memmap)
-		return -ENOMEM;
-	usemap = __kmalloc_section_usemap();
-	if (!usemap) {
-		__kfree_section_memmap(memmap, altmap);
-		return -ENOMEM;
-	}
 
 	ms = __pfn_to_section(start_pfn);
-	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
-		ret = -EEXIST;
-		goto out;
+	if (ms->section_mem_map & SECTION_MARKED_PRESENT)
+		return -EEXIST;
+
+	usemap = __kmalloc_section_usemap();
+	if (!usemap)
+		return -ENOMEM;
+	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
+	if (!memmap) {
+		kfree(usemap);
+		return  -ENOMEM;
 	}
 
 	/*
@@ -739,12 +737,7 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
 	section_mark_present(ms);
 	sparse_init_one_section(ms, section_nr, memmap, usemap);
 
-out:
-	if (ret < 0) {
-		kfree(usemap);
-		__kfree_section_memmap(memmap, altmap);
-	}
-	return ret;
+	return 0;
 }
 
 #ifdef CONFIG_MEMORY_HOTREMOVE
-- 
2.17.2

[PATCH v2 3/4] mm/sparse: Rename function related to section memmap allocation/free

[Baoquan He]

These functions are used to allocate/free section memmap, have nothing
to do with kmalloc/free during the handling. Rename them to remove
the confusion.

Signed-off-by: Baoquan He <bhe@redhat.com>
Acked-by: Mike Rapoport <rppt@linux.ibm.com>
---
 mm/sparse.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index f4f34d69131e..68a89d133fa7 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -590,13 +590,13 @@ void offline_mem_sections(unsigned long start_pfn, unsigned long end_pfn)
 #endif
 
 #ifdef CONFIG_SPARSEMEM_VMEMMAP
-static inline struct page *kmalloc_section_memmap(unsigned long pnum, int nid,
+static inline struct page *alloc_section_memmap(unsigned long pnum, int nid,
 		struct vmem_altmap *altmap)
 {
 	/* This will make the necessary allocations eventually. */
 	return sparse_mem_map_populate(pnum, nid, altmap);
 }
-static void __kfree_section_memmap(struct page *memmap,
+static void __free_section_memmap(struct page *memmap,
 		struct vmem_altmap *altmap)
 {
 	unsigned long start = (unsigned long)memmap;
@@ -614,7 +614,7 @@ static void free_map_bootmem(struct page *memmap)
 }
 #endif /* CONFIG_MEMORY_HOTREMOVE */
 #else
-static struct page *__kmalloc_section_memmap(void)
+static struct page *__alloc_section_memmap(void)
 {
 	struct page *page, *ret;
 	unsigned long memmap_size = sizeof(struct page) * PAGES_PER_SECTION;
@@ -635,13 +635,13 @@ static struct page *__kmalloc_section_memmap(void)
 	return ret;
 }
 
-static inline struct page *kmalloc_section_memmap(unsigned long pnum, int nid,
+static inline struct page *alloc_section_memmap(unsigned long pnum, int nid,
 		struct vmem_altmap *altmap)
 {
-	return __kmalloc_section_memmap();
+	return __alloc_section_memmap();
 }
 
-static void __kfree_section_memmap(struct page *memmap,
+static void __free_section_memmap(struct page *memmap,
 		struct vmem_altmap *altmap)
 {
 	if (is_vmalloc_addr(memmap))
@@ -722,7 +722,7 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
 	usemap = __kmalloc_section_usemap();
 	if (!usemap)
 		return -ENOMEM;
-	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
+	memmap = alloc_section_memmap(section_nr, nid, altmap);
 	if (!memmap) {
 		kfree(usemap);
 		return  -ENOMEM;
@@ -786,7 +786,7 @@ static void free_section_usemap(struct page *memmap, unsigned long *usemap,
 	if (PageSlab(usemap_page) || PageCompound(usemap_page)) {
 		kfree(usemap);
 		if (memmap)
-			__kfree_section_memmap(memmap, altmap);
+			__free_section_memmap(memmap, altmap);
 		return;
 	}
 
-- 
2.17.2

[PATCH v2 4/4] drivers/base/memory.c: Rename the misleading parameter

[Baoquan He]

The input parameter 'phys_index' of memory_block_action() is actually
the section number, but not the phys_index of memory_block. Fix it.

Signed-off-by: Baoquan He <bhe@redhat.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>
---
 drivers/base/memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index cb8347500ce2..184f4f8d1b62 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -231,13 +231,13 @@ static bool pages_correctly_probed(unsigned long start_pfn)
  * OK to have direct references to sparsemem variables in here.
  */
 static int
-memory_block_action(unsigned long phys_index, unsigned long action, int online_type)
+memory_block_action(unsigned long sec, unsigned long action, int online_type)
 {
 	unsigned long start_pfn;
 	unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
 	int ret;
 
-	start_pfn = section_nr_to_pfn(phys_index);
+	start_pfn = section_nr_to_pfn(sec);
 
 	switch (action) {
 	case MEM_ONLINE:
@@ -251,7 +251,7 @@ memory_block_action(unsigned long phys_index, unsigned long action, int online_t
 		break;
 	default:
 		WARN(1, KERN_WARNING "%s(%ld, %ld) unknown action: "
-		     "%ld\n", __func__, phys_index, action, action);
+		     "%ld\n", __func__, sec, action, action);
 		ret = -EINVAL;
 	}
 
-- 
2.17.2

Re: [PATCH v2 4/4] drivers/base/memory.c: Rename the misleading parameter

[Rafael J. Wysocki]

On Tue, Mar 26, 2019 at 10:02 AM Baoquan He <bhe@redhat.com> wrote:
>
> The input parameter 'phys_index' of memory_block_action() is actually
> the section number, but not the phys_index of memory_block. Fix it.
>
> Signed-off-by: Baoquan He <bhe@redhat.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/base/memory.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index cb8347500ce2..184f4f8d1b62 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -231,13 +231,13 @@ static bool pages_correctly_probed(unsigned long start_pfn)
>   * OK to have direct references to sparsemem variables in here.
>   */
>  static int
> -memory_block_action(unsigned long phys_index, unsigned long action, int online_type)
> +memory_block_action(unsigned long sec, unsigned long action, int online_type)
>  {
>         unsigned long start_pfn;
>         unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
>         int ret;
>
> -       start_pfn = section_nr_to_pfn(phys_index);
> +       start_pfn = section_nr_to_pfn(sec);
>
>         switch (action) {
>         case MEM_ONLINE:
> @@ -251,7 +251,7 @@ memory_block_action(unsigned long phys_index, unsigned long action, int online_t
>                 break;
>         default:
>                 WARN(1, KERN_WARNING "%s(%ld, %ld) unknown action: "
> -                    "%ld\n", __func__, phys_index, action, action);
> +                    "%ld\n", __func__, sec, action, action);
>                 ret = -EINVAL;
>         }
>
> --
> 2.17.2
>

Re: [PATCH v2 1/4] mm/sparse: Clean up the obsolete code comment

[Mike Rapoport]

On Tue, Mar 26, 2019 at 05:02:24PM +0800, Baoquan He wrote:
> The code comment above sparse_add_one_section() is obsolete and
> incorrect, clean it up and write new one.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>

Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>

> ---
> v1-v2:
>   Add comments to explain what the returned value means for
>   each error code.
> 
>  mm/sparse.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 69904aa6165b..b2111f996aa6 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -685,9 +685,18 @@ static void free_map_bootmem(struct page *memmap)
>  #endif /* CONFIG_SPARSEMEM_VMEMMAP */
>  
>  /*
> - * returns the number of sections whose mem_maps were properly
> - * set.  If this is <=0, then that means that the passed-in
> - * map was not consumed and must be freed.
> + * sparse_add_one_section - add a memory section
> + * @nid: The node to add section on
> + * @start_pfn: start pfn of the memory range
> + * @altmap: device page map
> + *
> + * This is only intended for hotplug.
> + *
> + * Returns:
> + *   0 on success.
> + *   Other error code on failure:
> + *     - -EEXIST - section has been present.
> + *     - -ENOMEM - out of memory.
>   */
>  int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  				     struct vmem_altmap *altmap)
> -- 
> 2.17.2
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v2 1/4] mm/sparse: Clean up the obsolete code comment

[Michal Hocko]

On Tue 26-03-19 17:02:24, Baoquan He wrote:
> The code comment above sparse_add_one_section() is obsolete and
> incorrect, clean it up and write new one.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>

Please note that you need /** to start a kernel doc. Other than that.

Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> v1-v2:
>   Add comments to explain what the returned value means for
>   each error code.
> 
>  mm/sparse.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 69904aa6165b..b2111f996aa6 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -685,9 +685,18 @@ static void free_map_bootmem(struct page *memmap)
>  #endif /* CONFIG_SPARSEMEM_VMEMMAP */
>  
>  /*
> - * returns the number of sections whose mem_maps were properly
> - * set.  If this is <=0, then that means that the passed-in
> - * map was not consumed and must be freed.
> + * sparse_add_one_section - add a memory section
> + * @nid: The node to add section on
> + * @start_pfn: start pfn of the memory range
> + * @altmap: device page map
> + *
> + * This is only intended for hotplug.
> + *
> + * Returns:
> + *   0 on success.
> + *   Other error code on failure:
> + *     - -EEXIST - section has been present.
> + *     - -ENOMEM - out of memory.
>   */
>  int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  				     struct vmem_altmap *altmap)
> -- 
> 2.17.2
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 2/4] mm/sparse: Optimize sparse_add_one_section()

[Mike Rapoport]

On Tue, Mar 26, 2019 at 05:02:25PM +0800, Baoquan He wrote:
> Reorder the allocation of usemap and memmap since usemap allocation
> is much simpler and easier. Otherwise hard work is done to make
> memmap ready, then have to rollback just because of usemap allocation
> failure.
> 
> And also check if section is present earlier. Then don't bother to
> allocate usemap and memmap if yes.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>

Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>

> ---
> v1->v2:
>   Do section existence checking earlier to further optimize code.
> 
>  mm/sparse.c | 29 +++++++++++------------------
>  1 file changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index b2111f996aa6..f4f34d69131e 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -714,20 +714,18 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  	ret = sparse_index_init(section_nr, nid);
>  	if (ret < 0 && ret != -EEXIST)
>  		return ret;
> -	ret = 0;
> -	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
> -	if (!memmap)
> -		return -ENOMEM;
> -	usemap = __kmalloc_section_usemap();
> -	if (!usemap) {
> -		__kfree_section_memmap(memmap, altmap);
> -		return -ENOMEM;
> -	}
>  
>  	ms = __pfn_to_section(start_pfn);
> -	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
> -		ret = -EEXIST;
> -		goto out;
> +	if (ms->section_mem_map & SECTION_MARKED_PRESENT)
> +		return -EEXIST;
> +
> +	usemap = __kmalloc_section_usemap();
> +	if (!usemap)
> +		return -ENOMEM;
> +	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
> +	if (!memmap) {
> +		kfree(usemap);
> +		return  -ENOMEM;
>  	}
>  
>  	/*
> @@ -739,12 +737,7 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  	section_mark_present(ms);
>  	sparse_init_one_section(ms, section_nr, memmap, usemap);
>  
> -out:
> -	if (ret < 0) {
> -		kfree(usemap);
> -		__kfree_section_memmap(memmap, altmap);
> -	}
> -	return ret;
> +	return 0;
>  }
>  
>  #ifdef CONFIG_MEMORY_HOTREMOVE
> -- 
> 2.17.2
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v2 2/4] mm/sparse: Optimize sparse_add_one_section()

[Michal Hocko]

On Tue 26-03-19 17:02:25, Baoquan He wrote:
> Reorder the allocation of usemap and memmap since usemap allocation
> is much simpler and easier. Otherwise hard work is done to make
> memmap ready, then have to rollback just because of usemap allocation
> failure.

Is this really worth it? I can see that !VMEMMAP is doing memmap size
allocation which would be 2MB aka costly allocation but we do not do
__GFP_RETRY_MAYFAIL so the allocator backs off early.

> And also check if section is present earlier. Then don't bother to
> allocate usemap and memmap if yes.

Moving the check up makes some sense.

> Signed-off-by: Baoquan He <bhe@redhat.com>

The patch is not incorrect but I am wondering whether it is really worth
it for the current code base. Is it fixing anything real or it is a mere
code shuffling to please an eye?

> ---
> v1->v2:
>   Do section existence checking earlier to further optimize code.
> 
>  mm/sparse.c | 29 +++++++++++------------------
>  1 file changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index b2111f996aa6..f4f34d69131e 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -714,20 +714,18 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  	ret = sparse_index_init(section_nr, nid);
>  	if (ret < 0 && ret != -EEXIST)
>  		return ret;
> -	ret = 0;
> -	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
> -	if (!memmap)
> -		return -ENOMEM;
> -	usemap = __kmalloc_section_usemap();
> -	if (!usemap) {
> -		__kfree_section_memmap(memmap, altmap);
> -		return -ENOMEM;
> -	}
>  
>  	ms = __pfn_to_section(start_pfn);
> -	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
> -		ret = -EEXIST;
> -		goto out;
> +	if (ms->section_mem_map & SECTION_MARKED_PRESENT)
> +		return -EEXIST;
> +
> +	usemap = __kmalloc_section_usemap();
> +	if (!usemap)
> +		return -ENOMEM;
> +	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
> +	if (!memmap) {
> +		kfree(usemap);
> +		return  -ENOMEM;
>  	}
>  
>  	/*
> @@ -739,12 +737,7 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  	section_mark_present(ms);
>  	sparse_init_one_section(ms, section_nr, memmap, usemap);
>  
> -out:
> -	if (ret < 0) {
> -		kfree(usemap);
> -		__kfree_section_memmap(memmap, altmap);
> -	}
> -	return ret;
> +	return 0;
>  }
>  
>  #ifdef CONFIG_MEMORY_HOTREMOVE
> -- 
> 2.17.2
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 1/4] mm/sparse: Clean up the obsolete code comment

[Baoquan He]

On 03/26/19 at 10:23am, Michal Hocko wrote:
> On Tue 26-03-19 17:02:24, Baoquan He wrote:
> > The code comment above sparse_add_one_section() is obsolete and
> > incorrect, clean it up and write new one.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> 
> Please note that you need /** to start a kernel doc. Other than that.

I didn't find a template in coding-style.rst, and saw someone is using
/*, others use /**. I will use '/**' instead. Thanks for telling.

> 
> Acked-by: Michal Hocko <mhocko@suse.com>
> > ---
> > v1-v2:
> >   Add comments to explain what the returned value means for
> >   each error code.
> > 
> >  mm/sparse.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/mm/sparse.c b/mm/sparse.c
> > index 69904aa6165b..b2111f996aa6 100644
> > --- a/mm/sparse.c
> > +++ b/mm/sparse.c
> > @@ -685,9 +685,18 @@ static void free_map_bootmem(struct page *memmap)
> >  #endif /* CONFIG_SPARSEMEM_VMEMMAP */
> >  
> >  /*
> > - * returns the number of sections whose mem_maps were properly
> > - * set.  If this is <=0, then that means that the passed-in
> > - * map was not consumed and must be freed.
> > + * sparse_add_one_section - add a memory section
> > + * @nid: The node to add section on
> > + * @start_pfn: start pfn of the memory range
> > + * @altmap: device page map
> > + *
> > + * This is only intended for hotplug.
> > + *
> > + * Returns:
> > + *   0 on success.
> > + *   Other error code on failure:
> > + *     - -EEXIST - section has been present.
> > + *     - -ENOMEM - out of memory.
> >   */
> >  int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
> >  				     struct vmem_altmap *altmap)
> > -- 
> > 2.17.2
> > 
> 
> -- 
> Michal Hocko
> SUSE Labs

Re: [PATCH v2 4/4] drivers/base/memory.c: Rename the misleading parameter

[Michal Hocko]

On Tue 26-03-19 17:02:27, Baoquan He wrote:
> The input parameter 'phys_index' of memory_block_action() is actually
> the section number, but not the phys_index of memory_block. Fix it.

phys_index is a relict from the past and it indeed denotes the section
number which is exported as phys_index via sysfs. start_section_nr would
be a better name IMHO but nothing to really bike shed about.

> Signed-off-by: Baoquan He <bhe@redhat.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: "Rafael J. Wysocki" <rafael@kernel.org>

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

> ---
>  drivers/base/memory.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/base/memory.c b/drivers/base/memory.c
> index cb8347500ce2..184f4f8d1b62 100644
> --- a/drivers/base/memory.c
> +++ b/drivers/base/memory.c
> @@ -231,13 +231,13 @@ static bool pages_correctly_probed(unsigned long start_pfn)
>   * OK to have direct references to sparsemem variables in here.
>   */
>  static int
> -memory_block_action(unsigned long phys_index, unsigned long action, int online_type)
> +memory_block_action(unsigned long sec, unsigned long action, int online_type)
>  {
>  	unsigned long start_pfn;
>  	unsigned long nr_pages = PAGES_PER_SECTION * sections_per_block;
>  	int ret;
>  
> -	start_pfn = section_nr_to_pfn(phys_index);
> +	start_pfn = section_nr_to_pfn(sec);
>  
>  	switch (action) {
>  	case MEM_ONLINE:
> @@ -251,7 +251,7 @@ memory_block_action(unsigned long phys_index, unsigned long action, int online_t
>  		break;
>  	default:
>  		WARN(1, KERN_WARNING "%s(%ld, %ld) unknown action: "
> -		     "%ld\n", __func__, phys_index, action, action);
> +		     "%ld\n", __func__, sec, action, action);
>  		ret = -EINVAL;
>  	}
>  
> -- 
> 2.17.2
> 

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 1/4] mm/sparse: Clean up the obsolete code comment

[Chao Fan]

On Tue, Mar 26, 2019 at 05:30:57PM +0800, Baoquan He wrote:
>On 03/26/19 at 10:23am, Michal Hocko wrote:
>> On Tue 26-03-19 17:02:24, Baoquan He wrote:
>> > The code comment above sparse_add_one_section() is obsolete and
>> > incorrect, clean it up and write new one.
>> > 
>> > Signed-off-by: Baoquan He <bhe@redhat.com>
>> 
>> Please note that you need /** to start a kernel doc. Other than that.
>
>I didn't find a template in coding-style.rst, and saw someone is using
>/*, others use /**. I will use '/**' instead. Thanks for telling.

How to format kernel-doc comments
---------------------------------

The opening comment mark ``/**`` is used for kernel-doc comments. The
``kernel-doc`` tool will extract comments marked this way. The rest of
the comment is formatted like a normal multi-line comment with a column
of asterisks on the left side, closing with ``*/`` on a line by itself.

See Documentation/doc-guide/kernel-doc.rst for more details.
Hope that can help you.

Thanks,
Chao Fan

>
>> 
>> Acked-by: Michal Hocko <mhocko@suse.com>
>> > ---
>> > v1-v2:
>> >   Add comments to explain what the returned value means for
>> >   each error code.
>> > 
>> >  mm/sparse.c | 15 ++++++++++++---
>> >  1 file changed, 12 insertions(+), 3 deletions(-)
>> > 
>> > diff --git a/mm/sparse.c b/mm/sparse.c
>> > index 69904aa6165b..b2111f996aa6 100644
>> > --- a/mm/sparse.c
>> > +++ b/mm/sparse.c
>> > @@ -685,9 +685,18 @@ static void free_map_bootmem(struct page *memmap)
>> >  #endif /* CONFIG_SPARSEMEM_VMEMMAP */
>> >  
>> >  /*
>> > - * returns the number of sections whose mem_maps were properly
>> > - * set.  If this is <=0, then that means that the passed-in
>> > - * map was not consumed and must be freed.
>> > + * sparse_add_one_section - add a memory section
>> > + * @nid: The node to add section on
>> > + * @start_pfn: start pfn of the memory range
>> > + * @altmap: device page map
>> > + *
>> > + * This is only intended for hotplug.
>> > + *
>> > + * Returns:
>> > + *   0 on success.
>> > + *   Other error code on failure:
>> > + *     - -EEXIST - section has been present.
>> > + *     - -ENOMEM - out of memory.
>> >   */
>> >  int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>> >  				     struct vmem_altmap *altmap)
>> > -- 
>> > 2.17.2
>> > 
>> 
>> -- 
>> Michal Hocko
>> SUSE Labs
>
>

Re: [PATCH v2 1/4] mm/sparse: Clean up the obsolete code comment

[Baoquan He]

On 03/26/19 at 05:36pm, Chao Fan wrote:
> On Tue, Mar 26, 2019 at 05:30:57PM +0800, Baoquan He wrote:
> >On 03/26/19 at 10:23am, Michal Hocko wrote:
> >> On Tue 26-03-19 17:02:24, Baoquan He wrote:
> >> > The code comment above sparse_add_one_section() is obsolete and
> >> > incorrect, clean it up and write new one.
> >> > 
> >> > Signed-off-by: Baoquan He <bhe@redhat.com>
> >> 
> >> Please note that you need /** to start a kernel doc. Other than that.
> >
> >I didn't find a template in coding-style.rst, and saw someone is using
> >/*, others use /**. I will use '/**' instead. Thanks for telling.
> 
> How to format kernel-doc comments
> ---------------------------------
> 
> The opening comment mark ``/**`` is used for kernel-doc comments. The
> ``kernel-doc`` tool will extract comments marked this way. The rest of
> the comment is formatted like a normal multi-line comment with a column
> of asterisks on the left side, closing with ``*/`` on a line by itself.
> 
> See Documentation/doc-guide/kernel-doc.rst for more details.
> Hope that can help you.

Great, there's a specific kernel-doc file. Thanks, I will update and
repost this one with '/**'.

Re: [PATCH v2 1/4] mm/sparse: Clean up the obsolete code comment

[Chao Fan]

On Tue, Mar 26, 2019 at 05:43:48PM +0800, Baoquan He wrote:
>On 03/26/19 at 05:36pm, Chao Fan wrote:
>> On Tue, Mar 26, 2019 at 05:30:57PM +0800, Baoquan He wrote:
>> >On 03/26/19 at 10:23am, Michal Hocko wrote:
>> >> On Tue 26-03-19 17:02:24, Baoquan He wrote:
>> >> > The code comment above sparse_add_one_section() is obsolete and
>> >> > incorrect, clean it up and write new one.
>> >> > 
>> >> > Signed-off-by: Baoquan He <bhe@redhat.com>
>> >> 
>> >> Please note that you need /** to start a kernel doc. Other than that.
>> >
>> >I didn't find a template in coding-style.rst, and saw someone is using
>> >/*, others use /**. I will use '/**' instead. Thanks for telling.
>> 
>> How to format kernel-doc comments
>> ---------------------------------
>> 
>> The opening comment mark ``/**`` is used for kernel-doc comments. The
>> ``kernel-doc`` tool will extract comments marked this way. The rest of
>> the comment is formatted like a normal multi-line comment with a column
>> of asterisks on the left side, closing with ``*/`` on a line by itself.
>> 
>> See Documentation/doc-guide/kernel-doc.rst for more details.
>> Hope that can help you.
>
>Great, there's a specific kernel-doc file. Thanks, I will update and
>repost this one with '/**'.

In that file, there is also some sample for a function comment:

Function documentation
----------------------

The general format of a function and function-like macro kernel-doc comment is::

  /**
   * function_name() - Brief description of function.
   * @arg1: Describe the first argument.
   * @arg2: Describe the second argument.
   *        One can provide multiple line descriptions
   *        for arguments.
   *
   * A longer description, with more discussion of the function function_name()
   * that might be useful to those using or modifying it. Begins with an
   * empty comment line, and may include additional embedded empty
   * comment lines.
   *
   * The longer description may have multiple paragraphs.
   *
   * Context: Describes whether the function can sleep, what locks it takes,
   *          releases, or expects to be held. It can extend over multiple
   *          lines.
   * Return: Describe the return value of function_name.
   *
   * The return value description can also have multiple paragraphs, and should
   * be placed at the end of the comment block.
   */


Anyway, I think you can get more information in that document.

Thanks,
Chao Fan

>
>

Re: [PATCH v2 2/4] mm/sparse: Optimize sparse_add_one_section()

[Baoquan He]

On 03/26/19 at 10:29am, Michal Hocko wrote:
> On Tue 26-03-19 17:02:25, Baoquan He wrote:
> > Reorder the allocation of usemap and memmap since usemap allocation
> > is much simpler and easier. Otherwise hard work is done to make
> > memmap ready, then have to rollback just because of usemap allocation
> > failure.
> 
> Is this really worth it? I can see that !VMEMMAP is doing memmap size
> allocation which would be 2MB aka costly allocation but we do not do
> __GFP_RETRY_MAYFAIL so the allocator backs off early.

In !VMEMMAP case, it truly does simple allocation directly. surely
usemap which size is 32 is smaller. So it doesn't matter that much who's
ahead or who's behind. However, this benefit a little in VMEMMAP case.

And this make code a little cleaner, e.g the error handling at the end
is taken away.

> 
> > And also check if section is present earlier. Then don't bother to
> > allocate usemap and memmap if yes.
> 
> Moving the check up makes some sense.
> 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> 
> The patch is not incorrect but I am wondering whether it is really worth
> it for the current code base. Is it fixing anything real or it is a mere
> code shuffling to please an eye?

It's not a fixing, just a tiny code refactorying inside
sparse_add_one_section(), seems it doesn't worsen thing if I got the
!VMEMMAP case correctly, not quite sure. I am fine to drop it if it's
not worth. I could miss something in different cases.

Thanks
Baoquan

> 
> > ---
> > v1->v2:
> >   Do section existence checking earlier to further optimize code.
> > 
> >  mm/sparse.c | 29 +++++++++++------------------
> >  1 file changed, 11 insertions(+), 18 deletions(-)
> > 
> > diff --git a/mm/sparse.c b/mm/sparse.c
> > index b2111f996aa6..f4f34d69131e 100644
> > --- a/mm/sparse.c
> > +++ b/mm/sparse.c
> > @@ -714,20 +714,18 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
> >  	ret = sparse_index_init(section_nr, nid);
> >  	if (ret < 0 && ret != -EEXIST)
> >  		return ret;
> > -	ret = 0;
> > -	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
> > -	if (!memmap)
> > -		return -ENOMEM;
> > -	usemap = __kmalloc_section_usemap();
> > -	if (!usemap) {
> > -		__kfree_section_memmap(memmap, altmap);
> > -		return -ENOMEM;
> > -	}
> >  
> >  	ms = __pfn_to_section(start_pfn);
> > -	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
> > -		ret = -EEXIST;
> > -		goto out;
> > +	if (ms->section_mem_map & SECTION_MARKED_PRESENT)
> > +		return -EEXIST;
> > +
> > +	usemap = __kmalloc_section_usemap();
> > +	if (!usemap)
> > +		return -ENOMEM;
> > +	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
> > +	if (!memmap) {
> > +		kfree(usemap);
> > +		return  -ENOMEM;
> >  	}
> >  
> >  	/*
> > @@ -739,12 +737,7 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
> >  	section_mark_present(ms);
> >  	sparse_init_one_section(ms, section_nr, memmap, usemap);
> >  
> > -out:
> > -	if (ret < 0) {
> > -		kfree(usemap);
> > -		__kfree_section_memmap(memmap, altmap);
> > -	}
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  #ifdef CONFIG_MEMORY_HOTREMOVE
> > -- 
> > 2.17.2
> > 
> 
> -- 
> Michal Hocko
> SUSE Labs

Re: [PATCH v2 2/4] mm/sparse: Optimize sparse_add_one_section()

[Michal Hocko]

On Tue 26-03-19 18:08:17, Baoquan He wrote:
> On 03/26/19 at 10:29am, Michal Hocko wrote:
> > On Tue 26-03-19 17:02:25, Baoquan He wrote:
> > > Reorder the allocation of usemap and memmap since usemap allocation
> > > is much simpler and easier. Otherwise hard work is done to make
> > > memmap ready, then have to rollback just because of usemap allocation
> > > failure.
> > 
> > Is this really worth it? I can see that !VMEMMAP is doing memmap size
> > allocation which would be 2MB aka costly allocation but we do not do
> > __GFP_RETRY_MAYFAIL so the allocator backs off early.
> 
> In !VMEMMAP case, it truly does simple allocation directly. surely
> usemap which size is 32 is smaller. So it doesn't matter that much who's
> ahead or who's behind. However, this benefit a little in VMEMMAP case.

How does it help there? The failure should be even much less probable
there because we simply fall back to a small 4kB pages and those
essentially never fail.

> And this make code a little cleaner, e.g the error handling at the end
> is taken away.
> 
> > 
> > > And also check if section is present earlier. Then don't bother to
> > > allocate usemap and memmap if yes.
> > 
> > Moving the check up makes some sense.
> > 
> > > Signed-off-by: Baoquan He <bhe@redhat.com>
> > 
> > The patch is not incorrect but I am wondering whether it is really worth
> > it for the current code base. Is it fixing anything real or it is a mere
> > code shuffling to please an eye?
> 
> It's not a fixing, just a tiny code refactorying inside
> sparse_add_one_section(), seems it doesn't worsen thing if I got the
> !VMEMMAP case correctly, not quite sure. I am fine to drop it if it's
> not worth. I could miss something in different cases.

Well, I usually prefer to not do micro-optimizations in a code that
really begs for a much larger surgery. There are other people working on
the code and patches like these might get into the way and cuase
conflicts without a very good justification.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 4/4] drivers/base/memory.c: Rename the misleading parameter

[Matthew Wilcox]

On Tue, Mar 26, 2019 at 05:02:27PM +0800, Baoquan He wrote:
> The input parameter 'phys_index' of memory_block_action() is actually
> the section number, but not the phys_index of memory_block. Fix it.

>  static int
> -memory_block_action(unsigned long phys_index, unsigned long action, int online_type)
> +memory_block_action(unsigned long sec, unsigned long action, int online_type)

'sec' is a bad abbreviation for 'section'.  We don't use it anyhere else
in the vm.

Looking through include/, I see it used as an abbreviation for second,
security, ELF section, and section of a book.  Nowhere as a memory
block section.  Please use an extra four letters for this parameter.

Re: [PATCH v2 4/4] drivers/base/memory.c: Rename the misleading parameter

[Baoquan He]

On 03/26/19 at 04:43am, Matthew Wilcox wrote:
> On Tue, Mar 26, 2019 at 05:02:27PM +0800, Baoquan He wrote:
> > The input parameter 'phys_index' of memory_block_action() is actually
> > the section number, but not the phys_index of memory_block. Fix it.
> 
> >  static int
> > -memory_block_action(unsigned long phys_index, unsigned long action, int online_type)
> > +memory_block_action(unsigned long sec, unsigned long action, int online_type)
> 
> 'sec' is a bad abbreviation for 'section'.  We don't use it anyhere else
> in the vm.

Hmm, here 'sec' is in a particular context, we may not confuse it with
other abbreviation. Since Michal also complained about it, seems an
update is needed. I will change it to start_section_nr as Michal
suggested. Thanks.

> 
> Looking through include/, I see it used as an abbreviation for second,
> security, ELF section, and section of a book.  Nowhere as a memory
> block section.  Please use an extra four letters for this parameter.

Re: [PATCH v2 2/4] mm/sparse: Optimize sparse_add_one_section()

[Baoquan He]

On 03/26/19 at 11:17am, Michal Hocko wrote:
> On Tue 26-03-19 18:08:17, Baoquan He wrote:
> > On 03/26/19 at 10:29am, Michal Hocko wrote:
> > > On Tue 26-03-19 17:02:25, Baoquan He wrote:
> > > > Reorder the allocation of usemap and memmap since usemap allocation
> > > > is much simpler and easier. Otherwise hard work is done to make
> > > > memmap ready, then have to rollback just because of usemap allocation
> > > > failure.
> > > 
> > > Is this really worth it? I can see that !VMEMMAP is doing memmap size
> > > allocation which would be 2MB aka costly allocation but we do not do
> > > __GFP_RETRY_MAYFAIL so the allocator backs off early.
> > 
> > In !VMEMMAP case, it truly does simple allocation directly. surely
> > usemap which size is 32 is smaller. So it doesn't matter that much who's
> > ahead or who's behind. However, this benefit a little in VMEMMAP case.
> 
> How does it help there? The failure should be even much less probable
> there because we simply fall back to a small 4kB pages and those
> essentially never fail.

OK, I am fine to drop it. Or only put the section existence checking
earlier to avoid unnecessary usemap/memmap allocation?


From 7594b86ebf5d6fcc8146eca8fc5625f1961a15b1 Mon Sep 17 00:00:00 2001
From: Baoquan He <bhe@redhat.com>
Date: Tue, 26 Mar 2019 18:48:39 +0800
Subject: [PATCH] mm/sparse: Check section's existence earlier in
 sparse_add_one_section()

No need to allocate usemap and memmap if section has been present.
And can clean up the handling on failure.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/sparse.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/mm/sparse.c b/mm/sparse.c
index 363f9d31b511..f564b531e0f7 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -714,7 +714,13 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
 	ret = sparse_index_init(section_nr, nid);
 	if (ret < 0 && ret != -EEXIST)
 		return ret;
-	ret = 0;
+
+	ms = __pfn_to_section(start_pfn);
+	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
+		ret = -EEXIST;
+		goto out;
+	}
+
 	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
 	if (!memmap)
 		return -ENOMEM;
@@ -724,12 +730,6 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
 		return -ENOMEM;
 	}
 
-	ms = __pfn_to_section(start_pfn);
-	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
-		ret = -EEXIST;
-		goto out;
-	}
-
 	/*
 	 * Poison uninitialized struct pages in order to catch invalid flags
 	 * combinations.
@@ -739,12 +739,7 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
 	section_mark_present(ms);
 	sparse_init_one_section(ms, section_nr, memmap, usemap);
 
-out:
-	if (ret < 0) {
-		kfree(usemap);
-		__kfree_section_memmap(memmap, altmap);
-	}
-	return ret;
+	return 0;
 }
 
 #ifdef CONFIG_MEMORY_HOTREMOVE
-- 
2.17.2

Re: [PATCH v2 2/4] mm/sparse: Optimize sparse_add_one_section()

[Mike Rapoport]

On Tue, Mar 26, 2019 at 09:45:22PM +0800, Baoquan He wrote:
> On 03/26/19 at 11:17am, Michal Hocko wrote:
> > On Tue 26-03-19 18:08:17, Baoquan He wrote:
> > > On 03/26/19 at 10:29am, Michal Hocko wrote:
> > > > On Tue 26-03-19 17:02:25, Baoquan He wrote:
> > > > > Reorder the allocation of usemap and memmap since usemap allocation
> > > > > is much simpler and easier. Otherwise hard work is done to make
> > > > > memmap ready, then have to rollback just because of usemap allocation
> > > > > failure.
> > > > 
> > > > Is this really worth it? I can see that !VMEMMAP is doing memmap size
> > > > allocation which would be 2MB aka costly allocation but we do not do
> > > > __GFP_RETRY_MAYFAIL so the allocator backs off early.
> > > 
> > > In !VMEMMAP case, it truly does simple allocation directly. surely
> > > usemap which size is 32 is smaller. So it doesn't matter that much who's
> > > ahead or who's behind. However, this benefit a little in VMEMMAP case.
> > 
> > How does it help there? The failure should be even much less probable
> > there because we simply fall back to a small 4kB pages and those
> > essentially never fail.
> 
> OK, I am fine to drop it. Or only put the section existence checking
> earlier to avoid unnecessary usemap/memmap allocation?
> 
> 
> From 7594b86ebf5d6fcc8146eca8fc5625f1961a15b1 Mon Sep 17 00:00:00 2001
> From: Baoquan He <bhe@redhat.com>
> Date: Tue, 26 Mar 2019 18:48:39 +0800
> Subject: [PATCH] mm/sparse: Check section's existence earlier in
>  sparse_add_one_section()
> 
> No need to allocate usemap and memmap if section has been present.
> And can clean up the handling on failure.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>
> ---
>  mm/sparse.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/mm/sparse.c b/mm/sparse.c
> index 363f9d31b511..f564b531e0f7 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -714,7 +714,13 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  	ret = sparse_index_init(section_nr, nid);
>  	if (ret < 0 && ret != -EEXIST)
>  		return ret;
> -	ret = 0;
> +
> +	ms = __pfn_to_section(start_pfn);
> +	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
> +		ret = -EEXIST;
> +		goto out;

		return -EEXIST; ?

> +	}
> +
>  	memmap = kmalloc_section_memmap(section_nr, nid, altmap);
>  	if (!memmap)
>  		return -ENOMEM;
> @@ -724,12 +730,6 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  		return -ENOMEM;
>  	}
>  
> -	ms = __pfn_to_section(start_pfn);
> -	if (ms->section_mem_map & SECTION_MARKED_PRESENT) {
> -		ret = -EEXIST;
> -		goto out;
> -	}
> -
>  	/*
>  	 * Poison uninitialized struct pages in order to catch invalid flags
>  	 * combinations.
> @@ -739,12 +739,7 @@ int __meminit sparse_add_one_section(int nid, unsigned long start_pfn,
>  	section_mark_present(ms);
>  	sparse_init_one_section(ms, section_nr, memmap, usemap);
>  
> -out:
> -	if (ret < 0) {
> -		kfree(usemap);
> -		__kfree_section_memmap(memmap, altmap);
> -	}
> -	return ret;
> +	return 0;
>  }
>  
>  #ifdef CONFIG_MEMORY_HOTREMOVE
> -- 
> 2.17.2
> 

-- 
Sincerely yours,
Mike.

Re: [PATCH v2 2/4] mm/sparse: Optimize sparse_add_one_section()

[Michal Hocko]

On Tue 26-03-19 21:45:22, Baoquan He wrote:
> On 03/26/19 at 11:17am, Michal Hocko wrote:
> > On Tue 26-03-19 18:08:17, Baoquan He wrote:
> > > On 03/26/19 at 10:29am, Michal Hocko wrote:
> > > > On Tue 26-03-19 17:02:25, Baoquan He wrote:
> > > > > Reorder the allocation of usemap and memmap since usemap allocation
> > > > > is much simpler and easier. Otherwise hard work is done to make
> > > > > memmap ready, then have to rollback just because of usemap allocation
> > > > > failure.
> > > > 
> > > > Is this really worth it? I can see that !VMEMMAP is doing memmap size
> > > > allocation which would be 2MB aka costly allocation but we do not do
> > > > __GFP_RETRY_MAYFAIL so the allocator backs off early.
> > > 
> > > In !VMEMMAP case, it truly does simple allocation directly. surely
> > > usemap which size is 32 is smaller. So it doesn't matter that much who's
> > > ahead or who's behind. However, this benefit a little in VMEMMAP case.
> > 
> > How does it help there? The failure should be even much less probable
> > there because we simply fall back to a small 4kB pages and those
> > essentially never fail.
> 
> OK, I am fine to drop it. Or only put the section existence checking
> earlier to avoid unnecessary usemap/memmap allocation?

DO you have any data on how often that happens? Should basically never
happening, right?
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 2/4] mm/sparse: Optimize sparse_add_one_section()

[Baoquan He]

On 03/26/19 at 03:03pm, Michal Hocko wrote:
> On Tue 26-03-19 21:45:22, Baoquan He wrote:
> > On 03/26/19 at 11:17am, Michal Hocko wrote:
> > > On Tue 26-03-19 18:08:17, Baoquan He wrote:
> > > > On 03/26/19 at 10:29am, Michal Hocko wrote:
> > > > > On Tue 26-03-19 17:02:25, Baoquan He wrote:
> > > > > > Reorder the allocation of usemap and memmap since usemap allocation
> > > > > > is much simpler and easier. Otherwise hard work is done to make
> > > > > > memmap ready, then have to rollback just because of usemap allocation
> > > > > > failure.
> > > > > 
> > > > > Is this really worth it? I can see that !VMEMMAP is doing memmap size
> > > > > allocation which would be 2MB aka costly allocation but we do not do
> > > > > __GFP_RETRY_MAYFAIL so the allocator backs off early.
> > > > 
> > > > In !VMEMMAP case, it truly does simple allocation directly. surely
> > > > usemap which size is 32 is smaller. So it doesn't matter that much who's
> > > > ahead or who's behind. However, this benefit a little in VMEMMAP case.
> > > 
> > > How does it help there? The failure should be even much less probable
> > > there because we simply fall back to a small 4kB pages and those
> > > essentially never fail.
> > 
> > OK, I am fine to drop it. Or only put the section existence checking
> > earlier to avoid unnecessary usemap/memmap allocation?
> 
> DO you have any data on how often that happens? Should basically never
> happening, right?

Oh, you think about it in this aspect. Yes, it rarely happens.
Always allocating firstly can increase efficiency. Then I will just drop
it.

Re: [PATCH v2 2/4] mm/sparse: Optimize sparse_add_one_section()

[Michal Hocko]

On Tue 26-03-19 22:18:03, Baoquan He wrote:
> On 03/26/19 at 03:03pm, Michal Hocko wrote:
> > On Tue 26-03-19 21:45:22, Baoquan He wrote:
> > > On 03/26/19 at 11:17am, Michal Hocko wrote:
> > > > On Tue 26-03-19 18:08:17, Baoquan He wrote:
> > > > > On 03/26/19 at 10:29am, Michal Hocko wrote:
> > > > > > On Tue 26-03-19 17:02:25, Baoquan He wrote:
> > > > > > > Reorder the allocation of usemap and memmap since usemap allocation
> > > > > > > is much simpler and easier. Otherwise hard work is done to make
> > > > > > > memmap ready, then have to rollback just because of usemap allocation
> > > > > > > failure.
> > > > > > 
> > > > > > Is this really worth it? I can see that !VMEMMAP is doing memmap size
> > > > > > allocation which would be 2MB aka costly allocation but we do not do
> > > > > > __GFP_RETRY_MAYFAIL so the allocator backs off early.
> > > > > 
> > > > > In !VMEMMAP case, it truly does simple allocation directly. surely
> > > > > usemap which size is 32 is smaller. So it doesn't matter that much who's
> > > > > ahead or who's behind. However, this benefit a little in VMEMMAP case.
> > > > 
> > > > How does it help there? The failure should be even much less probable
> > > > there because we simply fall back to a small 4kB pages and those
> > > > essentially never fail.
> > > 
> > > OK, I am fine to drop it. Or only put the section existence checking
> > > earlier to avoid unnecessary usemap/memmap allocation?
> > 
> > DO you have any data on how often that happens? Should basically never
> > happening, right?
> 
> Oh, you think about it in this aspect. Yes, it rarely happens.
> Always allocating firstly can increase efficiency. Then I will just drop
> it.

OK, let me try once more. Doing a check early is something that makes
sense in general. Another question is whether the check is needed at
all. So rather than fiddling with its placement I would go whether it is
actually failing at all. I suspect it doesn't because the memory hotplug
is currently enforced to be section aligned. There are people who would
like to allow subsection or section unaligned aware hotplug and then
this would be much more relevant but without any solid justification
such a patch is not really helpful because it might cause code conflicts
with other work or obscure the git blame tracking by an additional hop.

In short, if you want to optimize something then make sure you describe
what you are optimizing how it helps.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH v2 2/4] mm/sparse: Optimize sparse_add_one_section()

[Baoquan He]

Hi Michal,

On 03/26/19 at 03:31pm, Michal Hocko wrote:
> > > > OK, I am fine to drop it. Or only put the section existence checking
> > > > earlier to avoid unnecessary usemap/memmap allocation?
> > > 
> > > DO you have any data on how often that happens? Should basically never
> > > happening, right?
> > 
> > Oh, you think about it in this aspect. Yes, it rarely happens.
> > Always allocating firstly can increase efficiency. Then I will just drop
> > it.
> 
> OK, let me try once more. Doing a check early is something that makes
> sense in general. Another question is whether the check is needed at
> all. So rather than fiddling with its placement I would go whether it is
> actually failing at all. I suspect it doesn't because the memory hotplug
> is currently enforced to be section aligned. There are people who would
> like to allow subsection or section unaligned aware hotplug and then
> this would be much more relevant but without any solid justification
> such a patch is not really helpful because it might cause code conflicts
> with other work or obscure the git blame tracking by an additional hop.
> 
> In short, if you want to optimize something then make sure you describe
> what you are optimizing how it helps.

I must be dizzy last night when thinking and replying mails, I thought
about it a while, got a point you may mean. Now when I check mail and
rethink about it, that reply may make misunderstanding. It doesn't
actually makes sense to optimize, just a little code block moving. I now
agree with you that it doesn't optimize anything and may impact people's
code change. Sorry about that.

Thanks
Baoquan

Re: [PATCH v2 0/4] Clean up comments and codes in sparse_add_one_section()

[Baoquan He]

Talked to Michal, the local code refactorying may impact those big
feature or improvement patchset, e.g patch 2/4 and patch 3/4 will
conflict with Dan's patchset:
[PATCH v5 00/10] mm: Sub-section memory hotplug support

So I would like to discard them and only repost patch 1/4 and 4/4 after
addressing reviewers' concern. Sorry for the noise.

On 03/26/19 at 05:02pm, Baoquan He wrote:
> This is v2 post. V1 is here:
> http://lkml.kernel.org/r/20190320073540.12866-1-bhe@redhat.com
> 
> This patchset includes 4 patches. The first three patches are around
> sparse_add_one_section(). The last one is a simple clean up patch when
> review codes in hotplug path, carry it in this patchset.
> 
> Baoquan He (4):
>   mm/sparse: Clean up the obsolete code comment
>   mm/sparse: Optimize sparse_add_one_section()
>   mm/sparse: Rename function related to section memmap allocation/free
>   drivers/base/memory.c: Rename the misleading parameter
> 
>  drivers/base/memory.c |  6 ++---
>  mm/sparse.c           | 58 ++++++++++++++++++++++---------------------
>  2 files changed, 33 insertions(+), 31 deletions(-)
> 
> -- 
> 2.17.2
>