Re: [PATCH RFC v2 0/3] mm: Properly document tail pages for a folio

[Matthew Wilcox]

On Tue, Aug 15, 2023 at 03:37:21PM -0400, Peter Xu wrote:
> On Tue, Aug 15, 2023 at 04:45:52AM +0100, Matthew Wilcox wrote:
> > I always forget about THP_SWAP using tail->private.  That actually needs
> > to be asserted by the compiler, not just documented.  Something along
> > these lines.
> > 
> > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > index 659c7b84726c..3880b3f2e321 100644
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -340,8 +340,11 @@ struct folio {
> >  			atomic_t _pincount;
> >  #ifdef CONFIG_64BIT
> >  			unsigned int _folio_nr_pages;
> > -#endif
> > +			/* 4 byte gap here */
> >  	/* private: the union with struct page is transitional */
> > +			/* Fix THP_SWAP to not use tail->private */
> > +			unsigned long _private_1;
> > +#endif
> >  		};
> >  		struct page __page_1;
> >  	};
> > @@ -362,6 +365,9 @@ struct folio {
> >  	/* public: */
> >  			struct list_head _deferred_list;
> >  	/* private: the union with struct page is transitional */
> > +			unsigned long _avail_2a;
> > +			/* Fix THP_SWAP to not use tail->private */
> > +			unsigned long _private_2a;
> >  		};
> >  		struct page __page_2;
> >  	};
> > @@ -386,12 +392,18 @@ FOLIO_MATCH(memcg_data, memcg_data);
> >  			offsetof(struct page, pg) + sizeof(struct page))
> >  FOLIO_MATCH(flags, _flags_1);
> >  FOLIO_MATCH(compound_head, _head_1);
> > +#ifdef CONFIG_64BIT
> > +FOLIO_MATCH(private, _private_1);
> > +#endif
> >  #undef FOLIO_MATCH
> >  #define FOLIO_MATCH(pg, fl)						\
> >  	static_assert(offsetof(struct folio, fl) ==			\
> >  			offsetof(struct page, pg) + 2 * sizeof(struct page))
> >  FOLIO_MATCH(flags, _flags_2);
> >  FOLIO_MATCH(compound_head, _head_2);
> > +FOLIO_MATCH(flags, _flags_2a);
> > +FOLIO_MATCH(compound_head, _head_2a);
> > +FOLIO_MATCH(private, _private_2a);
> >  #undef FOLIO_MATCH
> >  
> >  /*
> > 
> > This is against the patchset I just posted which frees up a word in the
> > first tail page.
> 
> Okay, I assume you meant to suggest leverage FOLIO_MATCH(), which I can
> definitely try.  But then I'd hope it covers not only private field but all
> the fields that the tail pages reuses; the goal is to document everything
> no matter in what form.  I'll see what I can get..  Thanks.

No, sometimes there are things which shouldn't be documented because they
don't matter, and when changing code sometimes we forget to change the
documentation, and then people read the documentation which is different
from the code, and they get confused.

It matters that the various 'private' members line up.  It matters
that folio->index matches page->index.  It does not matter what
offset _entire_mapcount is at.  That can be moved around freely and no
documentation needs to be changed.

I don't want you to use FOLIO_MATCH to make any unnecessary assertions.
The only assertion missing is for _private_1 and _private_2a, and that's
why I wrote a patch to add them.