Re: [PATCH v3 2/4] mm/util: introduce snapshot_page()

[David Hildenbrand]

[...]

>   
> -dump:
> -	__dump_folio(foliop, &precise, pfn, idx);
> +	__dump_folio(&ps.folio_snapshot, &ps.page_snapshot, ps.pfn, ps.idx);

Nit that can be cleaned up later on top:

We should probably call this

__dump_page_snapshot() and then just pass ... the page_snapshot.

>   }
>   
>   void dump_page(const struct page *page, const char *reason)
> diff --git a/mm/util.c b/mm/util.c
> index 0b270c43d7d1..f270bf42465b 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -25,6 +25,7 @@
>   #include <linux/sizes.h>
>   #include <linux/compat.h>
>   #include <linux/fsnotify.h>
> +#include <linux/page_idle.h>
>   
>   #include <linux/uaccess.h>
>   
> @@ -1171,3 +1172,81 @@ int compat_vma_mmap_prepare(struct file *file, struct vm_area_struct *vma)
>   	return 0;
>   }
>   EXPORT_SYMBOL(compat_vma_mmap_prepare);
> +
> +static void set_ps_flags(struct page_snapshot *ps, const struct folio *folio,
> +			 const struct page *page)
> +{
> +	/*
> +	 * Only the first page of a high-order buddy page has PageBuddy() set.
> +	 * So we have to check manually whether this page is part of a high-
> +	 * order buddy page.
> +	 */
> +	if (PageBuddy(page))
> +		ps->flags |= PAGE_SNAPSHOT_PG_BUDDY;
> +	else if (page_count(page) == 0 && is_free_buddy_page(page))
> +		ps->flags |= PAGE_SNAPSHOT_PG_BUDDY;
> +
> +	if (folio_test_idle(folio))
> +		ps->flags |= PAGE_SNAPSHOT_PG_IDLE;
> +}
> +
> +/**
> + * snapshot_page() - Create a snapshot of a struct page
> + * @ps: Pointer to a struct page_snapshot to store the page snapshot
> + * @page: The page to snapshot
> + *
> + * Create a snapshot of the page and store both its struct page and struct
> + * folio representations in @ps.
> + *
> + * Note that creating a faithful snapshot may fail if the compound

Maybe highlight that this is not really expected to happen, ever.

> + * state of the page keeps changing (e.g., due to a folio split). In
> + * this case, ps->faithful is set to false, and the snapshot assumes

There is no ps->faithful.

> + * that @page refers to a single page.
> + */
> +void snapshot_page(struct page_snapshot *ps, const struct page *page)
> +{
> +	unsigned long head, nr_pages = 1;
> +	struct folio *foliop;
> +	int loops = 5;
> +
> +	ps->pfn = page_to_pfn(page);
> +	ps->flags = PAGE_SNAPSHOT_FAITHFUL;
> +
> +again:
> +	memset(&ps->folio_snapshot, 0, sizeof(struct folio));
> +	memcpy(&ps->page_snapshot, page, sizeof(*page));
> +	head = ps->page_snapshot.compound_head;
> +	if ((head & 1) == 0) {
> +		ps->idx = 0;
> +		foliop = (struct folio *)&ps->page_snapshot;
> +		if (!folio_test_large(foliop)) {
> +			set_ps_flags(ps, page_folio(page), page);
> +			memcpy(&ps->folio_snapshot, foliop,
> +			       sizeof(struct page));
> +			return;
> +		}
> +		foliop = (struct folio *)page;
> +	} else {
> +		foliop = (struct folio *)(head - 1);
> +		ps->idx = folio_page_idx(foliop, page);
> +	}

Condition could be cleaned up by reversing both things

if (head & 1) {
	/* Tail page, lookup the actual head. */
	foliop = (struct folio *)(head - 1);
	ps->idx = folio_page_idx(foliop, page);
} else
	...
}

But you're just moving that code, so no need to do that now.


I think we could improve some of that in the future a bit to
make it even more faithful.

But for now this should be just fine.

Acked-by: David Hildenbrand <david@xxxxxxxxxx>

--
Cheers,

David / dhildenb