[PATCH 0/4] A few cleanup patches around memory_hotplug

[PATCH 0/4] A few cleanup patches around memory_hotplug

[Miaohe Lin]

Hi,
This series contains a few patches to fix obsolete and misplaced comments,
clean up the try_offline_node function and so on. More details can be
found in the respective changelogs. Thanks!

Miaohe Lin (4):
  mm/memory_hotplug: remove obsolete comment of __add_pages
  mm/memory_hotplug: avoid calling zone_intersects() for ZONE_NORMAL
  mm/memory_hotplug: clean up try_offline_node
  mm/memory_hotplug: fix misplaced comment in offline_pages

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

-- 
2.23.0

[PATCH 1/4] mm/memory_hotplug: remove obsolete comment of __add_pages

[Miaohe Lin]

Since commit f1dd2cd13c4b ("mm, memory_hotplug: do not associate hotadded
memory to zones until online"), there is no need to pass in the zone.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory_hotplug.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index a4f69d399929..cbc67c27e0dd 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -296,10 +296,7 @@ struct page *pfn_to_online_page(unsigned long pfn)
 EXPORT_SYMBOL_GPL(pfn_to_online_page);
 
 /*
- * Reasonably generic function for adding memory.  It is
- * expected that archs that support memory hotplug will
- * call this function after deciding the zone to which to
- * add the new pages.
+ * Reasonably generic function for adding memory.
  */
 int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
 		struct mhp_params *params)
-- 
2.23.0

[PATCH 2/4] mm/memory_hotplug: avoid calling zone_intersects() for ZONE_NORMAL

[Miaohe Lin]

If zid reaches ZONE_NORMAL, the caller will always get the NORMAL zone no
matter what zone_intersects() returns. So we can save some possible cpu
cycles by avoid calling zone_intersects() for ZONE_NORMAL.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index cbc67c27e0dd..140809e60e9a 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -826,7 +826,7 @@ static struct zone *default_kernel_zone_for_pfn(int nid, unsigned long start_pfn
 	struct pglist_data *pgdat = NODE_DATA(nid);
 	int zid;
 
-	for (zid = 0; zid <= ZONE_NORMAL; zid++) {
+	for (zid = 0; zid < ZONE_NORMAL; zid++) {
 		struct zone *zone = &pgdat->node_zones[zid];
 
 		if (zone_intersects(zone, start_pfn, nr_pages))
-- 
2.23.0

[PATCH 3/4] mm/memory_hotplug: clean up try_offline_node

[Miaohe Lin]

We can use helper macro node_spanned_pages to check whether node spans
pages. And we can change the parameter of check_cpu_on_node to nid as
that's what it really cares. Thus we can further get rid of the local
variable pgdat and improve the readability a bit.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory_hotplug.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 140809e60e9a..4b9eef861ee4 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -2008,12 +2008,12 @@ static int get_nr_vmemmap_pages_cb(struct memory_block *mem, void *arg)
 	return mem->nr_vmemmap_pages;
 }
 
-static int check_cpu_on_node(pg_data_t *pgdat)
+static int check_cpu_on_node(int nid)
 {
 	int cpu;
 
 	for_each_present_cpu(cpu) {
-		if (cpu_to_node(cpu) == pgdat->node_id)
+		if (cpu_to_node(cpu) == nid)
 			/*
 			 * the cpu on this node isn't removed, and we can't
 			 * offline this node.
@@ -2047,7 +2047,6 @@ static int check_no_memblock_for_node_cb(struct memory_block *mem, void *arg)
  */
 void try_offline_node(int nid)
 {
-	pg_data_t *pgdat = NODE_DATA(nid);
 	int rc;
 
 	/*
@@ -2055,7 +2054,7 @@ void try_offline_node(int nid)
 	 * offline it. A node spans memory after move_pfn_range_to_zone(),
 	 * e.g., after the memory block was onlined.
 	 */
-	if (pgdat->node_spanned_pages)
+	if (node_spanned_pages(nid))
 		return;
 
 	/*
@@ -2067,7 +2066,7 @@ void try_offline_node(int nid)
 	if (rc)
 		return;
 
-	if (check_cpu_on_node(pgdat))
+	if (check_cpu_on_node(nid))
 		return;
 
 	/*
-- 
2.23.0

[PATCH 4/4] mm/memory_hotplug: fix misplaced comment in offline_pages

[Miaohe Lin]

It's misplaced since commit 7960509329c2 ("mm, memory_hotplug: print reason
for the offlining failure"). Move it to the right place.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory_hotplug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 4b9eef861ee4..7dc7e12302db 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1966,6 +1966,7 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
 	return 0;
 
 failed_removal_isolated:
+	/* pushback to free area */
 	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
 	memory_notify(MEM_CANCEL_OFFLINE, &arg);
 failed_removal_pcplists_disabled:
@@ -1976,7 +1977,6 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
 		 (unsigned long long) start_pfn << PAGE_SHIFT,
 		 ((unsigned long long) end_pfn << PAGE_SHIFT) - 1,
 		 reason);
-	/* pushback to free area */
 	mem_hotplug_done();
 	return ret;
 }
-- 
2.23.0

Re: [PATCH 1/4] mm/memory_hotplug: remove obsolete comment of __add_pages

[David Hildenbrand]

On 07.02.22 14:36, Miaohe Lin wrote:
> Since commit f1dd2cd13c4b ("mm, memory_hotplug: do not associate hotadded
> memory to zones until online"), there is no need to pass in the zone.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/memory_hotplug.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index a4f69d399929..cbc67c27e0dd 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -296,10 +296,7 @@ struct page *pfn_to_online_page(unsigned long pfn)
>  EXPORT_SYMBOL_GPL(pfn_to_online_page);
>  
>  /*
> - * Reasonably generic function for adding memory.  It is
> - * expected that archs that support memory hotplug will
> - * call this function after deciding the zone to which to
> - * add the new pages.
> + * Reasonably generic function for adding memory.
>   */
>  int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
>  		struct mhp_params *params)

I'd suggest just removing the comment completely.

-- 
Thanks,

David / dhildenb

Re: [PATCH 4/4] mm/memory_hotplug: fix misplaced comment in offline_pages

[David Hildenbrand]

On 07.02.22 14:36, Miaohe Lin wrote:
> It's misplaced since commit 7960509329c2 ("mm, memory_hotplug: print reason
> for the offlining failure"). Move it to the right place.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/memory_hotplug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 4b9eef861ee4..7dc7e12302db 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1966,6 +1966,7 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>  	return 0;
>  
>  failed_removal_isolated:
> +	/* pushback to free area */
>  	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
>  	memory_notify(MEM_CANCEL_OFFLINE, &arg);
>  failed_removal_pcplists_disabled:
> @@ -1976,7 +1977,6 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>  		 (unsigned long long) start_pfn << PAGE_SHIFT,
>  		 ((unsigned long long) end_pfn << PAGE_SHIFT) - 1,
>  		 reason);
> -	/* pushback to free area */
>  	mem_hotplug_done();
>  	return ret;
>  }

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 3/4] mm/memory_hotplug: clean up try_offline_node

[David Hildenbrand]

On 07.02.22 14:36, Miaohe Lin wrote:
> We can use helper macro node_spanned_pages to check whether node spans
> pages. And we can change the parameter of check_cpu_on_node to nid as
> that's what it really cares. Thus we can further get rid of the local
> variable pgdat and improve the readability a bit.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/memory_hotplug.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 140809e60e9a..4b9eef861ee4 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -2008,12 +2008,12 @@ static int get_nr_vmemmap_pages_cb(struct memory_block *mem, void *arg)
>  	return mem->nr_vmemmap_pages;
>  }
>  
> -static int check_cpu_on_node(pg_data_t *pgdat)
> +static int check_cpu_on_node(int nid)
>  {
>  	int cpu;
>  
>  	for_each_present_cpu(cpu) {
> -		if (cpu_to_node(cpu) == pgdat->node_id)
> +		if (cpu_to_node(cpu) == nid)
>  			/*
>  			 * the cpu on this node isn't removed, and we can't
>  			 * offline this node.
> @@ -2047,7 +2047,6 @@ static int check_no_memblock_for_node_cb(struct memory_block *mem, void *arg)
>   */
>  void try_offline_node(int nid)
>  {
> -	pg_data_t *pgdat = NODE_DATA(nid);
>  	int rc;
>  
>  	/*
> @@ -2055,7 +2054,7 @@ void try_offline_node(int nid)
>  	 * offline it. A node spans memory after move_pfn_range_to_zone(),
>  	 * e.g., after the memory block was onlined.
>  	 */
> -	if (pgdat->node_spanned_pages)
> +	if (node_spanned_pages(nid))
>  		return;
>  
>  	/*
> @@ -2067,7 +2066,7 @@ void try_offline_node(int nid)
>  	if (rc)
>  		return;
>  
> -	if (check_cpu_on_node(pgdat))
> +	if (check_cpu_on_node(nid))
>  		return;
>  
>  	/*

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/4] mm/memory_hotplug: avoid calling zone_intersects() for ZONE_NORMAL

[David Hildenbrand]

On 07.02.22 14:36, Miaohe Lin wrote:
> If zid reaches ZONE_NORMAL, the caller will always get the NORMAL zone no
> matter what zone_intersects() returns. So we can save some possible cpu
> cycles by avoid calling zone_intersects() for ZONE_NORMAL.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/memory_hotplug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index cbc67c27e0dd..140809e60e9a 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -826,7 +826,7 @@ static struct zone *default_kernel_zone_for_pfn(int nid, unsigned long start_pfn
>  	struct pglist_data *pgdat = NODE_DATA(nid);
>  	int zid;
>  
> -	for (zid = 0; zid <= ZONE_NORMAL; zid++) {
> +	for (zid = 0; zid < ZONE_NORMAL; zid++) {
>  		struct zone *zone = &pgdat->node_zones[zid];
>  
>  		if (zone_intersects(zone, start_pfn, nr_pages))


Makes sense, although we just don't care about the CPU cycles at that point.

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/4] mm/memory_hotplug: remove obsolete comment of __add_pages

[Andrew Morton]

On Mon, 7 Feb 2022 15:41:21 +0100 David Hildenbrand <david@redhat.com> wrote:

> On 07.02.22 14:36, Miaohe Lin wrote:
> > Since commit f1dd2cd13c4b ("mm, memory_hotplug: do not associate hotadded
> > memory to zones until online"), there is no need to pass in the zone.
> > 
> > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> > ---
> >  mm/memory_hotplug.c | 5 +----
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index a4f69d399929..cbc67c27e0dd 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -296,10 +296,7 @@ struct page *pfn_to_online_page(unsigned long pfn)
> >  EXPORT_SYMBOL_GPL(pfn_to_online_page);
> >  
> >  /*
> > - * Reasonably generic function for adding memory.  It is
> > - * expected that archs that support memory hotplug will
> > - * call this function after deciding the zone to which to
> > - * add the new pages.
> > + * Reasonably generic function for adding memory.
> >   */
> >  int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages,
> >  		struct mhp_params *params)
> 
> I'd suggest just removing the comment completely.

Thanks, I made that change.

A better site for documentation is at add_memory().  Which, as a
full-on exported-to-modules API function, should really have some
docs...

Re: [PATCH 2/4] mm/memory_hotplug: avoid calling zone_intersects() for ZONE_NORMAL

[Oscar Salvador]

On Mon, Feb 07, 2022 at 09:36:41PM +0800, Miaohe Lin wrote:
> If zid reaches ZONE_NORMAL, the caller will always get the NORMAL zone no
> matter what zone_intersects() returns. So we can save some possible cpu
> cycles by avoid calling zone_intersects() for ZONE_NORMAL.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Oscar Salvador <osalvador@suse.de>

> ---
>  mm/memory_hotplug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index cbc67c27e0dd..140809e60e9a 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -826,7 +826,7 @@ static struct zone *default_kernel_zone_for_pfn(int nid, unsigned long start_pfn
>  	struct pglist_data *pgdat = NODE_DATA(nid);
>  	int zid;
>  
> -	for (zid = 0; zid <= ZONE_NORMAL; zid++) {
> +	for (zid = 0; zid < ZONE_NORMAL; zid++) {
>  		struct zone *zone = &pgdat->node_zones[zid];
>  
>  		if (zone_intersects(zone, start_pfn, nr_pages))
> -- 
> 2.23.0
> 
> 

-- 
Oscar Salvador
SUSE Labs

Re: [PATCH 3/4] mm/memory_hotplug: clean up try_offline_node

[Oscar Salvador]

On Mon, Feb 07, 2022 at 09:36:42PM +0800, Miaohe Lin wrote:
> We can use helper macro node_spanned_pages to check whether node spans
> pages. And we can change the parameter of check_cpu_on_node to nid as
> that's what it really cares. Thus we can further get rid of the local
> variable pgdat and improve the readability a bit.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Oscar Salvador <osalvador@suse.de>

> ---
>  mm/memory_hotplug.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 140809e60e9a..4b9eef861ee4 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -2008,12 +2008,12 @@ static int get_nr_vmemmap_pages_cb(struct memory_block *mem, void *arg)
>  	return mem->nr_vmemmap_pages;
>  }
>  
> -static int check_cpu_on_node(pg_data_t *pgdat)
> +static int check_cpu_on_node(int nid)
>  {
>  	int cpu;
>  
>  	for_each_present_cpu(cpu) {
> -		if (cpu_to_node(cpu) == pgdat->node_id)
> +		if (cpu_to_node(cpu) == nid)
>  			/*
>  			 * the cpu on this node isn't removed, and we can't
>  			 * offline this node.
> @@ -2047,7 +2047,6 @@ static int check_no_memblock_for_node_cb(struct memory_block *mem, void *arg)
>   */
>  void try_offline_node(int nid)
>  {
> -	pg_data_t *pgdat = NODE_DATA(nid);
>  	int rc;
>  
>  	/*
> @@ -2055,7 +2054,7 @@ void try_offline_node(int nid)
>  	 * offline it. A node spans memory after move_pfn_range_to_zone(),
>  	 * e.g., after the memory block was onlined.
>  	 */
> -	if (pgdat->node_spanned_pages)
> +	if (node_spanned_pages(nid))
>  		return;
>  
>  	/*
> @@ -2067,7 +2066,7 @@ void try_offline_node(int nid)
>  	if (rc)
>  		return;
>  
> -	if (check_cpu_on_node(pgdat))
> +	if (check_cpu_on_node(nid))
>  		return;
>  
>  	/*
> -- 
> 2.23.0
> 
> 

-- 
Oscar Salvador
SUSE Labs

Re: [PATCH 4/4] mm/memory_hotplug: fix misplaced comment in offline_pages

[Oscar Salvador]

On Mon, Feb 07, 2022 at 09:36:43PM +0800, Miaohe Lin wrote:
> It's misplaced since commit 7960509329c2 ("mm, memory_hotplug: print reason
> for the offlining failure"). Move it to the right place.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Oscar Salvador <osalvador@suse.de>

> ---
>  mm/memory_hotplug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 4b9eef861ee4..7dc7e12302db 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1966,6 +1966,7 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>  	return 0;
>  
>  failed_removal_isolated:
> +	/* pushback to free area */
>  	undo_isolate_page_range(start_pfn, end_pfn, MIGRATE_MOVABLE);
>  	memory_notify(MEM_CANCEL_OFFLINE, &arg);
>  failed_removal_pcplists_disabled:
> @@ -1976,7 +1977,6 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>  		 (unsigned long long) start_pfn << PAGE_SHIFT,
>  		 ((unsigned long long) end_pfn << PAGE_SHIFT) - 1,
>  		 reason);
> -	/* pushback to free area */
>  	mem_hotplug_done();
>  	return ret;
>  }
> -- 
> 2.23.0
> 
> 

-- 
Oscar Salvador
SUSE Labs

Re: [PATCH 1/4] mm/memory_hotplug: remove obsolete comment of __add_pages

[Oscar Salvador]

On Mon, Feb 07, 2022 at 09:36:40PM +0800, Miaohe Lin wrote:
> Since commit f1dd2cd13c4b ("mm, memory_hotplug: do not associate hotadded
> memory to zones until online"), there is no need to pass in the zone.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Yeah, I am with David here, "Reasonably generic function for adding
memory" does not really tell me much about that function.
 

-- 
Oscar Salvador
SUSE Labs

Re: [PATCH 1/4] mm/memory_hotplug: remove obsolete comment of __add_pages

[Miaohe Lin]

On 2022/2/8 18:15, Oscar Salvador wrote:
> On Mon, Feb 07, 2022 at 09:36:40PM +0800, Miaohe Lin wrote:
>> Since commit f1dd2cd13c4b ("mm, memory_hotplug: do not associate hotadded
>> memory to zones until online"), there is no need to pass in the zone.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> 
> Yeah, I am with David here, "Reasonably generic function for adding
> memory" does not really tell me much about that function.
>  

Agree with you. And Andrew have kindly made that change.
Many thanks for your review. :)