On 05.08.25 02:53, Alex Williamson wrote:
On Mon, 4 Aug 2025 16:55:09 -0700
Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
On Mon, 4 Aug 2025 at 15:22, Alex Williamson <alex.williamson@xxxxxxxxxx> wrote:
Li Zhe (6):
mm: introduce num_pages_contiguous()
WHY?
There is exactly *ONE* user, why the heck do we introduce this
completely pointless "helper" function, and put it in a core header
file like it was worth it?
There was discussion here[1] where David Hildenbrand and Jason
Gunthorpe suggested this should be in common code and I believe there
was some intent that this would get reused. I took this as
endorsement from mm folks. This can certainly be pulled back into
subsystem code.
Yeah, we ended up here after trying to go the folio-way first, but then
realizing that code that called GUP shouldn't have to worry about
folios, just to detect consecutive pages+PFNs.
I think this helper will can come in handy even in folio context.
I recall pointing Joanne at it in different fuse context.
And it's not like that code is some kind of work of art that we want
to expose everybody to *anyway*. It's written in a particularly stupid
way that means that it's *way* more expensive than it needs to be.
And then it's made "inline" despite the code generation being
horrible, which makes it all entirely pointless.
Yes, I'm grumpy. This pull request came in late, I'm already
traveling, and then I look at it and it just makes me *angry* at how
bad that code is, and how annoying it is.
Sorry, I usually try to get in later during the first week to let the
dust settle a bit from the bigger subsystems, I guess I'm running a
little behind this cycle. We'll get it fixed and I'll resend. Thanks,
Alex
My builds are already slower than usual because they happen on my
laptop while traveling, I do *not* need to see this kind of absolutely
disgusting code that does stupid things that make the build even
slower.
So I refuse to pull this kind of crap.
If you insist on making my build slower and exposing these kinds of
helper functions, they had better be *good* helper functions.
Hint: absolutely nobody cares about "the pages crossed a sparsemem
border. If your driver cares about the number of contiguous pages, it
might as well say "yeah, they are contiguous, but they are in
different sparsemem chunks, so we'll break here too".
The concern is rather false positives, meaning, you want consecutive
PFNs (just like within a folio), but -- because the stars aligned --
you get consecutive "struct page" that do not translate to consecutive PFNs.
So that's why the nth page stuff is not optional in the current
implementation as far as I can tell.
And because that nth_page stuff is so tricky and everyone gets it wrong
all the time, I am actually in favor of having such a helper around. not
buried in some subsystem.
And at that point all you care about is 'struct page' being
contiguous, instead of doing that disgusting 'nth_page'.
I think stopping when we hit the end of a memory section in case of
!CONFIG_SPARSEMEM_VMEMMAP could be done and documented.
It's just that ... code gets more complicated: we end up really only
optimizing for the unloved child sparsemem withoutCONFIG_SPARSEMEM_VMEMMAP:
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 0d4ee569aa6b6..f080fa5a68d4a 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1773,6 +1773,9 @@ static inline unsigned long page_to_section(const struct page *page)
* the memory model, this can mean that the addresses of the "struct page"s
* are not contiguous.
*
+ * On sparsemem configs without CONFIG_SPARSEMEM_VMEMMAP, we will stop once we
+ * hit a memory section boundary.
+ *
* @pages: an array of page pointers
* @nr_pages: length of the array
*/
@@ -1781,8 +1784,16 @@ static inline unsigned long num_pages_contiguous(struct page **pages,
{
size_t i;
+ if (IS_ENABLED(CONFIG_SPARSEMEM) &&
+ !IS_ENABLED(CONFIG_SPARSEMEM_VMEMMAP)) {
+ const unsigned long pfn = page_to_pfn(pages[0]);
+
+ nr_pages = min_t(size_t, nr_pages,
+ PAGES_PER_SECTION - (pfn & PAGES_PER_SECTION));
+ }
+
for (i = 1; i < nr_pages; i++)
- if (pages[i] != nth_page(pages[0], i))
+ if (pages[i] != pages[i - 1] + 1)
break;
Whether that helper should live in mm/utils.c is a valid point the bigger it gets.
And then - since there is only *one* single user - you don't put it in
the most central header file that EVERYBODY ELSE cares about.
And you absolutely don't do it if it generates garbage code for no good reason!
I understand the hate for nth_page in this context.
But I rather hate it *completely* because people get it wrong all of the time.
In any case, enjoy you travel Linus.
--
Cheers,
David / dhildenb
