On 17.03.22 16:37, Zi Yan wrote:
> From: Zi Yan <ziy@xxxxxxxxxx>
>
> Enable set_migratetype_isolate() to check specified sub-range for
> unmovable pages during isolation. Page isolation is done
> at max(MAX_ORDER_NR_PAEGS, pageblock_nr_pages) granularity, but not all
> pages within that granularity are intended to be isolated. For example,
> alloc_contig_range(), which uses page isolation, allows ranges without
> alignment. This commit makes unmovable page check only look for
> interesting pages, so that page isolation can succeed for any
> non-overlapping ranges.
>
> Signed-off-by: Zi Yan <ziy@xxxxxxxxxx>
> ---
> include/linux/page-isolation.h | 10 +++++
> mm/page_alloc.c | 13 +------
> mm/page_isolation.c | 69 ++++++++++++++++++++--------------
> 3 files changed, 51 insertions(+), 41 deletions(-)
>
> diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
> index e14eddf6741a..eb4a208fe907 100644
> --- a/include/linux/page-isolation.h
> +++ b/include/linux/page-isolation.h
> @@ -15,6 +15,16 @@ static inline bool is_migrate_isolate(int migratetype)
> {
> return migratetype == MIGRATE_ISOLATE;
> }
> +static inline unsigned long pfn_max_align_down(unsigned long pfn)
> +{
> + return ALIGN_DOWN(pfn, MAX_ORDER_NR_PAGES);
> +}
> +
> +static inline unsigned long pfn_max_align_up(unsigned long pfn)
> +{
> + return ALIGN(pfn, MAX_ORDER_NR_PAGES);
> +}
> +
> #else
> static inline bool has_isolate_pageblock(struct zone *zone)
> {
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6de57d058d3d..680580a40a35 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8937,16 +8937,6 @@ void *__init alloc_large_system_hash(const char *tablename,
> }
>
> #ifdef CONFIG_CONTIG_ALLOC
> -static unsigned long pfn_max_align_down(unsigned long pfn)
> -{
> - return ALIGN_DOWN(pfn, MAX_ORDER_NR_PAGES);
> -}
> -
> -static unsigned long pfn_max_align_up(unsigned long pfn)
> -{
> - return ALIGN(pfn, MAX_ORDER_NR_PAGES);
> -}
> -
> #if defined(CONFIG_DYNAMIC_DEBUG) || \
> (defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))
> /* Usage: See admin-guide/dynamic-debug-howto.rst */
> @@ -9091,8 +9081,7 @@ int alloc_contig_range(unsigned long start, unsigned long end,
> * put back to page allocator so that buddy can use them.
> */
>
> - ret = start_isolate_page_range(pfn_max_align_down(start),
> - pfn_max_align_up(end), migratetype, 0);
> + ret = start_isolate_page_range(start, end, migratetype, 0);
> if (ret)
> return ret;
Shouldn't we similarly adjust undo_isolate_page_range()? IOW, all users
of pfn_max_align_down()/pfn_max_align_up(). would be gone from that file
and you can move these defines into mm/page_isolation.c instead of
include/linux/page-isolation.h?
Maybe perform this change in a separate patch for
start_isolate_page_range() and undo_isolate_page_range() ?
>
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index b34f1310aeaa..419c805dbdcd 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -16,7 +16,8 @@
> #include <trace/events/page_isolation.h>
>
> /*
> - * This function checks whether pageblock includes unmovable pages or not.
> + * This function checks whether pageblock within [start_pfn, end_pfn) includes
> + * unmovable pages or not.
I think we still want to limit that to a single pageblock (see below),
as we're going to isolate individual pageblocks. Then an updated
description could be:
"This function checks whether the range [start_pfn, end_pfn) includes
unmovable pages or not. The range must fall into a single pageblock and
consequently belong to a single zone."
> *
> * PageLRU check without isolation or lru_lock could race so that
> * MIGRATE_MOVABLE block might include unmovable pages. And __PageMovable
> @@ -28,27 +29,26 @@
> * cannot get removed (e.g., via memory unplug) concurrently.
> *
> */
> -static struct page *has_unmovable_pages(struct zone *zone, struct page *page,
> - int migratetype, int flags)
> +static struct page *has_unmovable_pages(unsigned long start_pfn, unsigned long end_pfn,
> + int migratetype, int flags)
> {
> - unsigned long iter = 0;
> - unsigned long pfn = page_to_pfn(page);
> - unsigned long offset = pfn % pageblock_nr_pages;
> + unsigned long pfn = start_pfn;
>
> - if (is_migrate_cma_page(page)) {
> - /*
> - * CMA allocations (alloc_contig_range) really need to mark
> - * isolate CMA pageblocks even when they are not movable in fact
> - * so consider them movable here.
> - */
> - if (is_migrate_cma(migratetype))
> - return NULL;
If we're really dealing with a range that falls into a single pageblock,
then you can leave the is_migrate_cma_page() in place and also lookup
the zone only once. That should speed up things and minimize the
required changes.
You can then further add VM_BUG_ON()s that make sure that start_pfn and
end_pfn-1 belong to a single pageblock.
> + for (pfn = start_pfn; pfn < end_pfn; pfn++) {
> + struct page *page = pfn_to_page(pfn);
> + struct zone *zone = page_zone(page);
>
> - return page;
> - }
> + if (is_migrate_cma_page(page)) {
> + /*
> + * CMA allocations (alloc_contig_range) really need to mark
> + * isolate CMA pageblocks even when they are not movable in fact
> + * so consider them movable here.
> + */
> + if (is_migrate_cma(migratetype))
> + return NULL;
>
> - for (; iter < pageblock_nr_pages - offset; iter++) {
> - page = pfn_to_page(pfn + iter);
> + return page;
> + }
>
> /*
> * Both, bootmem allocations and memory holes are marked
> @@ -85,7 +85,7 @@ static struct page *has_unmovable_pages(struct zone *zone, struct page *page,
> }
>
> skip_pages = compound_nr(head) - (page - head);
> - iter += skip_pages - 1;
> + pfn += skip_pages - 1;
> continue;
> }
>
> @@ -97,7 +97,7 @@ static struct page *has_unmovable_pages(struct zone *zone, struct page *page,
> */
> if (!page_ref_count(page)) {
> if (PageBuddy(page))
> - iter += (1 << buddy_order(page)) - 1;
> + pfn += (1 << buddy_order(page)) - 1;
> continue;
> }
>
> @@ -134,7 +134,13 @@ static struct page *has_unmovable_pages(struct zone *zone, struct page *page,
> return NULL;
> }
>
> -static int set_migratetype_isolate(struct page *page, int migratetype, int isol_flags)
> +/*
> + * This function set pageblock migratetype to isolate if no unmovable page is
> + * present in [start_pfn, end_pfn). The pageblock must intersect with
> + * [start_pfn, end_pfn).
> + */
> +static int set_migratetype_isolate(struct page *page, int migratetype, int isol_flags,
> + unsigned long start_pfn, unsigned long end_pfn)
> {
> struct zone *zone = page_zone(page);
> struct page *unmovable;
> @@ -155,8 +161,13 @@ static int set_migratetype_isolate(struct page *page, int migratetype, int isol_
> /*
> * FIXME: Now, memory hotplug doesn't call shrink_slab() by itself.
> * We just check MOVABLE pages.
> + *
> + * Pass the intersection of [start_pfn, end_pfn) and the page's pageblock
> + * to avoid redundant checks.
> */
I think I'd prefer some helper variables for readability.
> - unmovable = has_unmovable_pages(zone, page, migratetype, isol_flags);
> + unmovable = has_unmovable_pages(max(page_to_pfn(page), start_pfn),
> + min(ALIGN(page_to_pfn(page) + 1, pageblock_nr_pages), end_pfn),
> + migratetype, isol_flags);
> if (!unmovable) {
> unsigned long nr_pages;
> int mt = get_pageblock_migratetype(page);
> @@ -267,7 +278,6 @@ __first_valid_page(unsigned long pfn, unsigned long nr_pages)
> * be MIGRATE_ISOLATE.
> * @start_pfn: The lower PFN of the range to be isolated.
> * @end_pfn: The upper PFN of the range to be isolated.
> - * start_pfn/end_pfn must be aligned to pageblock_order.
> * @migratetype: Migrate type to set in error recovery.
> * @flags: The following flags are allowed (they can be combined in
> * a bit mask)
> @@ -309,15 +319,16 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
> unsigned long pfn;
> struct page *page;
>
> - BUG_ON(!IS_ALIGNED(start_pfn, pageblock_nr_pages));
> - BUG_ON(!IS_ALIGNED(end_pfn, pageblock_nr_pages));
> + unsigned long isolate_start = pfn_max_align_down(start_pfn);
> + unsigned long isolate_end = pfn_max_align_up(end_pfn);
>
> - for (pfn = start_pfn;
> - pfn < end_pfn;
> + for (pfn = isolate_start;
> + pfn < isolate_end;
> pfn += pageblock_nr_pages) {
> page = __first_valid_page(pfn, pageblock_nr_pages);
> - if (page && set_migratetype_isolate(page, migratetype, flags)) {
> - undo_isolate_page_range(start_pfn, pfn, migratetype);
> + if (page && set_migratetype_isolate(page, migratetype, flags,
> + start_pfn, end_pfn)) {
> + undo_isolate_page_range(isolate_start, pfn, migratetype);
> return -EBUSY;
> }
> }
--
Thanks,
David / dhildenb
