Re: [PATCH RFC] mm: Properly document tail pages for compound pages

[Matthew Wilcox]

On Thu, Aug 10, 2023 at 04:49:44PM -0400, Peter Xu wrote:
> Tail page struct reuse is over-comlicated.  Not only because we have
> implicit uses of tail page fields (mapcounts, or private for thp swap
> support, etc., that we _may_ still use in the page structs, but not obvious
> the relationship between that and the folio definitions), but also because
> we have 32/64 bits layouts for struct page so it's unclear what we can use
> and what we cannot when trying to find a new spot in folio struct.

I do not like this patch.

> We also have tricks like page->mapping, where we can reuse only the tail
> page 1/2 but nothing more than tail page 2.  It is all mostly hidden, until
> someone starts to read into a VM_BUG_ON_PAGE() of __split_huge_page_tail().

We can change those BUG_ON if we want to reuse mapping in more tail pages.
Ask!

> Let's document it clearly on what we can use and what we can't, with 100%
> explanations on each of them.  Hopefully this will make:

The explanations are still very page centric.  I do not like the style
of them, nor how you explain them.

> One pitfall is I'll need to split part of the tail page 1 definition into
> 32/64 bits differently, that introduced some duplications on the fields.
> But hopefully that's worthwhile as it makes everything crystal clear.  Not
> to mention that "pitfall" also brings a benefit that we can actually define
> fields in different order for 32/64 bits when we want.

No.  This is going to ruin kernel-doc.

> +	/*
> +	 * Some of the tail page fields (out of 8 WORDs for either 32/64

There's your first mistake; struct page is not necessarily 8 WORDs.
You've got 7 words for sure, then on 32-bit you have 8 because atomic_t
is word-sized.  memcg_data might be the 9th word, virtual could be
the tenth, two awful kmsan intrusions could bring it to twelve, and
last_cpupid could bring it to thirteen.

Sure, it's 8 words on x86-64 with CONFIG_MEMCG enabled.  But that's
just your system.

> +	 * bits archs) may not be reused by the folio object because
> +	 * they're already been used by the page struct:
> +	 *
> +	 * |-------+---------------|
> +	 * | Index | Field         |
> +	 * |-------+---------------|
> +	 * |     0 | flag          |
> +	 * |     1 | compound_head |
> +	 * |     2 | N/A [0]       |
> +	 * |     3 | mapping [1]   |
> +	 * |     4 | N/A [0]       |
> +	 * |     5 | private [2]   |
> +	 * |     6 | mapcount      |
> +	 * |     7 | N/A [0]       |

This is wrong.  You mustn't reuse refcount.  refcount must remain 0 on
all tail pages.  And you can't use anything after refcount, because it's
all optional on various configurations.

> +	 * |-------+---------------|
> +	 *
> +	 * [0] "N/A" marks fields that are available to leverage for the
> +	 *     large folio.

N/A is a bad way to say this.  "Free" or "Available" would be better.

> +	 * [1] "mapping" field is only used for sanity check, see
> +	 *     TAIL_MAPPING.  Still valid to use for tail pages 1/2.
> +	 *     (for that, see __split_huge_page_tail()).

No, definitely wrong to document this.

> +	 * [2] "private" field is used when THP_SWAP is on (disabled on 32
> +	 *     bits, or on hugetlb folios) .

Ugh, this needs to be fixed, not documented.  If you really must
document it, at least say that this needs to be fixed.

> +	 */
>  	union {
>  		struct {
> +	/* WORD 0-1: not valid to reuse */

... so now you're repeating all the information you provided above?

>  			unsigned long _flags_1;
>  			unsigned long _head_1;
> -	/* public: */

... did you check kernel-doc after removing this?

> +	/* WORD 2 */
>  			unsigned char _folio_dtor;
>  			unsigned char _folio_order;
> +			unsigned char _holes_1[2];

No.  If you need to search for holes, use pahole.

> +#ifdef CONFIG_64BIT
>  			atomic_t _entire_mapcount;
> +	/* WORD 3 */
>  			atomic_t _nr_pages_mapped;
>  			atomic_t _pincount;
> -#ifdef CONFIG_64BIT
> +	/* WORD 4 */
>  			unsigned int _folio_nr_pages;
> +			unsigned int _reserved_1_1;
> +	/* WORD 5-6: not valid to reuse */
> +			unsigned long _used_1_2[2];
> +	/* WORD 7 */
> +			unsigned long _reserved_1_2;
> +#else
> +	/* WORD 3 */
> +			atomic_t _entire_mapcount;
> +	/* WORD 4 */
> +			atomic_t _nr_pages_mapped;
> +	/* WORD 5: only valid for 32bits */
> +			atomic_t _pincount;
> +	/* WORD 6: not valid to reuse */
> +			unsigned long _used_1_2;
> +	/* WORD 7 */
> +			unsigned long _reserved_1;
>  #endif
> -	/* private: the union with struct page is transitional */
>  		};
> +	/* private: the union with struct page is transitional */

You don't understand why I did it like this.  Again, you have to build
the kernel-doc and you'll see why the private: and public: markers are
where they are.

There was even a thread on it, a year or two ago, where I outlined the
various tradeoffs between complexity of the output and the complexity
of the input.