Re: [PATCH v2 1/4] mm/gup: add compound page list iterator
On 2/5/21 2:46 AM, Joao Martins wrote:
...>> If instead you keep npages constant like it naturally wants to be, you
could
just do a "(*ntails)++" in the loop, to take care of *ntails.
I didn't do it as such as I would need to deref @ntails per iteration, so
it felt more efficient to do as above. On a second thought, I could
alternatively do the
following below, thoughts?
diff --git a/mm/gup.c b/mm/gup.c
index d68bcb482b11..8defe4f670d5 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -215,6 +215,32 @@ 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;
+
+ page = compound_head(list[i]);
+ for (nr = i + 1; nr < npages; nr++) {
+ if (compound_head(list[nr]) != page)
+ break;
+ }
+
+ *head = page;
+ *ntails = nr - i;
+}
+
Yes, this is cleaner and quite a bit easier to verify that it is correct.
However, given that the patch is correct and works as-is, the above is really
just
an optional idea, so please feel free to add:
Reviewed-by: John Hubbard
Thanks!
Hopefully I can retain that if the snippet above is preferred?
Joao
Yes. Still looks good.
thanks,
--
John Hubbard
NVIDIA
Re: [PATCH v2 1/4] mm/gup: add compound page list iterator
On 2/5/21 4:11 AM, John Hubbard wrote:
> On 2/4/21 12:24 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
>> Signed-off-by: Joao Martins
>> ---
>> mm/gup.c | 29 +
>> 1 file changed, 29 insertions(+)
>>
>> 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;
>
> It is worth noting that this is slightly more complex to read than it needs
> to be.
> You are changing both endpoints of a loop at once. That's hard to read for a
> human.
> And you're only doing it in order to gain the small benefit of being able to
> use nr directly at the end of the routine.
>
> If instead you keep npages constant like it naturally wants to be, you could
> just do a "(*ntails)++" in the loop, to take care of *ntails.
>
I didn't do it as such as I would need to deref @ntails per iteration, so
it felt more efficient to do as above. On a second thought, I could
alternatively do the
following below, thoughts?
diff --git a/mm/gup.c b/mm/gup.c
index d68bcb482b11..8defe4f670d5 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -215,6 +215,32 @@ 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;
+
+ page = compound_head(list[i]);
+ for (nr = i + 1; nr < npages; nr++) {
+ if (compound_head(list[nr]) != page)
+ break;
+ }
+
+ *head = page;
+ *ntails = nr - i;
+}
+
> However, given that the patch is correct and works as-is, the above is really
> just
> an optional idea, so please feel free to add:
>
> Reviewed-by: John Hubbard
>
>
Thanks!
Hopefully I can retain that if the snippet above is preferred?
Joao
Re: [PATCH v2 1/4] mm/gup: add compound page list iterator
On 2/4/21 12:24 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
Signed-off-by: Joao Martins
---
mm/gup.c | 29 +
1 file changed, 29 insertions(+)
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;
It is worth noting that this is slightly more complex to read than it needs to
be.
You are changing both endpoints of a loop at once. That's hard to read for a
human.
And you're only doing it in order to gain the small benefit of being able to
use nr directly at the end of the routine.
If instead you keep npages constant like it naturally wants to be, you could
just do a "(*ntails)++" in the loop, to take care of *ntails.
However, given that the patch is correct and works as-is, the above is really
just
an optional idea, so please feel free to add:
Reviewed-by: John Hubbard
thanks,
--
John Hubbard
NVIDIA
+ 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.
[PATCH v2 1/4] mm/gup: add compound page list iterator
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
Signed-off-by: Joao Martins
---
mm/gup.c | 29 +
1 file changed, 29 insertions(+)
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.
--
2.17.1
