| Message ID | 20250210212142.4002210-3-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | snapshot_page() | expand |
Hi Willy, On 2/11/2025 2:51 AM, Matthew Wilcox (Oracle) wrote: < snip > ... > +const struct folio *snapshot_page(struct folio *foliop, struct page *precise, > + unsigned long *idxp, const struct page *unstable) > +{ > + struct folio *folio; > + unsigned long head; > + unsigned long idx, nr_pages = 1; > + int loops = 5; > + > +again: > + memcpy(precise, unstable, sizeof(struct page)); > + head = precise->compound_head; > + /* Open-coded !PageTail because page_is_fake_head() doesn't work here */ > + if ((head & 1) == 0) { > + folio = (struct folio *)precise; > + *idxp = 0; > + /* Not a tail, not a head, we have a single page */ > + if (!folio_test_large(folio)) > + goto out; > + folio = (struct folio *)unstable; > + } else { > + folio = (struct folio *)(head - 1); > + *idxp = folio_page_idx(folio, unstable); > + } > + > + if (idx < MAX_FOLIO_NR_PAGES || folio_test_hugetlb(folio)) { idx is not initialized before use. I think you meant *idxp here. > + memcpy(foliop, folio, sizeof(struct folio)); > + nr_pages = folio_nr_pages(foliop); > + folio = foliop; > + } > + > + if (idx > nr_pages) { > + if (loops-- > 0) > + goto again; > + pr_warn("page does not match folio\n"); > + precise->compound_head &= ~1UL; > + folio = (struct folio *)precise; > + *idxp = 0; > + } > +out: > + return folio; > +} Please consider adding my Reviewed-by: Shivank Garg <shivankg@amd.com> with this fix: diff --git a/mm/util.c b/mm/util.c index 9f9cf3933eb1..155493b71d28 100644 --- a/mm/util.c +++ b/mm/util.c @@ -1253,7 +1253,7 @@ const struct folio *snapshot_page(struct folio *foliop, struct page *precise, { struct folio *folio; unsigned long head; - unsigned long idx, nr_pages = 1; + unsigned long nr_pages = 1; int loops = 5; again: @@ -1272,13 +1272,13 @@ const struct folio *snapshot_page(struct folio *foliop, struct page *precise, *idxp = folio_page_idx(folio, unstable); } - if (idx < MAX_FOLIO_NR_PAGES || folio_test_hugetlb(folio)) { + if (*idxp < MAX_FOLIO_NR_PAGES || folio_test_hugetlb(folio)) { memcpy(foliop, folio, sizeof(struct folio)); nr_pages = folio_nr_pages(foliop); folio = foliop; } - if (idx > nr_pages) { + if (*idxp > nr_pages) { if (loops-- > 0) goto again; pr_warn("page does not match folio\n");
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..
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) +{ + struct folio *folio; + unsigned long head; + unsigned long idx, nr_pages = 1; + int loops = 5; + +again: + memcpy(precise, unstable, sizeof(struct page)); + head = precise->compound_head; + /* Open-coded !PageTail because page_is_fake_head() doesn't work here */ + if ((head & 1) == 0) { + folio = (struct folio *)precise; + *idxp = 0; + /* Not a tail, not a head, we have a single page */ + if (!folio_test_large(folio)) + goto out; + folio = (struct folio *)unstable; + } else { + folio = (struct folio *)(head - 1); + *idxp = folio_page_idx(folio, unstable); + } + + if (idx < MAX_FOLIO_NR_PAGES || folio_test_hugetlb(folio)) { + memcpy(foliop, folio, sizeof(struct folio)); + nr_pages = folio_nr_pages(foliop); + folio = foliop; + } + + if (idx > nr_pages) { + if (loops-- > 0) + goto again; + pr_warn("page does not match folio\n"); + precise->compound_head &= ~1UL; + folio = (struct folio *)precise; + *idxp = 0; + } +out: + return folio; +}
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(-)