Messages in this thread |  | | Subject | Re: [PATCH 1/4] mm/gup: add compound page list iterator | From | John Hubbard <> | Date | Wed, 3 Feb 2021 15:00:01 -0800 |
| |
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, -- John Hubbard NVIDIA
|  |