Message-ID: <CAHk-=whCgz0Kh3dGB-razGqgzM=spcO1fcyzD3vQd8PEO-bA0g@mail.gmail.com>
Date: Sat, 23 Nov 2024 12:57:49 -0800
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: John Hubbard <jhubbard@...dia.com>
Cc: David Hildenbrand <david@...hat.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	Matthew Wilcox <willy@...radead.org>, linux-mm@...ck.org, mm-commits@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [GIT PULL] MM updates for 6.13-rc1

On Sat, 23 Nov 2024 at 12:30, John Hubbard <jhubbard@...dia.com> wrote:
>
> >
> > ... not able to come up with good names though. folio_page0(), folio_first_page(), ... :(
>
> Eh? You're doing great at coming up with good names, IMHO. Either of the
> above would work nicely!
>
> I'll put my vote in for folio_page0(), it's concise and yet crystal clear.

I think all of this is completely missing the point.

The point is that "&folio->page" can be *compared* to a page pointer,
even when "folio" itself is not a valid pointer itself.

Changing

        if (&folio->page == page)

to

        if (folio_page0(folio) == page)

doesn't actually help anything at all. It still makes confused people
who do not understand pointer comparisons "oh, but if 'folio' is
invalid, I can't do 'folio_page0(folio)'".

See the problem, and see how you are not actually _fixing_ the confusion?

The way to hopefully *fix* the confusion is to have the actual
comparison itself inside the helper. Something like a

   static __always_inline bool folio_is_page(struct folio *folio,
struct page *page)
  { return &folio->page == page; }

and maybe even add a comment about how pointer comparisons are valid
even when the pointers are NULL or entirely invalid error pointers.

If you want to be extra fancy, you'd do something like

  union page_or_folio {
        struct page *page;
        struct folio *folio;
  };

  static inline bool folio_match(union page_or_folio a,
                                 union page_or_folio b)
  {
        return a.page == b.page;
  }

which doesn't care about argument ordering, and allows any combination
of folio or page pointers (but unlike a 'void *' argument, accepts
*only* folio or page pointers). So then you can do either
"folio_match(folio, page)" or "folio_match(page, folio)" and they are
the same thing.

Of course, the above does depend on "&folio->page" being effectively a
no-op (ie the page and folio pointers really are bitwise the same, ie
the ->page entry is at offset zero).

Which they are, and which all the FOLIO_MATCH things verify, but it
might be worth clarifying.

Honestly, I feel that we should use that "union page_or_folio" in more
places to avoid duplicating some of the helper functions, when a
function really doesn't care whether it gets a page or a folio. We
have a fair number of unnecessarily duplicated functions, I feel (eg
folio_mapping_flags() vs PageMappingFlags()), and I suspect some of
the "convert to folio" work could have been simplified by having code
that just happily accepts one or the other.

It probably matters less today, though, since a lot of the folio
conversion has been done.

And yes, you can also use _Generic() to automatically handle either
case, and that's particularly useful if you actually want to do
different things for a folio vs a page. For example, you could have a
"size" function that returns PAGE_SIZE for a 'struct page *', but
folio_size() for a 'struct folio *'.

Like the union argument trick, the _Generic() thing can be used to
make conversions easier, when you have functions that simply
JustWork(tm) and DoTheRightThing(tm) regardless of the type, and you
don't necessarily have to convert everything in one go.

              Linus

Powered by blists - more mailing lists