Messages in this thread | | | Date | Mon, 10 Jan 2022 21:10:43 -0800 | Subject | Re: [PATCH v2 11/28] mm: Make compound_pincount always available | From | John Hubbard <> |
| |
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
| |