Re: [RFC PATCH 2/3] mm: Create snapshot_page()

[David Hildenbrand <david@redhat.com>]

On 10.02.25 22:21, Matthew Wilcox (Oracle) wrote:
> Move the guts of __dump_page() into a new function called
> snapshot_page().  With that done, __dump_page() becomes trivial so
> inline it into dump_page().
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>   mm/debug.c    | 53 +++++++++------------------------------------------
>   mm/internal.h |  2 ++
>   mm/util.c     | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 61 insertions(+), 44 deletions(-)
> 
> diff --git a/mm/debug.c b/mm/debug.c
> index fa3d9686034c..1ea5dacb1524 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -120,54 +120,19 @@ static void __dump_folio(const struct folio *folio, struct page *page,
>   			2 * sizeof(struct page), false);
>   }
>   
> -static void __dump_page(const struct page *page)
> -{
> -	struct folio *foliop, folio;
> -	struct page precise;
> -	unsigned long head;
> -	unsigned long pfn = page_to_pfn(page);
> -	unsigned long idx, nr_pages = 1;
> -	int loops = 5;
> -
> -again:
> -	memcpy(&precise, page, sizeof(*page));
> -	head = precise.compound_head;
> -	if ((head & 1) == 0) {
> -		foliop = (struct folio *)&precise;
> -		idx = 0;
> -		if (!folio_test_large(foliop))
> -			goto dump;
> -		foliop = (struct folio *)page;
> -	} else {
> -		foliop = (struct folio *)(head - 1);
> -		idx = folio_page_idx(foliop, page);
> -	}
> -
> -	if (idx < MAX_FOLIO_NR_PAGES) {
> -		memcpy(&folio, foliop, 2 * sizeof(struct page));
> -		nr_pages = folio_nr_pages(&folio);
> -		foliop = &folio;
> -	}
> -
> -	if (idx > nr_pages) {
> -		if (loops-- > 0)
> -			goto again;
> -		pr_warn("page does not match folio\n");
> -		precise.compound_head &= ~1UL;
> -		foliop = (struct folio *)&precise;
> -		idx = 0;
> -	}
> -
> -dump:
> -	__dump_folio(foliop, &precise, pfn, idx);
> -}
> -
>   void dump_page(const struct page *page, const char *reason)
>   {
> +	struct folio stack_folio;
> +	struct page stack_page;
> +	const struct folio *folio;
> +	unsigned long idx;
> +
>   	if (PagePoisoned(page))
>   		pr_warn("page:%p is uninitialized and poisoned", page);
> -	else
> -		__dump_page(page);
> +	else {
> +		folio = snapshot_page(&stack_folio, &stack_page, &idx, page);
> +		__dump_folio(folio, &stack_page, page_to_pfn(page), idx);
> +	}
>   	if (reason)
>   		pr_warn("page dumped because: %s\n", reason);
>   	dump_page_owner(page);
> diff --git a/mm/internal.h b/mm/internal.h
> index 109ef30fee11..8fdfea104068 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -856,6 +856,8 @@ static inline bool free_area_empty(struct free_area *area, int migratetype)
>   
>   /* mm/util.c */
>   struct anon_vma *folio_anon_vma(const struct folio *folio);
> +const struct folio *snapshot_page(struct folio *foliop, struct page *precise,
> +		unsigned long *idxp, const struct page *unstable);
>   
>   #ifdef CONFIG_MMU
>   void unmap_mapping_folio(struct folio *folio);
> diff --git a/mm/util.c b/mm/util.c
> index 682ecdb1b1c2..9f9cf3933eb1 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -1239,3 +1239,53 @@ void flush_dcache_folio(struct folio *folio)
>   }
>   EXPORT_SYMBOL(flush_dcache_folio);
>   #endif
> +
> +/*
> + * If you have an unstable reference to a page, use this to get a
> + * somewhat-consistent (potentially outdated) snapshot.  The consistency
> + * is limited to the page being contained in the folio.  You need to pass in
> + * a scratch folio and scratch page, probably allocated on the stack.
> + * You get back a pointer to the scratch folio you passed in, marked
> + * as const to remind you not to modify this.
> + */
> +const struct folio *snapshot_page(struct folio *foliop, struct page *precise,
> +		unsigned long *idxp, const struct page *unstable)
> +{


A better interface might be something like:

struct page_snapshot {
	struct folio folio_snapshot;
	struct page page_snapshot;
	/* page_to_pfn(page_snapshot) etc. don't work. */
	unsigned long pfn;
	/* folio_page_idx(folio_snapshot, ...) etc. don't work. */
	unsigned long idx;
};

void snapshot_page(struct page_snapshot *ps, struct page *page);

Or even (likely performance is less relevant)

struct page_snapshot snapshot_page(struct page *page);


__dump_folio() would then only accept that structure.


In the future, we could indicate what kind of memdesc the page was a 
part of, and have "struct folio folio_snapshot;" be in a union with 
other types we support to snapshot etc..

-- 
Cheers,

David / dhildenb