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
