linux-kernel - Re: [PATCH v10 01/33] mm: Introduce struct folio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YJ5jNR2HwgfXk7Wv@casper.infradead.org>
Date:   Fri, 14 May 2021 12:47:01 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Vlastimil Babka <vbabka@...e.cz>
Cc:     akpm@...ux-foundation.org, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Jeff Layton <jlayton@...nel.org>
Subject: Re: [PATCH v10 01/33] mm: Introduce struct folio

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