| Message ID | 20210203220025.8568-2-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/gup: page unpining improvements | expand |
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@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..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. > +{ > + struct page *head = compound_head(pages[0]); > + unsigned int ntails; > + > + for (ntails = 1; ntails < npages; ntails++) { > + if (compound_head(pages[ntails]) != head) > + break; > + } > + > + return ntails; > +} > + > +static inline void compound_next(unsigned long i, unsigned long npages, > + struct page **list, struct page **head, > + unsigned int *ntails) > +{ > + if (i >= npages) > + return; > + > + *ntails = count_ntails(list + i, npages - i); > + *head = compound_head(list[i]); > +} > + > +#define for_each_compound_head(i, list, npages, head, ntails) \ When using macros, which are dangerous in general, you have to worry about things like name collisions. I really dislike that C has forced this unsafe pattern upon us, but of course we are stuck with it, for iterator helpers. Given that we're stuck, you should probably use names such as __i, __list, etc, in the the above #define. Otherwise you could stomp on existing variables. > + 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. > thanks,
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@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..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. >> +{ >> + struct page *head = compound_head(pages[0]); >> + unsigned int ntails; >> + >> + for (ntails = 1; ntails < npages; ntails++) { >> + if (compound_head(pages[ntails]) != head) >> + break; >> + } >> + >> + return ntails; >> +} >> + >> +static inline void compound_next(unsigned long i, unsigned long npages, >> + struct page **list, struct page **head, >> + unsigned int *ntails) >> +{ >> + if (i >= npages) >> + return; >> + >> + *ntails = count_ntails(list + i, npages - i); >> + *head = compound_head(list[i]); >> +} >> + >> +#define for_each_compound_head(i, list, npages, head, ntails) \ > > When using macros, which are dangerous in general, you have to worry about > things like name collisions. I really dislike that C has forced this unsafe > pattern upon us, but of course we are stuck with it, for iterator helpers. > /me nods > Given that we're stuck, you should probably use names such as __i, __list, etc, > in the the above #define. Otherwise you could stomp on existing variables. Will do. Joao
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@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..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.
On Wed, Feb 03, 2021 at 03:00:01PM -0800, John Hubbard wrote: > > +static inline void compound_next(unsigned long i, unsigned long npages, > > + struct page **list, struct page **head, > > + unsigned int *ntails) > > +{ > > + if (i >= npages) > > + return; > > + > > + *ntails = count_ntails(list + i, npages - i); > > + *head = compound_head(list[i]); > > +} > > + > > +#define for_each_compound_head(i, list, npages, head, ntails) \ > > When using macros, which are dangerous in general, you have to worry about > things like name collisions. I really dislike that C has forced this unsafe > pattern upon us, but of course we are stuck with it, for iterator helpers. > > Given that we're stuck, you should probably use names such as __i, __list, etc, > in the the above #define. Otherwise you could stomp on existing variables. Not this macro, it after cpp gets through with it all the macro names vanish, it can't collide with variables. The usual worry is you might collide with other #defines, but we don't seem to worry about that in the kernel Jason
On 2/4/21 11:53 AM, Jason Gunthorpe wrote: > On Wed, Feb 03, 2021 at 03:00:01PM -0800, John Hubbard wrote: >>> +static inline void compound_next(unsigned long i, unsigned long npages, >>> + struct page **list, struct page **head, >>> + unsigned int *ntails) >>> +{ >>> + if (i >= npages) >>> + return; >>> + >>> + *ntails = count_ntails(list + i, npages - i); >>> + *head = compound_head(list[i]); >>> +} >>> + >>> +#define for_each_compound_head(i, list, npages, head, ntails) \ >> >> When using macros, which are dangerous in general, you have to worry about >> things like name collisions. I really dislike that C has forced this unsafe >> pattern upon us, but of course we are stuck with it, for iterator helpers. >> >> Given that we're stuck, you should probably use names such as __i, __list, etc, >> in the the above #define. Otherwise you could stomp on existing variables. > > Not this macro, it after cpp gets through with it all the macro names > vanish, it can't collide with variables. > Yes, I guess it does just vaporize, because it turns all the args into their substituted values. I was just having flashbacks from similar cases I guess. > The usual worry is you might collide with other #defines, but we don't > seem to worry about that in the kernel > Well, I worry about it a little anyway. haha :) thanks,
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) +{ + struct page *head = compound_head(pages[0]); + unsigned int ntails; + + for (ntails = 1; ntails < npages; ntails++) { + if (compound_head(pages[ntails]) != head) + break; + } + + return ntails; +} + +static inline void compound_next(unsigned long i, unsigned long npages, + struct page **list, struct page **head, + unsigned int *ntails) +{ + if (i >= npages) + return; + + *ntails = count_ntails(list + i, npages - i); + *head = compound_head(list[i]); +} + +#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(+)