On 1/10/22 20:38, Matthew Wilcox wrote:
On Mon, Jan 10, 2022 at 08:06:54PM -0800, John Hubbard wrote:
+#ifdef CONFIG_64BIT
return page[1].compound_nr;
+#else
+ return 1UL << compound_order(page);
+#endif
Now that you are highlighting this, I have this persistent feeling (not
yet confirmed by any testing) that compound_nr is a micro-optimization
that is actually invisible at runtime--but is now slicing up our code
with ifdefs, and using space in a fairly valuable location.
Not for this patch or series, but maybe a separate patch or series
should just remove the compound_nr field entirely, yes? It is
surprising to carry around both compound_order and (1 <<
compound_order), right next to each other. It would be different if this
were an expensive calculation, but it's just a shift.
Maybe testing would prove that that's a bad idea, and maybe someone has
already looked into it, but I wanted to point it out.
It' probably worth looking at the patch which added it ... 1378a5ee451a
in August 2020. I didn't provide any performance numbers, but code size
definitely went down.
I looked at that, and the lore link for the conversation, but failed to learn
anything additional. Of course if you recall that there was in fact a measurable
performance improvement, then as of now, it's recorded somewhere. :)
It's far from clear whether we'll need or want this space in page[1] in the
future anyway, just wanted to poke at it though.
@@ -52,7 +51,7 @@ static int page_pincount_sub(struct page *page, int refs)
{
VM_BUG_ON_PAGE(page != compound_head(page), page);
- if (hpage_pincount_available(page))
+ if (PageHead(page))
OK, so we just verified (via VM_BUG_ON_PAGE(), which is not always active)
that this is not a tail page. And so PageHead() effectively means PageCompound().
I wonder if it would be better to just use PageCompound() here and in similar
cases. Because that's what is logically being checked, after all. It seems
slightly more accurate.
Well PageCompound() is defined as PageHead() || PageTail(). I don't
think the intent was for people to always ask "Is this a compound page",
more "This is a good shorthand to replace PageHead() || PageTail()".
It's kind of moot anyway because this gets replaced with
folio_test_large() further down the patch series.
OK.
thanks,
--
John Hubbard
NVIDIA
