| Message ID | 20210204202500.26474-2-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/gup: page unpining improvements | expand |
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 <jgg@nvidia.com> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > 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 <jhubbard@nvidia.com> thanks,
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 <jgg@nvidia.com> >> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> >> --- >> 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 <jhubbard@nvidia.com> > > Thanks! Hopefully I can retain that if the snippet above is preferred? Joao
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 <jhubbard@nvidia.com> >> >> > Thanks! > > Hopefully I can retain that if the snippet above is preferred? > > Joao > Yes. Still looks good. thanks,
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.
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@nvidia.com> Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- mm/gup.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)