| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5e372e25-7202-e0b6-0763-d267698db5b6@nvidia.com>
Date: Wed, 3 Feb 2021 15:37:26 -0800
From: John Hubbard <jhubbard@...dia.com>
To: Joao Martins <joao.m.martins@...cle.com>, <linux-mm@...ck.org>
CC: <linux-kernel@...r.kernel.org>, <linux-rdma@...r.kernel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Jason Gunthorpe <jgg@...pe.ca>,
Doug Ledford <dledford@...hat.com>,
Matthew Wilcox <willy@...radead.org>
Subject: Re: [PATCH 3/4] mm/gup: add a range variant of
unpin_user_pages_dirty_lock()
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@...dia.com>
> Signed-off-by: Joao Martins <joao.m.martins@...cle.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.
>
Powered by blists - more mailing lists