diff mbox series

[1/2] mm, memory_hotplug: cleanup memory offline path

Message ID 20190404125916.10215-2-osalvador@suse.de (mailing list archive)
State New, archived
Headers show
Series Preparing memhotplug for allocating memmap from hot-added range | expand

Commit Message

Oscar Salvador April 4, 2019, 12:59 p.m. UTC 
From: Michal Hocko <mhocko@suse.com>

check_pages_isolated_cb currently accounts the whole pfn range as being
offlined if test_pages_isolated suceeds on the range. This is based on
the assumption that all pages in the range are freed which is currently
the case in most cases but it won't be with later changes, as pages
marked as vmemmap won't be isolated.

Move the offlined pages counting to offline_isolated_pages_cb and
rely on __offline_isolated_pages to return the correct value.
check_pages_isolated_cb will still do it's primary job and check the pfn
range.

While we are at it remove check_pages_isolated and offline_isolated_pages
and use directly walk_system_ram_range as do in online_pages.

Signed-off-by: Michal Hocko <mhocko@suse.com>
Signed-off-by: Oscar Salvador <osalvador@suse.de>
---
 include/linux/memory_hotplug.h |  3 ++-
 mm/memory_hotplug.c            | 46 +++++++++++-------------------------------
 mm/page_alloc.c                | 11 ++++++++--
 3 files changed, 23 insertions(+), 37 deletions(-)

Comments

David Hildenbrand April 4, 2019, 1:18 p.m. UTC | #1 
On 04.04.19 14:59, Oscar Salvador wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> check_pages_isolated_cb currently accounts the whole pfn range as being
> offlined if test_pages_isolated suceeds on the range. This is based on
> the assumption that all pages in the range are freed which is currently
> the case in most cases but it won't be with later changes, as pages
> marked as vmemmap won't be isolated.
> 
> Move the offlined pages counting to offline_isolated_pages_cb and
> rely on __offline_isolated_pages to return the correct value.
> check_pages_isolated_cb will still do it's primary job and check the pfn
> range.
> 
> While we are at it remove check_pages_isolated and offline_isolated_pages
> and use directly walk_system_ram_range as do in online_pages.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> Signed-off-by: Oscar Salvador <osalvador@suse.de>
> ---
>  include/linux/memory_hotplug.h |  3 ++-
>  mm/memory_hotplug.c            | 46 +++++++++++-------------------------------
>  mm/page_alloc.c                | 11 ++++++++--
>  3 files changed, 23 insertions(+), 37 deletions(-)
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 8ade08c50d26..3c8cf347804c 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -87,7 +87,8 @@ extern int add_one_highpage(struct page *page, int pfn, int bad_ppro);
>  extern int online_pages(unsigned long, unsigned long, int);
>  extern int test_pages_in_a_zone(unsigned long start_pfn, unsigned long end_pfn,
>  	unsigned long *valid_start, unsigned long *valid_end);
> -extern void __offline_isolated_pages(unsigned long, unsigned long);
> +extern unsigned long __offline_isolated_pages(unsigned long start_pfn,
> +						unsigned long end_pfn);
>  
>  typedef void (*online_page_callback_t)(struct page *page, unsigned int order);
>  
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index f206b8b66af1..d8a3e9554aec 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1451,15 +1451,11 @@ static int
>  offline_isolated_pages_cb(unsigned long start, unsigned long nr_pages,
>  			void *data)
>  {
> -	__offline_isolated_pages(start, start + nr_pages);
> -	return 0;
> -}
> +	unsigned long offlined_pages;
>  
> -static void
> -offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
> -{
> -	walk_system_ram_range(start_pfn, end_pfn - start_pfn, NULL,
> -				offline_isolated_pages_cb);
> +	offlined_pages = __offline_isolated_pages(start, start + nr_pages);
> +	*(unsigned long *)data += offlined_pages;

unsigned long *offlined_pages = data;

*offlined_pages += __offline_isolated_pages(start, start + nr_pages);

> +	return 0;
>  }
>  
>  /*
> @@ -1469,26 +1465,7 @@ static int
>  check_pages_isolated_cb(unsigned long start_pfn, unsigned long nr_pages,
>  			void *data)
>  {
> -	int ret;
> -	long offlined = *(long *)data;
> -	ret = test_pages_isolated(start_pfn, start_pfn + nr_pages, true);
> -	offlined = nr_pages;
> -	if (!ret)
> -		*(long *)data += offlined;
> -	return ret;
> -}
> -
> -static long
> -check_pages_isolated(unsigned long start_pfn, unsigned long end_pfn)
> -{
> -	long offlined = 0;
> -	int ret;
> -
> -	ret = walk_system_ram_range(start_pfn, end_pfn - start_pfn, &offlined,
> -			check_pages_isolated_cb);
> -	if (ret < 0)
> -		offlined = (long)ret;
> -	return offlined;
> +	return test_pages_isolated(start_pfn, start_pfn + nr_pages, true);
>  }
>  
>  static int __init cmdline_parse_movable_node(char *p)
> @@ -1573,7 +1550,7 @@ static int __ref __offline_pages(unsigned long start_pfn,
>  		  unsigned long end_pfn)
>  {
>  	unsigned long pfn, nr_pages;
> -	long offlined_pages;
> +	unsigned long offlined_pages = 0;
>  	int ret, node, nr_isolate_pageblock;
>  	unsigned long flags;
>  	unsigned long valid_start, valid_end;
> @@ -1649,14 +1626,15 @@ static int __ref __offline_pages(unsigned long start_pfn,
>  			goto failed_removal_isolated;
>  		}
>  		/* check again */
> -		offlined_pages = check_pages_isolated(start_pfn, end_pfn);
> -	} while (offlined_pages < 0);
> +		ret = walk_system_ram_range(start_pfn, end_pfn - start_pfn, NULL,
> +							check_pages_isolated_cb);

indentation looks strange, but might be my mail client.

> +	} while (ret);
>  
> -	pr_info("Offlined Pages %ld\n", offlined_pages);
>  	/* Ok, all of our target is isolated.
>  	   We cannot do rollback at this point. */
> -	offline_isolated_pages(start_pfn, end_pfn);
> -
> +	walk_system_ram_range(start_pfn, end_pfn - start_pfn, &offlined_pages,
> +						offline_isolated_pages_cb);

dito

> +	pr_info("Offlined Pages %ld\n", offlined_pages);
>  	/*
>  	 * Onlining will reset pagetype flags and makes migrate type
>  	 * MOVABLE, so just need to decrease the number of isolated
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0c53807a2943..d36ca67064c9 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8375,7 +8375,7 @@ void zone_pcp_reset(struct zone *zone)
>   * All pages in the range must be in a single zone and isolated
>   * before calling this.
>   */
> -void
> +unsigned long
>  __offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
>  {
>  	struct page *page;
> @@ -8383,12 +8383,15 @@ __offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
>  	unsigned int order, i;
>  	unsigned long pfn;
>  	unsigned long flags;
> +	unsigned long offlined_pages = 0;
> +
>  	/* find the first valid pfn */
>  	for (pfn = start_pfn; pfn < end_pfn; pfn++)
>  		if (pfn_valid(pfn))
>  			break;
>  	if (pfn == end_pfn)
> -		return;
> +		return offlined_pages;
> +
>  	offline_mem_sections(pfn, end_pfn);
>  	zone = page_zone(pfn_to_page(pfn));
>  	spin_lock_irqsave(&zone->lock, flags);
> @@ -8406,12 +8409,14 @@ __offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
>  		if (unlikely(!PageBuddy(page) && PageHWPoison(page))) {
>  			pfn++;
>  			SetPageReserved(page);
> +			offlined_pages++;
>  			continue;
>  		}
>  
>  		BUG_ON(page_count(page));
>  		BUG_ON(!PageBuddy(page));
>  		order = page_order(page);
> +		offlined_pages += 1 << order;
>  #ifdef CONFIG_DEBUG_VM
>  		pr_info("remove from free list %lx %d %lx\n",
>  			pfn, 1 << order, end_pfn);
> @@ -8422,6 +8427,8 @@ __offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
>  		pfn += (1 << order);
>  	}
>  	spin_unlock_irqrestore(&zone->lock, flags);
> +
> +	return offlined_pages;
>  }
>  #endif
>  
> 


Only nits

Reviewed-by: David Hildenbrand <david@redhat.com>
Oscar Salvador April 4, 2019, 1:25 p.m. UTC | #2 
On Thu, Apr 04, 2019 at 03:18:00PM +0200, David Hildenbrand wrote:
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index f206b8b66af1..d8a3e9554aec 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -1451,15 +1451,11 @@ static int
> >  offline_isolated_pages_cb(unsigned long start, unsigned long nr_pages,
> >  			void *data)
> >  {
> > -	__offline_isolated_pages(start, start + nr_pages);
> > -	return 0;
> > -}
> > +	unsigned long offlined_pages;
> >  
> > -static void
> > -offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
> > -{
> > -	walk_system_ram_range(start_pfn, end_pfn - start_pfn, NULL,
> > -				offline_isolated_pages_cb);
> > +	offlined_pages = __offline_isolated_pages(start, start + nr_pages);
> > +	*(unsigned long *)data += offlined_pages;
> 
> unsigned long *offlined_pages = data;
> 
> *offlined_pages += __offline_isolated_pages(start, start + nr_pages);

Yeah, more readable.

> Only nits

About the identation, I double checked the code and it looks fine to me.
In [1] looks fine too, might be your mail client?

[1] https://patchwork.kernel.org/patch/10885571/

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

Thanks ;-)
David Hildenbrand April 4, 2019, 2:47 p.m. UTC | #3 
On 04.04.19 15:25, Oscar Salvador wrote:
> On Thu, Apr 04, 2019 at 03:18:00PM +0200, David Hildenbrand wrote:
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index f206b8b66af1..d8a3e9554aec 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -1451,15 +1451,11 @@ static int
>>>  offline_isolated_pages_cb(unsigned long start, unsigned long nr_pages,
>>>  			void *data)
>>>  {
>>> -	__offline_isolated_pages(start, start + nr_pages);
>>> -	return 0;
>>> -}
>>> +	unsigned long offlined_pages;
>>>  
>>> -static void
>>> -offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
>>> -{
>>> -	walk_system_ram_range(start_pfn, end_pfn - start_pfn, NULL,
>>> -				offline_isolated_pages_cb);
>>> +	offlined_pages = __offline_isolated_pages(start, start + nr_pages);
>>> +	*(unsigned long *)data += offlined_pages;
>>
>> unsigned long *offlined_pages = data;
>>
>> *offlined_pages += __offline_isolated_pages(start, start + nr_pages);
> 
> Yeah, more readable.
> 
>> Only nits
> 
> About the identation, I double checked the code and it looks fine to me.
> In [1] looks fine too, might be your mail client?
> 
> [1] https://patchwork.kernel.org/patch/10885571/

Double checked, alignment on the parameter on the new line is very weird.

And both lines cross 80 lines per line ... nit :)

> 
>>
>> Reviewed-by: David Hildenbrand <david@redhat.com>
> 
> Thanks ;-)
>
Oscar Salvador April 4, 2019, 3:40 p.m. UTC | #4 
On Thu, Apr 04, 2019 at 04:47:43PM +0200, David Hildenbrand wrote:
> On 04.04.19 15:25, Oscar Salvador wrote:
> > On Thu, Apr 04, 2019 at 03:18:00PM +0200, David Hildenbrand wrote:
> >>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> >>> index f206b8b66af1..d8a3e9554aec 100644
> >>> --- a/mm/memory_hotplug.c
> >>> +++ b/mm/memory_hotplug.c
> >>> @@ -1451,15 +1451,11 @@ static int
> >>>  offline_isolated_pages_cb(unsigned long start, unsigned long nr_pages,
> >>>  			void *data)
> >>>  {
> >>> -	__offline_isolated_pages(start, start + nr_pages);
> >>> -	return 0;
> >>> -}
> >>> +	unsigned long offlined_pages;
> >>>  
> >>> -static void
> >>> -offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
> >>> -{
> >>> -	walk_system_ram_range(start_pfn, end_pfn - start_pfn, NULL,
> >>> -				offline_isolated_pages_cb);
> >>> +	offlined_pages = __offline_isolated_pages(start, start + nr_pages);
> >>> +	*(unsigned long *)data += offlined_pages;
> >>
> >> unsigned long *offlined_pages = data;
> >>
> >> *offlined_pages += __offline_isolated_pages(start, start + nr_pages);
> > 
> > Yeah, more readable.
> > 
> >> Only nits
> > 
> > About the identation, I double checked the code and it looks fine to me.
> > In [1] looks fine too, might be your mail client?
> > 
> > [1] https://patchwork.kernel.org/patch/10885571/
> 
> Double checked, alignment on the parameter on the new line is very weird.

Uhm, are not you confused because we removed the "while (off...)", and
"ret =" gets idented right below "/*check again*".

Try to apply the patch and check whether you still see the issue.
I just checked out the branch and it looks fine to me.

> And both lines cross 80 lines per line ... nit :)

Yeah, 81 characters, but I decided to go with that rather than start doing
tricky things to accomplish 80 characters.
Maybe Andrew agrees, or he might slap me.
David Hildenbrand April 4, 2019, 4:50 p.m. UTC | #5 
On 04.04.19 17:40, Oscar Salvador wrote:
> On Thu, Apr 04, 2019 at 04:47:43PM +0200, David Hildenbrand wrote:
>> On 04.04.19 15:25, Oscar Salvador wrote:
>>> On Thu, Apr 04, 2019 at 03:18:00PM +0200, David Hildenbrand wrote:
>>>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>>>> index f206b8b66af1..d8a3e9554aec 100644
>>>>> --- a/mm/memory_hotplug.c
>>>>> +++ b/mm/memory_hotplug.c
>>>>> @@ -1451,15 +1451,11 @@ static int
>>>>>  offline_isolated_pages_cb(unsigned long start, unsigned long nr_pages,
>>>>>  			void *data)
>>>>>  {
>>>>> -	__offline_isolated_pages(start, start + nr_pages);
>>>>> -	return 0;
>>>>> -}
>>>>> +	unsigned long offlined_pages;
>>>>>  
>>>>> -static void
>>>>> -offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
>>>>> -{
>>>>> -	walk_system_ram_range(start_pfn, end_pfn - start_pfn, NULL,
>>>>> -				offline_isolated_pages_cb);
>>>>> +	offlined_pages = __offline_isolated_pages(start, start + nr_pages);
>>>>> +	*(unsigned long *)data += offlined_pages;
>>>>
>>>> unsigned long *offlined_pages = data;
>>>>
>>>> *offlined_pages += __offline_isolated_pages(start, start + nr_pages);
>>>
>>> Yeah, more readable.
>>>
>>>> Only nits
>>>
>>> About the identation, I double checked the code and it looks fine to me.
>>> In [1] looks fine too, might be your mail client?
>>>
>>> [1] https://patchwork.kernel.org/patch/10885571/
>>
>> Double checked, alignment on the parameter on the new line is very weird.
> 
> Uhm, are not you confused because we removed the "while (off...)", and
> "ret =" gets idented right below "/*check again*".
> 
> Try to apply the patch and check whether you still see the issue.
> I just checked out the branch and it looks fine to me.


That's what I did and it hurts my eyes (dropping two tabs, converting
tabs to spaces)

your patch:

ret = walk_system_ram_range(start_pfn, end_pfn - start_pfn, NULL,
                                        check_pages_isolated_cb);

vs.

ret = walk_system_ram_range(start_pfn, end_pfn - start_pfn, NULL,
                            check_pages_isolated_cb);


Just so we are on the same page, we usually indent parameters on
additional lines to the start of other parameters. Not to the end
of the previous line.

> 
>> And both lines cross 80 lines per line ... nit :)
> 
> Yeah, 81 characters, but I decided to go with that rather than start doing
> tricky things to accomplish 80 characters.
> Maybe Andrew agrees, or he might slap me.
> 

Why not simply


ret = walk_system_ram_range(start_pfn, end_pfn - start_pfn,
			    NULL, check_pages_isolated_cb);

just as we have in add_memory_resource along with walk_memory_range().


A lot of nit-picking, sorry :)
diff mbox series

Patch

diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 8ade08c50d26..3c8cf347804c 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -87,7 +87,8 @@  extern int add_one_highpage(struct page *page, int pfn, int bad_ppro);
 extern int online_pages(unsigned long, unsigned long, int);
 extern int test_pages_in_a_zone(unsigned long start_pfn, unsigned long end_pfn,
 	unsigned long *valid_start, unsigned long *valid_end);
-extern void __offline_isolated_pages(unsigned long, unsigned long);
+extern unsigned long __offline_isolated_pages(unsigned long start_pfn,
+						unsigned long end_pfn);
 
 typedef void (*online_page_callback_t)(struct page *page, unsigned int order);
 
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index f206b8b66af1..d8a3e9554aec 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1451,15 +1451,11 @@  static int
 offline_isolated_pages_cb(unsigned long start, unsigned long nr_pages,
 			void *data)
 {
-	__offline_isolated_pages(start, start + nr_pages);
-	return 0;
-}
+	unsigned long offlined_pages;
 
-static void
-offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
-{
-	walk_system_ram_range(start_pfn, end_pfn - start_pfn, NULL,
-				offline_isolated_pages_cb);
+	offlined_pages = __offline_isolated_pages(start, start + nr_pages);
+	*(unsigned long *)data += offlined_pages;
+	return 0;
 }
 
 /*
@@ -1469,26 +1465,7 @@  static int
 check_pages_isolated_cb(unsigned long start_pfn, unsigned long nr_pages,
 			void *data)
 {
-	int ret;
-	long offlined = *(long *)data;
-	ret = test_pages_isolated(start_pfn, start_pfn + nr_pages, true);
-	offlined = nr_pages;
-	if (!ret)
-		*(long *)data += offlined;
-	return ret;
-}
-
-static long
-check_pages_isolated(unsigned long start_pfn, unsigned long end_pfn)
-{
-	long offlined = 0;
-	int ret;
-
-	ret = walk_system_ram_range(start_pfn, end_pfn - start_pfn, &offlined,
-			check_pages_isolated_cb);
-	if (ret < 0)
-		offlined = (long)ret;
-	return offlined;
+	return test_pages_isolated(start_pfn, start_pfn + nr_pages, true);
 }
 
 static int __init cmdline_parse_movable_node(char *p)
@@ -1573,7 +1550,7 @@  static int __ref __offline_pages(unsigned long start_pfn,
 		  unsigned long end_pfn)
 {
 	unsigned long pfn, nr_pages;
-	long offlined_pages;
+	unsigned long offlined_pages = 0;
 	int ret, node, nr_isolate_pageblock;
 	unsigned long flags;
 	unsigned long valid_start, valid_end;
@@ -1649,14 +1626,15 @@  static int __ref __offline_pages(unsigned long start_pfn,
 			goto failed_removal_isolated;
 		}
 		/* check again */
-		offlined_pages = check_pages_isolated(start_pfn, end_pfn);
-	} while (offlined_pages < 0);
+		ret = walk_system_ram_range(start_pfn, end_pfn - start_pfn, NULL,
+							check_pages_isolated_cb);
+	} while (ret);
 
-	pr_info("Offlined Pages %ld\n", offlined_pages);
 	/* Ok, all of our target is isolated.
 	   We cannot do rollback at this point. */
-	offline_isolated_pages(start_pfn, end_pfn);
-
+	walk_system_ram_range(start_pfn, end_pfn - start_pfn, &offlined_pages,
+						offline_isolated_pages_cb);
+	pr_info("Offlined Pages %ld\n", offlined_pages);
 	/*
 	 * Onlining will reset pagetype flags and makes migrate type
 	 * MOVABLE, so just need to decrease the number of isolated
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0c53807a2943..d36ca67064c9 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -8375,7 +8375,7 @@  void zone_pcp_reset(struct zone *zone)
  * All pages in the range must be in a single zone and isolated
  * before calling this.
  */
-void
+unsigned long
 __offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
 {
 	struct page *page;
@@ -8383,12 +8383,15 @@  __offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
 	unsigned int order, i;
 	unsigned long pfn;
 	unsigned long flags;
+	unsigned long offlined_pages = 0;
+
 	/* find the first valid pfn */
 	for (pfn = start_pfn; pfn < end_pfn; pfn++)
 		if (pfn_valid(pfn))
 			break;
 	if (pfn == end_pfn)
-		return;
+		return offlined_pages;
+
 	offline_mem_sections(pfn, end_pfn);
 	zone = page_zone(pfn_to_page(pfn));
 	spin_lock_irqsave(&zone->lock, flags);
@@ -8406,12 +8409,14 @@  __offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
 		if (unlikely(!PageBuddy(page) && PageHWPoison(page))) {
 			pfn++;
 			SetPageReserved(page);
+			offlined_pages++;
 			continue;
 		}
 
 		BUG_ON(page_count(page));
 		BUG_ON(!PageBuddy(page));
 		order = page_order(page);
+		offlined_pages += 1 << order;
 #ifdef CONFIG_DEBUG_VM
 		pr_info("remove from free list %lx %d %lx\n",
 			pfn, 1 << order, end_pfn);
@@ -8422,6 +8427,8 @@  __offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
 		pfn += (1 << order);
 	}
 	spin_unlock_irqrestore(&zone->lock, flags);
+
+	return offlined_pages;
 }
 #endif