Re: [PATCH] mm: Wire up tail page poisoning over ->mappings

[Peter Xu]

On Mon, Aug 21, 2023 at 03:29:01AM +0100, Matthew Wilcox wrote:
> On Sun, Aug 20, 2023 at 09:13:55PM -0400, Peter Xu wrote:
> > > https://lore.kernel.org/linux-mm/ZNp7yUgUrIpILnXu@xxxxxxxxxxxxxxxxxxxx/
> > 
> > https://lore.kernel.org/linux-mm/ZNqFv0AwkfDKExiw@x1n/#t
> > 
> > Firstly, I've answered and you didn't follow that up.
> 
> I didn't see it.  I get a lot of email ...
> 
> > > > More importantly, I think this is over-parametrisation.  If you start to
> > > > use extra fields in struct folio, just change the code in page_alloc.c
> > > > directly.
> > 
> > Change the hard-coded "2"s in different functions?  Can you kindly explain
> > why can't we just have a macro to help?
> 
> Because it's unnecessary.  You're putting in way too much effort here
> for something that might happen once.
> 
> > Setting tail mapping for tail 1/2 is even wrong, which part of this patch
> > fixes:
> > 
> > @@ -428,7 +428,8 @@ static inline void prep_compound_tail(struct page *head, int tail_idx)
> >  {
> >         struct page *p = head + tail_idx;
> > 
> > -       p->mapping = TAIL_MAPPING;
> > +       if (tail_idx > TAIL_MAPPING_REUSED_MAX)
> > +               p->mapping = TAIL_MAPPING;
> >         set_compound_head(p, head);
> >         set_page_private(p, 0);
> >  }
> 
> I didn't see this.  This is wrong.  tail->mapping is only reused for
> large rmappable pages.  It's not reused for other compound pages.

Just to make sure we're on the same page: I think it's not only
_deferred_list (of tail page 2) that reused the mapping field (word offset
3), but also _nr_pages_mapped (of tail page 1)?

> 
> If you really insist on cleaning this up, the special casing of tail pages
> should move out of page_alloc entirely.  folio_undo_large_rmappable()
> should restore TAIL_MAPPING for all tail pages that were modified by
> folio_prep_large_rmappable().
> 
> The other thing we should do is verify that the additional large-rmap
> fields have the correct values in folio_undo_large_rmappable().
> 
> But let's look back to why TAIL_MAPPING was introduced.  Commit
> 1c290f642101e poisoned tail->mapping to catch users which forgot to
> call compound_head().  So we can actually remove TAIL_MAPPING entirely
> if we get rid of page->mapping.
> 
> You probably think that's an unattainable goal; there are something like
> 340 occurrences of the string 'page->mapping' in the kernel right now
> (and there are probably instances of struct page named something other
> than 'page'), but a lot of those are actually in comments, which would
> be my fault for not fixing them during folio conversions.
> 
> However, I have a small patch series which enables 'allnoconfig' to
> build after renaming page->mapping to page->_mapping.  Aside from fs/
> there are *very* few places which look at page->mapping [1].  I'll post
> that patch series tomorrow.

Assuming it's still not yet posted; I can wait and read it.

If you plan to remove the whole TAIL_MAPPING in a few days then I agree
this patch is not needed, but so far I don't know when it'll land and also
why, before that it does sound like we can still keep this patch.

Regarding the question on "why removing TAIL_MAPPING": poisoning an unused
field is always helpful to me even if not referenced with "page->mapping".
So I don't see an immediate benefit from removing the poisoning if it's
already there; OTOH not sure whether poison more unused fields will be more
helpful in general?

> 
> I think with some serious work, we can land "remove page->mapping"
> (which would include removing TAIL_MAPPING) by the end of the year.
> And that work gets us closer to the goal of shrinking struct page.
> 
> [1] device-dax, intel_th, mthca, cortina, fb_defio

Thanks,

-- 
Peter Xu