lkml.org 
[lkml]   [2021]   [Feb]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [PATCH 3/4] mm/gup: add a range variant of unpin_user_pages_dirty_lock()
    From
    Date


    On 2/3/21 11:37 PM, John Hubbard wrote:
    > 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...
    >
    Yes.

    I was definitely wrestling back and forth a lot about having separate routines for two
    different iterators helpers i.e. compound_next_head()or having it all merged into one
    compound_next() / count_ntails().

    >> 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);
    >
    Yes.

    Let me also try instead to put move everything into two sole iterator helper routines,
    compound_next() and compound_next_range(), and thus get rid of this count_ntails(). It
    should also help in removing a compound_head() call which should save cycles.

    Joao

    \
     
     \ /
      Last update: 2021-02-04 12:39    [W:3.335 / U:0.108 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site