Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 10 Aug 2023 22:43:44 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Peter Xu <peterx@redhat.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Yu Zhao <yuzhao@google.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Yang Shi <shy828301@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Hugh Dickins <hughd@google.com>,
        David Hildenbrand <david@redhat.com>,
        Ryan Roberts <ryan.roberts@arm.com>
Subject: Re: [PATCH RFC] mm: Properly document tail pages for compound pages
Message-ID: <ZNVaEOmUUM5rR4CA@casper.infradead.org>
References: <20230810204944.53471-1-peterx@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230810204944.53471-1-peterx@redhat.com>
Precedence: bulk

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.