Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 14 May 2021 12:47:01 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Vlastimil Babka <vbabka@suse.cz>
Cc:     akpm@linux-foundation.org, linux-fsdevel@vger.kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        Jeff Layton <jlayton@kernel.org>
Subject: Re: [PATCH v10 01/33] mm: Introduce struct folio
Message-ID: <YJ5jNR2HwgfXk7Wv@casper.infradead.org>
References: <20210511214735.1836149-1-willy@infradead.org>
 <20210511214735.1836149-2-willy@infradead.org>
 <ad8cd2e7-4111-f523-bc9c-5702b9071a5f@suse.cz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ad8cd2e7-4111-f523-bc9c-5702b9071a5f@suse.cz>
Precedence: bulk

On Fri, May 14, 2021 at 12:40:05PM +0200, Vlastimil Babka wrote:
> On 5/11/21 11:47 PM, Matthew Wilcox (Oracle) wrote:
> > +/**
> > + * folio_page - Return a page from a folio.
> > + * @folio: The folio.
> > + * @n: The page number to return.
> > + *
> > + * @n is relative to the start of the folio.  It should be between
> > + * 0 and folio_nr_pages(@folio) - 1, but this is not checked for.
> > + */
> > +#define folio_page(folio, n)	nth_page(&(folio)->page, n)
> 
> BTW, would it make sense to have also a folio_page(folio) wrapper? Or is
> "&folio->page" used in later patches sufficiently elegant and stable enough for
> the future?

Ah!  If you see &folio->page in a patch, it's "a bad smell" [1].  At
this stage, it probably indicates "This other thing I need isn't
converted entirely to folios yet".  I consider it fine in
implementations of utility functions like this:

+static inline unsigned int folio_order(struct folio *folio)
+{
+       return compound_order(&folio->page);
+}

but when we see it here:

+void folio_unlock(struct folio *folio)
 {
        BUILD_BUG_ON(PG_waiters != 7);
-       page = compound_head(page);
-       VM_BUG_ON_PAGE(!PageLocked(page), page);
-       if (clear_bit_unlock_is_negative_byte(PG_locked, &page->flags))
-               wake_up_page_bit(page, PG_locked);
+       VM_BUG_ON_FOLIO(!folio_locked(folio), folio);
+       if (clear_bit_unlock_is_negative_byte(PG_locked, folio_flags(folio, 0)))
+               wake_up_page_bit(&folio->page, PG_locked);
 }

that's an indication that wake_up_page_bit() needs to be converted to
folio_wake_bit(), which happens in a later patch.  I could probably
avoid this temporary problem with a different ordering of the patches,
but it's not clear to me that's a good use of my time.

The existing folio_page() is a way of distinguishing between "this
function i need to call doesn't have a folio equivalent yet" and "this
function i need to call needs to deal specifically with one page in
this folio".  For the former, use &folio->page; for the latter, use
folio_page() or folio_file_page().

[1] https://en.wikipedia.org/wiki/Code_smell