| Message ID | 20231211162214.2146080-3-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Finish two folio conversions | expand |
On 11.12.23 17:22, Matthew Wilcox (Oracle) wrote: > We should only see anon folios in this function (and there are many > assumptions of that already), so we can simplify these two assertions. If we swapped in a fresh page, it is not PageAnon before we do the page_add_anon_rmap() call. So I'm pretty sure this is wrong. [I have plans of moving the "turn into anon folio" out of page_add_anon_rmap(), it will require teaching page_add_new_anon_rmap() about RMAP_EXCLUSIVE]
On Tue, Dec 12, 2023 at 01:26:35PM +0100, David Hildenbrand wrote: > On 11.12.23 17:22, Matthew Wilcox (Oracle) wrote: > > We should only see anon folios in this function (and there are many > > assumptions of that already), so we can simplify these two assertions. > > If we swapped in a fresh page, it is not PageAnon before we do the > page_add_anon_rmap() call. > > So I'm pretty sure this is wrong. Argh, yes. What do you think to just dropping the assertions altogether? You added them in 78fbe906cc90 as part of general paranoia about using an existing flag for a new purpose. I think they've now served their purpose and can go away. Perhaps simply: VM_BUG_ON_PAGE(PageAnonExclusive(page), page);
On 12.12.23 14:52, Matthew Wilcox wrote: > On Tue, Dec 12, 2023 at 01:26:35PM +0100, David Hildenbrand wrote: >> On 11.12.23 17:22, Matthew Wilcox (Oracle) wrote: >>> We should only see anon folios in this function (and there are many >>> assumptions of that already), so we can simplify these two assertions. >> >> If we swapped in a fresh page, it is not PageAnon before we do the >> page_add_anon_rmap() call. >> >> So I'm pretty sure this is wrong. > > Argh, yes. > > What do you think to just dropping the assertions altogether? You > added them in 78fbe906cc90 as part of general paranoia about using > an existing flag for a new purpose. I think they've now served their > purpose and can go away. At least the ones here, yes. I mean, unuse_pte() is a very corner case feature either way. > > Perhaps simply: > > VM_BUG_ON_PAGE(PageAnonExclusive(page), page); If we refault a page form the swapcache, it would already be PageAnon. Maybe just drop the assertions completely in unuse_pte().
diff --git a/mm/swapfile.c b/mm/swapfile.c index 0371b7b3cd27..88842c6fb8fe 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -1789,8 +1789,8 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd, arch_swap_restore(entry, page_folio(page)); /* See do_swap_page() */ - BUG_ON(!PageAnon(page) && PageMappedToDisk(page)); - BUG_ON(PageAnon(page) && PageAnonExclusive(page)); + VM_BUG_ON_FOLIO(!folio_test_anon(folio), folio); + VM_BUG_ON_PAGE(PageAnonExclusive(page), page); dec_mm_counter(vma->vm_mm, MM_SWAPENTS); inc_mm_counter(vma->vm_mm, MM_ANONPAGES);
We should only see anon folios in this function (and there are many assumptions of that already), so we can simplify these two assertions. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- mm/swapfile.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)