Re: [PATCH] mm: Skip the reserved bootmem for compaction

[David Hildenbrand <david@redhat.com>]

On 02.09.24 14:24, Rong Qianfeng wrote:
> Reserved pages are basically non-lru pages. This kind of memory can't be
> used as migration sources and targets, skip it can bring some performance
> benefits.

Any numbers? :)

> 
> Because some drivers may also use PG_reserved, we just set PB_migrate_skip
> for those clustered reserved bootmem during memory initialization.
> 
> Signed-off-by: Rong Qianfeng <rongqianfeng@vivo.com>
> ---
>   include/linux/pageblock-flags.h | 13 +++++++++++
>   mm/compaction.c                 | 40 +++++++++++++++++++++++++++++++++
>   mm/mm_init.c                    | 14 ++++++++++++
>   mm/page_alloc.c                 |  7 ++++++
>   4 files changed, 74 insertions(+)
> 
> diff --git a/include/linux/pageblock-flags.h b/include/linux/pageblock-flags.h
> index fc6b9c87cb0a..63c5b0c69c1a 100644
> --- a/include/linux/pageblock-flags.h
> +++ b/include/linux/pageblock-flags.h
> @@ -86,6 +86,11 @@ void set_pfnblock_flags_mask(struct page *page,
>   	set_pfnblock_flags_mask(page, (1 << PB_migrate_skip),	\
>   			page_to_pfn(page),			\
>   			(1 << PB_migrate_skip))
> +
> +extern void set_pageblock_skip_range(unsigned long start_pfn,
> +				     unsigned long end_pfn);

two tabs indentation on the second line please. Applies to all others as 
well.

> +extern void clear_pageblock_skip_range(unsigned long start_pfn,
> +				       unsigned long end_pfn);
>   #else
>   static inline bool get_pageblock_skip(struct page *page)
>   {
> @@ -97,6 +102,14 @@ static inline void clear_pageblock_skip(struct page *page)
>   static inline void set_pageblock_skip(struct page *page)
>   {
>   }
> +static inline void set_pageblock_skip_range(unsigned long start_pfn,
> +					    unsigned long end_pfn)
> +{
> +}
> +static inline void clear_pageblock_skip_range(unsigned long start_pfn,
> +					      unsigned long end_pfn)
> +{
> +}

[...]

>   /*
>    * Compound pages of >= pageblock_order should consistently be skipped until
>    * released. It is always pointless to compact pages of such order (if they are
> diff --git a/mm/mm_init.c b/mm/mm_init.c
> index 4ba5607aaf19..8b7dc8e00bf1 100644
> --- a/mm/mm_init.c
> +++ b/mm/mm_init.c
> @@ -768,6 +768,13 @@ void __meminit reserve_bootmem_region(phys_addr_t start,
>   			__SetPageReserved(page);
>   		}
>   	}
> +
> +	/*
> +	 * Set PB_migrate_skip for reserved region. for cma memory
> +	 * and the memory released by free_reserved_area(), we will
> +	 * clear PB_migrate_skip when they are initialized.
> +	 */
> +	set_pageblock_skip_range(start_pfn, end_pfn);
>   }
>   
>   /* If zone is ZONE_MOVABLE but memory is mirrored, it is an overlapped init */
> @@ -2236,6 +2243,13 @@ void __init init_cma_reserved_pageblock(struct page *page)
>   		set_page_count(p, 0);
>   	} while (++p, --i);
>   
> +	/*
> +	 * We set the PB_migrate_skip in
> +	 * reserve_bootmem_region() for cma
> +	 * memory, clear it now.

You can fit this easily into less lines

> +	 */
> +	clear_pageblock_skip(page);
> +
>   	set_pageblock_migratetype(page, MIGRATE_CMA);
>   	set_page_refcounted(page);
>   	/* pages were reserved and not allocated */
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index b98f9bb28234..a7729dac0198 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5887,6 +5887,13 @@ unsigned long free_reserved_area(void *start, void *end, int poison, const char
>   	if (pages && s)
>   		pr_info("Freeing %s memory: %ldK\n", s, K(pages));
>   
> +	/*
> +	 * Clear PB_migrate_skip if the memory have released
> +	 * to the buddy system.
> +	 */

... after freeing the memory to the buddy."

And maybe

if (pages) {
	if (s)
		pr_info("Freeing %s memory: %ldK\n", s, K(pages));
	clear_pageblock_skip_range(...)
}

> +	clear_pageblock_skip_range(page_to_pfn(virt_to_page(start)),
> +				   page_to_pfn(virt_to_page(end)));
> +

PHYS_PFN(virt_to_phys(start)) might look a bit nicer, not need to
get pages involved. virt_to_pfn might be even better(), but it's
not available on all archs I think.


What about free_reserved_page() ? There might be more, though 
(kimage_free_pages()). You have to take a look at all functions where we 
clear PageReserved.

-- 
Cheers,

David / dhildenb