Re: [PATCH 1/4] mm/gup: add compound page list iterator

[Joao Martins]

On 2/4/21 11:27 AM, Joao Martins wrote:
> On 2/3/21 11:00 PM, John Hubbard wrote:
>> On 2/3/21 2:00 PM, Joao Martins wrote:
>>> Add an helper that iterates over head pages in a list of pages. It
>>> essentially counts the tails until the next page to process has a
>>> different head that the current. This is going to be used by
>>> unpin_user_pages() family of functions, to batch the head page refcount
>>> updates once for all passed consecutive tail pages.
>>>
>>> Suggested-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
>>> Signed-off-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
>>> ---
>>>   mm/gup.c | 29 +++++++++++++++++++++++++++++
>>>   1 file changed, 29 insertions(+)
>>>
>>> diff --git a/mm/gup.c b/mm/gup.c
>>> index d68bcb482b11..4f88dcef39f2 100644
>>> --- a/mm/gup.c
>>> +++ b/mm/gup.c
>>> @@ -215,6 +215,35 @@ void unpin_user_page(struct page *page)
>>>   }
>>>   EXPORT_SYMBOL(unpin_user_page);
>>>   
>>> +static inline unsigned int count_ntails(struct page **pages, unsigned long npages)
>>
>> Silly naming nit: could we please name this function count_pagetails()? count_ntails
>> is a bit redundant, plus slightly less clear.
>>
> Hmm, pagetails is also a tiny bit redundant. Perhaps count_subpages() instead?
> 
> count_ntails is meant to be 'count number of tails' i.e. to align terminology with head +
> tails which was also suggested over the other series.
> 
Given your comment on the third patch, I reworked a bit and got rid of the count_ntails.

So it's looking like this, also the macro arguments renaming as well):

diff --git a/mm/gup.c b/mm/gup.c
index d68bcb482b11..d1549c61c2f6 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -215,6 +215,35 @@ void unpin_user_page(struct page *page)
 }
 EXPORT_SYMBOL(unpin_user_page);

+static inline void compound_next(unsigned long i, unsigned long npages,
+                                struct page **list, struct page **head,
+                                unsigned int *ntails)
+{
+       struct page *page;
+       unsigned int nr;
+
+       if (i >= npages)
+               return;
+
+       list += i;
+       npages -= i;
+       page = compound_head(*list);
+
+       for (nr = 1; nr < npages; nr++) {
+               if (compound_head(list[nr]) != page)
+                       break;
+       }
+
+       *head = page;
+       *ntails = nr;
+}
+
+#define for_each_compound_head(__i, __list, __npages, __head, __ntails) \
+       for (__i = 0, \
+            compound_next(__i, __npages, __list, &(__head), &(__ntails)); \
+            __i < __npages; __i += __ntails, \
+            compound_next(__i, __npages, __list, &(__head), &(__ntails)))
+
 /**
  * unpin_user_pages_dirty_lock() - release and optionally dirty gup-pinned pages
  * @pages:  array of pages to be maybe marked dirty, and definitely released.