Messages in this thread Patch in this message | | | Subject | Re: [PATCH 3/4] mm/gup: add a range variant of unpin_user_pages_dirty_lock() | From | John Hubbard <> | Date | Wed, 3 Feb 2021 15:37:26 -0800 |
| |
On 2/3/21 2:00 PM, Joao Martins wrote: > Add a unpin_user_page_range() API which takes a starting page > and how many consecutive pages we want to dirty. > > Given that we won't be iterating on a list of changes, change > compound_next() to receive a bool, whether to calculate from the starting > page, or walk the page array. Finally add a separate iterator,
A bool arg is sometimes, but not always, a hint that you really just want a separate set of routines. Below...
> for_each_compound_range() that just operate in page ranges as opposed > to page array. > > For users (like RDMA mr_dereg) where each sg represents a > contiguous set of pages, we're able to more efficiently unpin > pages without having to supply an array of pages much of what > happens today with unpin_user_pages(). > > Suggested-by: Jason Gunthorpe <jgg@nvidia.com> > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > include/linux/mm.h | 2 ++ > mm/gup.c | 48 ++++++++++++++++++++++++++++++++++++++-------- > 2 files changed, 42 insertions(+), 8 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index a608feb0d42e..b76063f7f18a 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1265,6 +1265,8 @@ static inline void put_page(struct page *page) > void unpin_user_page(struct page *page); > void unpin_user_pages_dirty_lock(struct page **pages, unsigned long npages, > bool make_dirty); > +void unpin_user_page_range_dirty_lock(struct page *page, unsigned long npages, > + bool make_dirty); > void unpin_user_pages(struct page **pages, unsigned long npages); > > /** > diff --git a/mm/gup.c b/mm/gup.c > index 971a24b4b73f..1b57355d5033 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -215,11 +215,16 @@ void unpin_user_page(struct page *page) > } > EXPORT_SYMBOL(unpin_user_page); > > -static inline unsigned int count_ntails(struct page **pages, unsigned long npages) > +static inline unsigned int count_ntails(struct page **pages, > + unsigned long npages, bool range) > { > - struct page *head = compound_head(pages[0]); > + struct page *page = pages[0], *head = compound_head(page); > unsigned int ntails; > > + if (range) > + return (!PageCompound(head) || compound_order(head) <= 1) ? 1 : > + min_t(unsigned int, (head + compound_nr(head) - page), npages);
Here, you clearly should use a separate set of _range routines. Because you're basically creating two different routines here! Keep it simple.
Once you're in a separate routine, you might feel more comfortable expanding that to a more readable form, too:
if (!PageCompound(head) || compound_order(head) <= 1) return 1;
return min_t(unsigned int, (head + compound_nr(head) - page), npages);
thanks, -- John Hubbard NVIDIA
> + > for (ntails = 1; ntails < npages; ntails++) { > if (compound_head(pages[ntails]) != head) > break; > @@ -229,20 +234,32 @@ static inline unsigned int count_ntails(struct page **pages, unsigned long npage > } > > static inline void compound_next(unsigned long i, unsigned long npages, > - struct page **list, struct page **head, > - unsigned int *ntails) > + struct page **list, bool range, > + struct page **head, unsigned int *ntails) > { > + struct page *p, **next = &p; > + > if (i >= npages) > return; > > - *ntails = count_ntails(list + i, npages - i); > - *head = compound_head(list[i]); > + if (range) > + *next = *list + i; > + else > + next = list + i; > + > + *ntails = count_ntails(next, npages - i, range); > + *head = compound_head(*next); > } > > +#define for_each_compound_range(i, list, npages, head, ntails) \ > + for (i = 0, compound_next(i, npages, list, true, &head, &ntails); \ > + i < npages; i += ntails, \ > + compound_next(i, npages, list, true, &head, &ntails)) > + > #define for_each_compound_head(i, list, npages, head, ntails) \ > - for (i = 0, compound_next(i, npages, list, &head, &ntails); \ > + for (i = 0, compound_next(i, npages, list, false, &head, &ntails); \ > i < npages; i += ntails, \ > - compound_next(i, npages, list, &head, &ntails)) > + compound_next(i, npages, list, false, &head, &ntails)) > > /** > * unpin_user_pages_dirty_lock() - release and optionally dirty gup-pinned pages > @@ -306,6 +323,21 @@ void unpin_user_pages_dirty_lock(struct page **pages, unsigned long npages, > } > EXPORT_SYMBOL(unpin_user_pages_dirty_lock); > > +void unpin_user_page_range_dirty_lock(struct page *page, unsigned long npages, > + bool make_dirty) > +{ > + unsigned long index; > + struct page *head; > + unsigned int ntails; > + > + for_each_compound_range(index, &page, npages, head, ntails) { > + if (make_dirty && !PageDirty(head)) > + set_page_dirty_lock(head); > + put_compound_head(head, ntails, FOLL_PIN); > + } > +} > +EXPORT_SYMBOL(unpin_user_page_range_dirty_lock); > + > /** > * unpin_user_pages() - release an array of gup-pinned pages. > * @pages: array of pages to be marked dirty and released. >
| |