[v2,03/10] mm/memory_hotplug: simplify page offlining
diff mbox series

Message ID 20200819175957.28465-4-david@redhat.com
State In Next
Commit 7de69a3eb49fe8e72acb361803a0c502a317f5dd
Headers show
Series
  • mm/memory_hotplug: online_pages()/offline_pages() cleanups
Related show

Commit Message

David Hildenbrand Aug. 19, 2020, 5:59 p.m. UTC
We make sure that we cannot have any memory holes right at the beginning
of offline_pages(). We no longer need walk_system_ram_range() and can
call test_pages_isolated() and __offline_isolated_pages() directly.

offlined_pages always corresponds to nr_pages, so we can simplify that.

Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
Cc: Baoquan He <bhe@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Oscar Salvador <osalvador@suse.de>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 mm/memory_hotplug.c | 46 ++++++++++-----------------------------------
 1 file changed, 10 insertions(+), 36 deletions(-)

Comments

Oscar Salvador Aug. 24, 2020, 10:44 a.m. UTC | #1
On Wed, Aug 19, 2020 at 07:59:50PM +0200, David Hildenbrand wrote:
> We make sure that we cannot have any memory holes right at the beginning
> of offline_pages(). We no longer need walk_system_ram_range() and can
> call test_pages_isolated() and __offline_isolated_pages() directly.
> 
> offlined_pages always corresponds to nr_pages, so we can simplify that.
> 
> Acked-by: Michal Hocko <mhocko@suse.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> Cc: Oscar Salvador <osalvador@suse.de>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Oscar Salvador <osalvador@suse.de>
Andrew Morton Sept. 3, 2020, 9:58 p.m. UTC | #2
On Wed, 19 Aug 2020 19:59:50 +0200 David Hildenbrand <david@redhat.com> wrote:

> We make sure that we cannot have any memory holes right at the beginning
> of offline_pages(). We no longer need walk_system_ram_range() and can
> call test_pages_isolated() and __offline_isolated_pages() directly.
> 
> offlined_pages always corresponds to nr_pages, so we can simplify that.

This patch ran afoul of Pavel's "mm/memory_hotplug: drain per-cpu pages
again during memory offline", here:

> @@ -1481,7 +1459,7 @@ static int count_system_ram_pages_cb(unsigned long start_pfn,
>  int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages)
>  {
>  	const unsigned long end_pfn = start_pfn + nr_pages;
> -	unsigned long pfn, system_ram_pages = 0, offlined_pages = 0;
> +	unsigned long pfn, system_ram_pages = 0;
>  	int ret, node, nr_isolate_pageblock;
>  	unsigned long flags;
>  	struct zone *zone;
> @@ -1579,16 +1557,12 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages)
>  			reason = "failure to dissolve huge pages";
>  			goto failed_removal_isolated;
>  		}
> -		/* check again */
> -		ret = walk_system_ram_range(start_pfn, end_pfn - start_pfn,
> -					    NULL, check_pages_isolated_cb);
> -	} while (ret);
> -
> -	/* Ok, all of our target is isolated.
> -	   We cannot do rollback at this point. */
> -	walk_system_ram_range(start_pfn, end_pfn - start_pfn,
> -			      &offlined_pages, offline_isolated_pages_cb);
> -	pr_info("Offlined Pages %ld\n", offlined_pages);
> +	} while (test_pages_isolated(start_pfn, end_pfn, MEMORY_OFFLINE));
> +
> +	/* Mark all sections offline and remove free pages from the buddy. */
> +	__offline_isolated_pages(start_pfn, end_pfn);
> +	pr_info("Offlined Pages %ld\n", nr_pages);
> +
>  	/*
>  	 * Onlining will reset pagetype flags and makes migrate type

I did this.  Looks OK?

From: David Hildenbrand <david@redhat.com>
Subject: mm/memory_hotplug: simplify page offlining

We make sure that we cannot have any memory holes right at the beginning
of offline_pages().  We no longer need walk_system_ram_range() and can
call test_pages_isolated() and __offline_isolated_pages() directly.

offlined_pages always corresponds to nr_pages, so we can simplify that.

Link: https://lkml.kernel.org/r/20200819175957.28465-4-david@redhat.com
Signed-off-by: David Hildenbrand <david@redhat.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Reviewed-by: Oscar Salvador <osalvador@suse.de>
Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
Cc: Baoquan He <bhe@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Charan Teja Reddy <charante@codeaurora.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Michel Lespinasse <walken@google.com>
Cc: Mike Rapoport <rppt@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/memory_hotplug.c |   61 +++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 36 deletions(-)

--- a/mm/memory_hotplug.c~mm-memory_hotplug-simplify-page-offlining
+++ a/mm/memory_hotplug.c
@@ -1383,28 +1383,6 @@ do_migrate_range(unsigned long start_pfn
 	return ret;
 }
 
-/* Mark all sections offline and remove all free pages from the buddy. */
-static int
-offline_isolated_pages_cb(unsigned long start, unsigned long nr_pages,
-			void *data)
-{
-	unsigned long *offlined_pages = (unsigned long *)data;
-
-	*offlined_pages += __offline_isolated_pages(start, start + nr_pages);
-	return 0;
-}
-
-/*
- * Check all pages in range, recorded as memory resource, are isolated.
- */
-static int
-check_pages_isolated_cb(unsigned long start_pfn, unsigned long nr_pages,
-			void *data)
-{
-	return test_pages_isolated(start_pfn, start_pfn + nr_pages,
-				   MEMORY_OFFLINE);
-}
-
 static int __init cmdline_parse_movable_node(char *p)
 {
 	movable_node_enabled = true;
@@ -1491,7 +1469,7 @@ static int count_system_ram_pages_cb(uns
 int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages)
 {
 	const unsigned long end_pfn = start_pfn + nr_pages;
-	unsigned long pfn, system_ram_pages = 0, offlined_pages = 0;
+	unsigned long pfn, system_ram_pages = 0;
 	int ret, node, nr_isolate_pageblock;
 	unsigned long flags;
 	struct zone *zone;
@@ -1589,16 +1567,27 @@ int __ref offline_pages(unsigned long st
 			reason = "failure to dissolve huge pages";
 			goto failed_removal_isolated;
 		}
-		/* check again */
-		ret = walk_system_ram_range(start_pfn, end_pfn - start_pfn,
-					    NULL, check_pages_isolated_cb);
-	} while (ret);
-
-	/* Ok, all of our target is isolated.
-	   We cannot do rollback at this point. */
-	walk_system_ram_range(start_pfn, end_pfn - start_pfn,
-			      &offlined_pages, offline_isolated_pages_cb);
-	pr_info("Offlined Pages %ld\n", offlined_pages);
+
+		/*
+		 * per-cpu pages are drained in start_isolate_page_range, but if
+		 * there are still pages that are not free, make sure that we
+		 * drain again, because when we isolated range we might
+		 * have raced with another thread that was adding pages to pcp
+		 * list.
+		 *
+		 * Forward progress should be still guaranteed because
+		 * pages on the pcp list can only belong to MOVABLE_ZONE
+		 * because has_unmovable_pages explicitly checks for
+		 * PageBuddy on freed pages on other zones.
+		 */
+		if (ret)
+			drain_all_pages(zone);
+	} while (test_pages_isolated(start_pfn, end_pfn, MEMORY_OFFLINE));
+
+	/* Mark all sections offline and remove free pages from the buddy. */
+	__offline_isolated_pages(start_pfn, end_pfn);
+	pr_info("Offlined Pages %ld\n", nr_pages);
+
 	/*
 	 * Onlining will reset pagetype flags and makes migrate type
 	 * MOVABLE, so just need to decrease the number of isolated
@@ -1609,11 +1598,11 @@ int __ref offline_pages(unsigned long st
 	spin_unlock_irqrestore(&zone->lock, flags);
 
 	/* removal success */
-	adjust_managed_page_count(pfn_to_page(start_pfn), -offlined_pages);
-	zone->present_pages -= offlined_pages;
+	adjust_managed_page_count(pfn_to_page(start_pfn), -nr_pages);
+	zone->present_pages -= nr_pages;
 
 	pgdat_resize_lock(zone->zone_pgdat, &flags);
-	zone->zone_pgdat->node_present_pages -= offlined_pages;
+	zone->zone_pgdat->node_present_pages -= nr_pages;
 	pgdat_resize_unlock(zone->zone_pgdat, &flags);
 
 	init_per_zone_wmark_min();
David Hildenbrand Sept. 4, 2020, 5:47 a.m. UTC | #3
> Am 03.09.2020 um 23:58 schrieb Andrew Morton <akpm@linux-foundation.org>:
> 
> ´╗┐On Wed, 19 Aug 2020 19:59:50 +0200 David Hildenbrand <david@redhat.com> wrote:
> 
>> We make sure that we cannot have any memory holes right at the beginning
>> of offline_pages(). We no longer need walk_system_ram_range() and can
>> call test_pages_isolated() and __offline_isolated_pages() directly.
>> 
>> offlined_pages always corresponds to nr_pages, so we can simplify that.
> 
> This patch ran afoul of Pavel's "mm/memory_hotplug: drain per-cpu pages
> again during memory offline", here:
> 
>> @@ -1481,7 +1459,7 @@ static int count_system_ram_pages_cb(unsigned long start_pfn,
>> int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages)
>> {
>>    const unsigned long end_pfn = start_pfn + nr_pages;
>> -    unsigned long pfn, system_ram_pages = 0, offlined_pages = 0;
>> +    unsigned long pfn, system_ram_pages = 0;
>>    int ret, node, nr_isolate_pageblock;
>>    unsigned long flags;
>>    struct zone *zone;
>> @@ -1579,16 +1557,12 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages)
>>            reason = "failure to dissolve huge pages";
>>            goto failed_removal_isolated;
>>        }
>> -        /* check again */
>> -        ret = walk_system_ram_range(start_pfn, end_pfn - start_pfn,
>> -                        NULL, check_pages_isolated_cb);
>> -    } while (ret);
>> -
>> -    /* Ok, all of our target is isolated.
>> -       We cannot do rollback at this point. */
>> -    walk_system_ram_range(start_pfn, end_pfn - start_pfn,
>> -                  &offlined_pages, offline_isolated_pages_cb);
>> -    pr_info("Offlined Pages %ld\n", offlined_pages);
>> +    } while (test_pages_isolated(start_pfn, end_pfn, MEMORY_OFFLINE));
>> +
>> +    /* Mark all sections offline and remove free pages from the buddy. */
>> +    __offline_isolated_pages(start_pfn, end_pfn);
>> +    pr_info("Offlined Pages %ld\n", nr_pages);
>> +
>>    /*
>>     * Onlining will reset pagetype flags and makes migrate type
> 
> I did this.  Looks OK?
> 

Reading on my smartphone, it looks like you squashed both patches?

> From: David Hildenbrand <david@redhat.com>
> Subject: mm/memory_hotplug: simplify page offlining
> 
> We make sure that we cannot have any memory holes right at the beginning
> of offline_pages().  We no longer need walk_system_ram_range() and can
> call test_pages_isolated() and __offline_isolated_pages() directly.
> 
> offlined_pages always corresponds to nr_pages, so we can simplify that.
> 
> Link: https://lkml.kernel.org/r/20200819175957.28465-4-david@redhat.com
> Signed-off-by: David Hildenbrand <david@redhat.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Reviewed-by: Oscar Salvador <osalvador@suse.de>
> Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
> Cc: Baoquan He <bhe@redhat.com>
> Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
> Cc: Charan Teja Reddy <charante@codeaurora.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Fenghua Yu <fenghua.yu@intel.com>
> Cc: Logan Gunthorpe <logang@deltatee.com>
> Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Michel Lespinasse <walken@google.com>
> Cc: Mike Rapoport <rppt@kernel.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
> mm/memory_hotplug.c |   61 +++++++++++++++++-------------------------
> 1 file changed, 25 insertions(+), 36 deletions(-)
> 
> --- a/mm/memory_hotplug.c~mm-memory_hotplug-simplify-page-offlining
> +++ a/mm/memory_hotplug.c
> @@ -1383,28 +1383,6 @@ do_migrate_range(unsigned long start_pfn
>    return ret;
> }
> 
> -/* Mark all sections offline and remove all free pages from the buddy. */
> -static int
> -offline_isolated_pages_cb(unsigned long start, unsigned long nr_pages,
> -            void *data)
> -{
> -    unsigned long *offlined_pages = (unsigned long *)data;
> -
> -    *offlined_pages += __offline_isolated_pages(start, start + nr_pages);
> -    return 0;
> -}
> -
> -/*
> - * Check all pages in range, recorded as memory resource, are isolated.
> - */
> -static int
> -check_pages_isolated_cb(unsigned long start_pfn, unsigned long nr_pages,
> -            void *data)
> -{
> -    return test_pages_isolated(start_pfn, start_pfn + nr_pages,
> -                   MEMORY_OFFLINE);
> -}
> -
> static int __init cmdline_parse_movable_node(char *p)
> {
>    movable_node_enabled = true;
> @@ -1491,7 +1469,7 @@ static int count_system_ram_pages_cb(uns
> int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages)
> {
>    const unsigned long end_pfn = start_pfn + nr_pages;
> -    unsigned long pfn, system_ram_pages = 0, offlined_pages = 0;
> +    unsigned long pfn, system_ram_pages = 0;
>    int ret, node, nr_isolate_pageblock;
>    unsigned long flags;
>    struct zone *zone;
> @@ -1589,16 +1567,27 @@ int __ref offline_pages(unsigned long st
>            reason = "failure to dissolve huge pages";
>            goto failed_removal_isolated;
>        }
> -        /* check again */
> -        ret = walk_system_ram_range(start_pfn, end_pfn - start_pfn,
> -                        NULL, check_pages_isolated_cb);
> -    } while (ret);
> -
> -    /* Ok, all of our target is isolated.
> -       We cannot do rollback at this point. */
> -    walk_system_ram_range(start_pfn, end_pfn - start_pfn,
> -                  &offlined_pages, offline_isolated_pages_cb);
> -    pr_info("Offlined Pages %ld\n", offlined_pages);
> +
> +        /*
> +         * per-cpu pages are drained in start_isolate_page_range, but if
> +         * there are still pages that are not free, make sure that we
> +         * drain again, because when we isolated range we might
> +         * have raced with another thread that was adding pages to pcp
> +         * list.
> +         *
> +         * Forward progress should be still guaranteed because
> +         * pages on the pcp list can only belong to MOVABLE_ZONE
> +         * because has_unmovable_pages explicitly checks for
> +         * PageBuddy on freed pages on other zones.
> +         */
> +        if (ret)
> +            drain_all_pages(zone);
> +    } while (test_pages_isolated(start_pfn, end_pfn, MEMORY_OFFLINE));

I think we have to do

ret = test_pages_isolated()
if (ret)
...
} while (ret);

So keeping the old code flow. I cannot resend before next Tuesday.

> +
> +    /* Mark all sections offline and remove free pages from the buddy. */
> +    __offline_isolated_pages(start_pfn, end_pfn);
> +    pr_info("Offlined Pages %ld\n", nr_pages);
> +
>    /*
>     * Onlining will reset pagetype flags and makes migrate type
>     * MOVABLE, so just need to decrease the number of isolated
> @@ -1609,11 +1598,11 @@ int __ref offline_pages(unsigned long st
>    spin_unlock_irqrestore(&zone->lock, flags);
> 
>    /* removal success */
> -    adjust_managed_page_count(pfn_to_page(start_pfn), -offlined_pages);
> -    zone->present_pages -= offlined_pages;
> +    adjust_managed_page_count(pfn_to_page(start_pfn), -nr_pages);
> +    zone->present_pages -= nr_pages;
> 
>    pgdat_resize_lock(zone->zone_pgdat, &flags);
> -    zone->zone_pgdat->node_present_pages -= offlined_pages;
> +    zone->zone_pgdat->node_present_pages -= nr_pages;
>    pgdat_resize_unlock(zone->zone_pgdat, &flags);
> 
>    init_per_zone_wmark_min();
> _
>
Michal Hocko Sept. 4, 2020, 7:46 a.m. UTC | #4
On Fri 04-09-20 07:47:45, David Hildenbrand wrote:
> 
> 
> > Am 03.09.2020 um 23:58 schrieb Andrew Morton <akpm@linux-foundation.org>:
[...]
> > @@ -1589,16 +1567,27 @@ int __ref offline_pages(unsigned long st
> >            reason = "failure to dissolve huge pages";
> >            goto failed_removal_isolated;
> >        }
> > -        /* check again */
> > -        ret = walk_system_ram_range(start_pfn, end_pfn - start_pfn,
> > -                        NULL, check_pages_isolated_cb);
> > -    } while (ret);
> > -
> > -    /* Ok, all of our target is isolated.
> > -       We cannot do rollback at this point. */
> > -    walk_system_ram_range(start_pfn, end_pfn - start_pfn,
> > -                  &offlined_pages, offline_isolated_pages_cb);
> > -    pr_info("Offlined Pages %ld\n", offlined_pages);
> > +
> > +        /*
> > +         * per-cpu pages are drained in start_isolate_page_range, but if
> > +         * there are still pages that are not free, make sure that we
> > +         * drain again, because when we isolated range we might
> > +         * have raced with another thread that was adding pages to pcp
> > +         * list.
> > +         *
> > +         * Forward progress should be still guaranteed because
> > +         * pages on the pcp list can only belong to MOVABLE_ZONE
> > +         * because has_unmovable_pages explicitly checks for
> > +         * PageBuddy on freed pages on other zones.
> > +         */
> > +        if (ret)
> > +            drain_all_pages(zone);
> > +    } while (test_pages_isolated(start_pfn, end_pfn, MEMORY_OFFLINE));
> 
> I think we have to do
> 
> ret = test_pages_isolated()
> if (ret)

Yes.
Andrew Morton Sept. 4, 2020, 7:21 p.m. UTC | #5
On Fri, 4 Sep 2020 07:47:45 +0200 David Hildenbrand <david@redhat.com> wrote:

> 
> 
> > +         * PageBuddy on freed pages on other zones.
> > +         */
> > +        if (ret)
> > +            drain_all_pages(zone);
> > +    } while (test_pages_isolated(start_pfn, end_pfn, MEMORY_OFFLINE));
> 
> I think we have to do
> 
> ret = test_pages_isolated()
> if (ret)
> ...
> } while (ret);
> 
> So keeping the old code flow. I cannot resend before next Tuesday.

Here's what I now (effectively) have:


From: David Hildenbrand <david@redhat.com>
Subject: mm/memory_hotplug: simplify page offlining

We make sure that we cannot have any memory holes right at the beginning
of offline_pages().  We no longer need walk_system_ram_range() and can
call test_pages_isolated() and __offline_isolated_pages() directly.

offlined_pages always corresponds to nr_pages, so we can simplify that.

Link: https://lkml.kernel.org/r/20200819175957.28465-4-david@redhat.com
Signed-off-by: David Hildenbrand <david@redhat.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Reviewed-by: Oscar Salvador <osalvador@suse.de>
Cc: Wei Yang <richard.weiyang@linux.alibaba.com>
Cc: Baoquan He <bhe@redhat.com>
Cc: Pankaj Gupta <pankaj.gupta.linux@gmail.com>
Cc: Charan Teja Reddy <charante@codeaurora.org>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Michel Lespinasse <walken@google.com>
Cc: Mike Rapoport <rppt@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>

--- a/mm/memory_hotplug.c~mm-memory_hotplug-simplify-page-offlining
+++ a/mm/memory_hotplug.c
@@ -1383,28 +1383,6 @@ do_migrate_range(unsigned long start_pfn
 	return ret;
 }
 
-/* Mark all sections offline and remove all free pages from the buddy. */
-static int
-offline_isolated_pages_cb(unsigned long start, unsigned long nr_pages,
-			void *data)
-{
-	unsigned long *offlined_pages = (unsigned long *)data;
-
-	*offlined_pages += __offline_isolated_pages(start, start + nr_pages);
-	return 0;
-}
-
-/*
- * Check all pages in range, recorded as memory resource, are isolated.
- */
-static int
-check_pages_isolated_cb(unsigned long start_pfn, unsigned long nr_pages,
-			void *data)
-{
-	return test_pages_isolated(start_pfn, start_pfn + nr_pages,
-				   MEMORY_OFFLINE);
-}
-
 static int __init cmdline_parse_movable_node(char *p)
 {
 	movable_node_enabled = true;
@@ -1491,7 +1469,7 @@ static int count_system_ram_pages_cb(uns
 int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages)
 {
 	const unsigned long end_pfn = start_pfn + nr_pages;
-	unsigned long pfn, system_ram_pages = 0, offlined_pages = 0;
+	unsigned long pfn, system_ram_pages = 0;
 	int ret, node, nr_isolate_pageblock;
 	unsigned long flags;
 	struct zone *zone;
@@ -1589,9 +1567,7 @@ int __ref offline_pages(unsigned long st
 			reason = "failure to dissolve huge pages";
 			goto failed_removal_isolated;
 		}
-		/* check again */
-		ret = walk_system_ram_range(start_pfn, end_pfn - start_pfn,
-					    NULL, check_pages_isolated_cb);
+
 		/*
 		 * per-cpu pages are drained in start_isolate_page_range, but if
 		 * there are still pages that are not free, make sure that we
@@ -1604,15 +1580,15 @@ int __ref offline_pages(unsigned long st
 		 * because has_unmovable_pages explicitly checks for
 		 * PageBuddy on freed pages on other zones.
 		 */
+		ret = test_pages_isolated(start_pfn, end_pfn, MEMORY_OFFLINE);
 		if (ret)
 			drain_all_pages(zone);
 	} while (ret);
 
-	/* Ok, all of our target is isolated.
-	   We cannot do rollback at this point. */
-	walk_system_ram_range(start_pfn, end_pfn - start_pfn,
-			      &offlined_pages, offline_isolated_pages_cb);
-	pr_info("Offlined Pages %ld\n", offlined_pages);
+	/* Mark all sections offline and remove free pages from the buddy. */
+	__offline_isolated_pages(start_pfn, end_pfn);
+	pr_info("Offlined Pages %ld\n", nr_pages);
+
 	/*
 	 * Onlining will reset pagetype flags and makes migrate type
 	 * MOVABLE, so just need to decrease the number of isolated
@@ -1623,11 +1599,11 @@ int __ref offline_pages(unsigned long st
 	spin_unlock_irqrestore(&zone->lock, flags);
 
 	/* removal success */
-	adjust_managed_page_count(pfn_to_page(start_pfn), -offlined_pages);
-	zone->present_pages -= offlined_pages;
+	adjust_managed_page_count(pfn_to_page(start_pfn), -nr_pages);
+	zone->present_pages -= nr_pages;
 
 	pgdat_resize_lock(zone->zone_pgdat, &flags);
-	zone->zone_pgdat->node_present_pages -= offlined_pages;
+	zone->zone_pgdat->node_present_pages -= nr_pages;
 	pgdat_resize_unlock(zone->zone_pgdat, &flags);
 
 	init_per_zone_wmark_min();
Michal Hocko Sept. 7, 2020, 6:45 a.m. UTC | #6
On Fri 04-09-20 12:21:34, Andrew Morton wrote:
> On Fri, 4 Sep 2020 07:47:45 +0200 David Hildenbrand <david@redhat.com> wrote:
[...]
> @@ -1589,9 +1567,7 @@ int __ref offline_pages(unsigned long st
>  			reason = "failure to dissolve huge pages";
>  			goto failed_removal_isolated;
>  		}
> -		/* check again */
> -		ret = walk_system_ram_range(start_pfn, end_pfn - start_pfn,
> -					    NULL, check_pages_isolated_cb);
> +
>  		/*
>  		 * per-cpu pages are drained in start_isolate_page_range, but if
>  		 * there are still pages that are not free, make sure that we
> @@ -1604,15 +1580,15 @@ int __ref offline_pages(unsigned long st
>  		 * because has_unmovable_pages explicitly checks for
>  		 * PageBuddy on freed pages on other zones.
>  		 */
> +		ret = test_pages_isolated(start_pfn, end_pfn, MEMORY_OFFLINE);
>  		if (ret)
>  			drain_all_pages(zone);
>  	} while (ret);

Looks ok
David Hildenbrand Sept. 8, 2020, 9:10 a.m. UTC | #7
On 07.09.20 08:45, Michal Hocko wrote:
> On Fri 04-09-20 12:21:34, Andrew Morton wrote:
>> On Fri, 4 Sep 2020 07:47:45 +0200 David Hildenbrand <david@redhat.com> wrote:
> [...]
>> @@ -1589,9 +1567,7 @@ int __ref offline_pages(unsigned long st
>>  			reason = "failure to dissolve huge pages";
>>  			goto failed_removal_isolated;
>>  		}
>> -		/* check again */
>> -		ret = walk_system_ram_range(start_pfn, end_pfn - start_pfn,
>> -					    NULL, check_pages_isolated_cb);
>> +
>>  		/*
>>  		 * per-cpu pages are drained in start_isolate_page_range, but if
>>  		 * there are still pages that are not free, make sure that we
>> @@ -1604,15 +1580,15 @@ int __ref offline_pages(unsigned long st
>>  		 * because has_unmovable_pages explicitly checks for
>>  		 * PageBuddy on freed pages on other zones.
>>  		 */
>> +		ret = test_pages_isolated(start_pfn, end_pfn, MEMORY_OFFLINE);
>>  		if (ret)
>>  			drain_all_pages(zone);
>>  	} while (ret);
> 
> Looks ok
> 

Agreed, thanks Michal and Andrew.

Patch
diff mbox series

diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 6856702af68d9..50aa5df696e9d 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1373,28 +1373,6 @@  do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
 	return ret;
 }
 
-/* Mark all sections offline and remove all free pages from the buddy. */
-static int
-offline_isolated_pages_cb(unsigned long start, unsigned long nr_pages,
-			void *data)
-{
-	unsigned long *offlined_pages = (unsigned long *)data;
-
-	*offlined_pages += __offline_isolated_pages(start, start + nr_pages);
-	return 0;
-}
-
-/*
- * Check all pages in range, recorded as memory resource, are isolated.
- */
-static int
-check_pages_isolated_cb(unsigned long start_pfn, unsigned long nr_pages,
-			void *data)
-{
-	return test_pages_isolated(start_pfn, start_pfn + nr_pages,
-				   MEMORY_OFFLINE);
-}
-
 static int __init cmdline_parse_movable_node(char *p)
 {
 	movable_node_enabled = true;
@@ -1481,7 +1459,7 @@  static int count_system_ram_pages_cb(unsigned long start_pfn,
 int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages)
 {
 	const unsigned long end_pfn = start_pfn + nr_pages;
-	unsigned long pfn, system_ram_pages = 0, offlined_pages = 0;
+	unsigned long pfn, system_ram_pages = 0;
 	int ret, node, nr_isolate_pageblock;
 	unsigned long flags;
 	struct zone *zone;
@@ -1579,16 +1557,12 @@  int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages)
 			reason = "failure to dissolve huge pages";
 			goto failed_removal_isolated;
 		}
-		/* check again */
-		ret = walk_system_ram_range(start_pfn, end_pfn - start_pfn,
-					    NULL, check_pages_isolated_cb);
-	} while (ret);
-
-	/* Ok, all of our target is isolated.
-	   We cannot do rollback at this point. */
-	walk_system_ram_range(start_pfn, end_pfn - start_pfn,
-			      &offlined_pages, offline_isolated_pages_cb);
-	pr_info("Offlined Pages %ld\n", offlined_pages);
+	} while (test_pages_isolated(start_pfn, end_pfn, MEMORY_OFFLINE));
+
+	/* Mark all sections offline and remove free pages from the buddy. */
+	__offline_isolated_pages(start_pfn, end_pfn);
+	pr_info("Offlined Pages %ld\n", nr_pages);
+
 	/*
 	 * Onlining will reset pagetype flags and makes migrate type
 	 * MOVABLE, so just need to decrease the number of isolated
@@ -1599,11 +1573,11 @@  int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages)
 	spin_unlock_irqrestore(&zone->lock, flags);
 
 	/* removal success */
-	adjust_managed_page_count(pfn_to_page(start_pfn), -offlined_pages);
-	zone->present_pages -= offlined_pages;
+	adjust_managed_page_count(pfn_to_page(start_pfn), -nr_pages);
+	zone->present_pages -= nr_pages;
 
 	pgdat_resize_lock(zone->zone_pgdat, &flags);
-	zone->zone_pgdat->node_present_pages -= offlined_pages;
+	zone->zone_pgdat->node_present_pages -= nr_pages;
 	pgdat_resize_unlock(zone->zone_pgdat, &flags);
 
 	init_per_zone_wmark_min();