On 1/9/22 20:23, Matthew Wilcox (Oracle) wrote:
Move compound_pincount from the third page to the second page, which
means it's available for all compound pages. That lets us delete
hpage_pincount_available().
Wow, OK. That's a welcome simplification. Looks good. A couple comments
below, too.
...
@@ -955,7 +944,9 @@ static inline int compound_pincount(struct page *page)
static inline void set_compound_order(struct page *page, unsigned int order)
{
page[1].compound_order = order;
+#ifdef CONFIG_64BIT
page[1].compound_nr = 1U << order;
+#endif
}
/* Returns the number of pages in this potentially compound page. */
@@ -963,7 +954,11 @@ static inline unsigned long compound_nr(struct page *page)
{
if (!PageHead(page))
return 1;
+#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.
...
@@ -42,7 +41,7 @@ static void page_pincount_add(struct page *page, int refs)
{
VM_BUG_ON_PAGE(page != compound_head(page), page);
- if (hpage_pincount_available(page))
+ if (PageHead(page))
atomic_add(refs, compound_pincount_ptr(page));
else
page_ref_add(page, refs * (GUP_PIN_COUNTING_BIAS - 1));
@@ -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.
atomic_sub(refs, compound_pincount_ptr(page));
else
refs *= GUP_PIN_COUNTING_BIAS;
@@ -129,12 +128,11 @@ static inline struct page *try_get_compound_head(struct page *page, int refs)
*
* FOLL_GET: page's refcount will be incremented by @refs.
*
- * FOLL_PIN on compound pages that are > two pages long: page's refcount will
- * be incremented by @refs, and page[2].hpage_pinned_refcount will be
- * incremented by @refs * GUP_PIN_COUNTING_BIAS.
+ * FOLL_PIN on compound pages: page's refcount will be incremented by
+ * @refs, and page[1].compound_pincount will be incremented by @refs.
ha, thanks for fixing that documentation bug!
This all looks good, the above are very minor questions,
Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx>
thanks,
--
John Hubbard
NVIDIA
