From: Ira Weiny <ira.weiny@intel.com>
To: john.hubbard@gmail.com
Cc: "Andrew Morton" <akpm@linux-foundation.org>,
"Christoph Hellwig" <hch@infradead.org>,
"Dan Williams" <dan.j.williams@intel.com>,
"Dave Chinner" <david@fromorbit.com>,
"Dave Hansen" <dave.hansen@linux.intel.com>,
"Jan Kara" <jack@suse.cz>, "Jason Gunthorpe" <jgg@ziepe.ca>,
"Jérôme Glisse" <jglisse@redhat.com>,
LKML <linux-kernel@vger.kernel.org>,
amd-gfx@lists.freedesktop.org, ceph-devel@vger.kernel.org,
devel@driverdev.osuosl.org, devel@lists.orangefs.org,
dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org,
kvm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-block@vger.kernel.org, linux-crypto@vger.kernel.org,
linux-fbdev@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-media@vger.kernel.org, linux-mm@kvack.org,
linux-nfs@vger.kernel.org, linux-rdma@vger.kernel.org,
linux-rpi-kernel@lists.infradead.org, linux-xfs@vger.kernel.org,
netdev@vger.kernel.org, rds-devel@oss.oracle.com,
sparclinux@vger.kernel.org, x86@kernel.org,
xen-devel@lists.xenproject.org,
"John Hubbard" <jhubbard@nvidia.com>,
"Christoph Hellwig" <hch@lst.de>,
"Matthew Wilcox" <willy@infradead.org>
Subject: Re: [PATCH v2 01/34] mm/gup: add make_dirty arg to put_user_pages_dirty_lock()
Date: Tue, 6 Aug 2019 10:39:46 -0700 [thread overview]
Message-ID: <20190806173945.GA4748@iweiny-DESK2.sc.intel.com> (raw)
In-Reply-To: <20190804224915.28669-2-jhubbard@nvidia.com>
On Sun, Aug 04, 2019 at 03:48:42PM -0700, john.hubbard@gmail.com wrote:
> From: John Hubbard <jhubbard@nvidia.com>
>
> Provide a more capable variation of put_user_pages_dirty_lock(),
> and delete put_user_pages_dirty(). This is based on the
> following:
>
> 1. Lots of call sites become simpler if a bool is passed
> into put_user_page*(), instead of making the call site
> choose which put_user_page*() variant to call.
>
> 2. Christoph Hellwig's observation that set_page_dirty_lock()
> is usually correct, and set_page_dirty() is usually a
> bug, or at least questionable, within a put_user_page*()
> calling chain.
>
> This leads to the following API choices:
>
> * put_user_pages_dirty_lock(page, npages, make_dirty)
>
> * There is no put_user_pages_dirty(). You have to
> hand code that, in the rare case that it's
> required.
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Jan Kara <jack@suse.cz>
> Cc: Ira Weiny <ira.weiny@intel.com>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
> drivers/infiniband/core/umem.c | 5 +-
> drivers/infiniband/hw/hfi1/user_pages.c | 5 +-
> drivers/infiniband/hw/qib/qib_user_pages.c | 13 +--
> drivers/infiniband/hw/usnic/usnic_uiom.c | 5 +-
> drivers/infiniband/sw/siw/siw_mem.c | 19 +---
> include/linux/mm.h | 5 +-
> mm/gup.c | 115 +++++++++------------
> 7 files changed, 61 insertions(+), 106 deletions(-)
>
> diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
> index 08da840ed7ee..965cf9dea71a 100644
> --- a/drivers/infiniband/core/umem.c
> +++ b/drivers/infiniband/core/umem.c
> @@ -54,10 +54,7 @@ static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int d
>
> for_each_sg_page(umem->sg_head.sgl, &sg_iter, umem->sg_nents, 0) {
> page = sg_page_iter_page(&sg_iter);
> - if (umem->writable && dirty)
> - put_user_pages_dirty_lock(&page, 1);
> - else
> - put_user_page(page);
> + put_user_pages_dirty_lock(&page, 1, umem->writable && dirty);
> }
>
> sg_free_table(&umem->sg_head);
> diff --git a/drivers/infiniband/hw/hfi1/user_pages.c b/drivers/infiniband/hw/hfi1/user_pages.c
> index b89a9b9aef7a..469acb961fbd 100644
> --- a/drivers/infiniband/hw/hfi1/user_pages.c
> +++ b/drivers/infiniband/hw/hfi1/user_pages.c
> @@ -118,10 +118,7 @@ int hfi1_acquire_user_pages(struct mm_struct *mm, unsigned long vaddr, size_t np
> void hfi1_release_user_pages(struct mm_struct *mm, struct page **p,
> size_t npages, bool dirty)
> {
> - if (dirty)
> - put_user_pages_dirty_lock(p, npages);
> - else
> - put_user_pages(p, npages);
> + put_user_pages_dirty_lock(p, npages, dirty);
>
> if (mm) { /* during close after signal, mm can be NULL */
> atomic64_sub(npages, &mm->pinned_vm);
> diff --git a/drivers/infiniband/hw/qib/qib_user_pages.c b/drivers/infiniband/hw/qib/qib_user_pages.c
> index bfbfbb7e0ff4..26c1fb8d45cc 100644
> --- a/drivers/infiniband/hw/qib/qib_user_pages.c
> +++ b/drivers/infiniband/hw/qib/qib_user_pages.c
> @@ -37,15 +37,6 @@
>
> #include "qib.h"
>
> -static void __qib_release_user_pages(struct page **p, size_t num_pages,
> - int dirty)
> -{
> - if (dirty)
> - put_user_pages_dirty_lock(p, num_pages);
> - else
> - put_user_pages(p, num_pages);
> -}
> -
> /**
> * qib_map_page - a safety wrapper around pci_map_page()
> *
> @@ -124,7 +115,7 @@ int qib_get_user_pages(unsigned long start_page, size_t num_pages,
>
> return 0;
> bail_release:
> - __qib_release_user_pages(p, got, 0);
> + put_user_pages_dirty_lock(p, got, false);
> bail:
> atomic64_sub(num_pages, ¤t->mm->pinned_vm);
> return ret;
> @@ -132,7 +123,7 @@ int qib_get_user_pages(unsigned long start_page, size_t num_pages,
>
> void qib_release_user_pages(struct page **p, size_t num_pages)
> {
> - __qib_release_user_pages(p, num_pages, 1);
> + put_user_pages_dirty_lock(p, num_pages, true);
>
> /* during close after signal, mm can be NULL */
> if (current->mm)
> diff --git a/drivers/infiniband/hw/usnic/usnic_uiom.c b/drivers/infiniband/hw/usnic/usnic_uiom.c
> index 0b0237d41613..62e6ffa9ad78 100644
> --- a/drivers/infiniband/hw/usnic/usnic_uiom.c
> +++ b/drivers/infiniband/hw/usnic/usnic_uiom.c
> @@ -75,10 +75,7 @@ static void usnic_uiom_put_pages(struct list_head *chunk_list, int dirty)
> for_each_sg(chunk->page_list, sg, chunk->nents, i) {
> page = sg_page(sg);
> pa = sg_phys(sg);
> - if (dirty)
> - put_user_pages_dirty_lock(&page, 1);
> - else
> - put_user_page(page);
> + put_user_pages_dirty_lock(&page, 1, dirty);
> usnic_dbg("pa: %pa\n", &pa);
> }
> kfree(chunk);
> diff --git a/drivers/infiniband/sw/siw/siw_mem.c b/drivers/infiniband/sw/siw/siw_mem.c
> index 67171c82b0c4..1e197753bf2f 100644
> --- a/drivers/infiniband/sw/siw/siw_mem.c
> +++ b/drivers/infiniband/sw/siw/siw_mem.c
> @@ -60,20 +60,6 @@ struct siw_mem *siw_mem_id2obj(struct siw_device *sdev, int stag_index)
> return NULL;
> }
>
> -static void siw_free_plist(struct siw_page_chunk *chunk, int num_pages,
> - bool dirty)
> -{
> - struct page **p = chunk->plist;
> -
> - while (num_pages--) {
> - if (!PageDirty(*p) && dirty)
> - put_user_pages_dirty_lock(p, 1);
> - else
> - put_user_page(*p);
> - p++;
> - }
> -}
> -
> void siw_umem_release(struct siw_umem *umem, bool dirty)
> {
> struct mm_struct *mm_s = umem->owning_mm;
> @@ -82,8 +68,9 @@ void siw_umem_release(struct siw_umem *umem, bool dirty)
> for (i = 0; num_pages; i++) {
> int to_free = min_t(int, PAGES_PER_CHUNK, num_pages);
>
> - siw_free_plist(&umem->page_chunk[i], to_free,
> - umem->writable && dirty);
> + put_user_pages_dirty_lock(umem->page_chunk[i].plist,
> + to_free,
> + umem->writable && dirty);
> kfree(umem->page_chunk[i].plist);
> num_pages -= to_free;
> }
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 0334ca97c584..9759b6a24420 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1057,8 +1057,9 @@ static inline void put_user_page(struct page *page)
> put_page(page);
> }
>
> -void put_user_pages_dirty(struct page **pages, unsigned long npages);
> -void put_user_pages_dirty_lock(struct page **pages, unsigned long npages);
> +void put_user_pages_dirty_lock(struct page **pages, unsigned long npages,
> + bool make_dirty);
> +
> void put_user_pages(struct page **pages, unsigned long npages);
>
> #if defined(CONFIG_SPARSEMEM) && !defined(CONFIG_SPARSEMEM_VMEMMAP)
> diff --git a/mm/gup.c b/mm/gup.c
> index 98f13ab37bac..7fefd7ab02c4 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -29,85 +29,70 @@ struct follow_page_context {
> unsigned int page_mask;
> };
>
> -typedef int (*set_dirty_func_t)(struct page *page);
> -
> -static void __put_user_pages_dirty(struct page **pages,
> - unsigned long npages,
> - set_dirty_func_t sdf)
> -{
> - unsigned long index;
> -
> - for (index = 0; index < npages; index++) {
> - struct page *page = compound_head(pages[index]);
> -
> - /*
> - * Checking PageDirty at this point may race with
> - * clear_page_dirty_for_io(), but that's OK. Two key cases:
> - *
> - * 1) This code sees the page as already dirty, so it skips
> - * the call to sdf(). That could happen because
> - * clear_page_dirty_for_io() called page_mkclean(),
> - * followed by set_page_dirty(). However, now the page is
> - * going to get written back, which meets the original
> - * intention of setting it dirty, so all is well:
> - * clear_page_dirty_for_io() goes on to call
> - * TestClearPageDirty(), and write the page back.
> - *
> - * 2) This code sees the page as clean, so it calls sdf().
> - * The page stays dirty, despite being written back, so it
> - * gets written back again in the next writeback cycle.
> - * This is harmless.
> - */
> - if (!PageDirty(page))
> - sdf(page);
> -
> - put_user_page(page);
> - }
> -}
> -
> /**
> - * put_user_pages_dirty() - release and dirty an array of gup-pinned pages
> - * @pages: array of pages to be marked dirty and released.
> + * put_user_pages_dirty_lock() - release and optionally dirty gup-pinned pages
> + * @pages: array of pages to be maybe marked dirty, and definitely released.
Better would be.
@pages: array of pages to be put
> * @npages: number of pages in the @pages array.
> + * @make_dirty: whether to mark the pages dirty
> *
> * "gup-pinned page" refers to a page that has had one of the get_user_pages()
> * variants called on that page.
> *
> * For each page in the @pages array, make that page (or its head page, if a
> - * compound page) dirty, if it was previously listed as clean. Then, release
> - * the page using put_user_page().
> + * compound page) dirty, if @make_dirty is true, and if the page was previously
> + * listed as clean. In any case, releases all pages using put_user_page(),
> + * possibly via put_user_pages(), for the non-dirty case.
I don't think users of this interface need this level of detail. I think
something like.
* For each page in the @pages array, release the page. If @make_dirty is
* true, mark the page dirty prior to release.
> *
> * Please see the put_user_page() documentation for details.
> *
> - * set_page_dirty(), which does not lock the page, is used here.
> - * Therefore, it is the caller's responsibility to ensure that this is
> - * safe. If not, then put_user_pages_dirty_lock() should be called instead.
> + * set_page_dirty_lock() is used internally. If instead, set_page_dirty() is
> + * required, then the caller should a) verify that this is really correct,
> + * because _lock() is usually required, and b) hand code it:
> + * set_page_dirty_lock(), put_user_page().
> *
> */
> -void put_user_pages_dirty(struct page **pages, unsigned long npages)
> +void put_user_pages_dirty_lock(struct page **pages, unsigned long npages,
> + bool make_dirty)
> {
> - __put_user_pages_dirty(pages, npages, set_page_dirty);
> -}
> -EXPORT_SYMBOL(put_user_pages_dirty);
> + unsigned long index;
>
> -/**
> - * put_user_pages_dirty_lock() - release and dirty an array of gup-pinned pages
> - * @pages: array of pages to be marked dirty and released.
> - * @npages: number of pages in the @pages array.
> - *
> - * For each page in the @pages array, make that page (or its head page, if a
> - * compound page) dirty, if it was previously listed as clean. Then, release
> - * the page using put_user_page().
> - *
> - * Please see the put_user_page() documentation for details.
> - *
> - * This is just like put_user_pages_dirty(), except that it invokes
> - * set_page_dirty_lock(), instead of set_page_dirty().
> - *
> - */
> -void put_user_pages_dirty_lock(struct page **pages, unsigned long npages)
> -{
> - __put_user_pages_dirty(pages, npages, set_page_dirty_lock);
> + /*
> + * TODO: this can be optimized for huge pages: if a series of pages is
> + * physically contiguous and part of the same compound page, then a
> + * single operation to the head page should suffice.
> + */
I think this comment belongs to the for loop below... or just something about
how to make this and put_user_pages() more efficient. It is odd, that this is
the same comment as in put_user_pages()...
The code is good. So... Other than the comments.
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Ira
> +
> + if (!make_dirty) {
> + put_user_pages(pages, npages);
> + return;
> + }
> +
> + for (index = 0; index < npages; index++) {
> + struct page *page = compound_head(pages[index]);
> + /*
> + * Checking PageDirty at this point may race with
> + * clear_page_dirty_for_io(), but that's OK. Two key
> + * cases:
> + *
> + * 1) This code sees the page as already dirty, so it
> + * skips the call to set_page_dirty(). That could happen
> + * because clear_page_dirty_for_io() called
> + * page_mkclean(), followed by set_page_dirty().
> + * However, now the page is going to get written back,
> + * which meets the original intention of setting it
> + * dirty, so all is well: clear_page_dirty_for_io() goes
> + * on to call TestClearPageDirty(), and write the page
> + * back.
> + *
> + * 2) This code sees the page as clean, so it calls
> + * set_page_dirty(). The page stays dirty, despite being
> + * written back, so it gets written back again in the
> + * next writeback cycle. This is harmless.
> + */
> + if (!PageDirty(page))
> + set_page_dirty_lock(page);
> + put_user_page(page);
> + }
> }
> EXPORT_SYMBOL(put_user_pages_dirty_lock);
>
> --
> 2.22.0
>
next prev parent reply other threads:[~2019-08-06 17:40 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-04 22:48 [PATCH v2 00/34] put_user_pages(): miscellaneous call sites john.hubbard
2019-08-04 22:48 ` [PATCH v2 01/34] mm/gup: add make_dirty arg to put_user_pages_dirty_lock() john.hubbard
2019-08-06 17:39 ` Ira Weiny [this message]
2019-08-06 20:39 ` John Hubbard
2019-08-04 22:48 ` [PATCH v2 02/34] net/rds: convert put_page() to put_user_page*() john.hubbard
2019-08-04 22:48 ` [PATCH v2 03/34] net/ceph: " john.hubbard
2019-08-04 22:48 ` [PATCH v2 04/34] x86/kvm: " john.hubbard
2019-08-04 22:48 ` [PATCH v2 05/34] drm/etnaviv: convert release_pages() to put_user_pages() john.hubbard
2019-08-04 22:48 ` [PATCH v2 06/34] drm/i915: convert put_page() to put_user_page*() john.hubbard
2019-08-05 16:53 ` Rodrigo Vivi
2019-08-04 22:48 ` [PATCH v2 07/34] drm/radeon: " john.hubbard
2019-08-04 22:48 ` [PATCH v2 08/34] media/ivtv: " john.hubbard
2019-08-04 22:48 ` [PATCH v2 09/34] media/v4l2-core/mm: " john.hubbard
2019-08-04 22:48 ` [PATCH v2 10/34] genwqe: " john.hubbard
2019-08-04 22:48 ` [PATCH v2 11/34] scif: " john.hubbard
2019-08-04 22:48 ` [PATCH v2 12/34] vmci: " john.hubbard
2019-08-04 22:48 ` [PATCH v2 13/34] rapidio: " john.hubbard
2019-08-04 22:48 ` [PATCH v2 14/34] oradax: " john.hubbard
2019-08-04 22:48 ` [PATCH v2 15/34] staging/vc04_services: " john.hubbard
2019-08-13 5:23 ` Stefan Wahren
2019-08-04 22:48 ` [PATCH v2 16/34] drivers/tee: " john.hubbard
2019-08-04 22:48 ` [PATCH v2 17/34] vfio: " john.hubbard
2019-08-04 22:48 ` [PATCH v2 18/34] fbdev/pvr2fb: " john.hubbard
2019-08-04 22:49 ` [PATCH v2 19/34] fsl_hypervisor: " john.hubbard
2019-08-04 22:49 ` [PATCH v2 20/34] xen: " john.hubbard
2019-08-05 4:15 ` Juergen Gross
2019-08-04 22:49 ` [PATCH v2 21/34] fs/exec.c: " john.hubbard
2019-08-04 22:49 ` [PATCH v2 22/34] orangefs: " john.hubbard
2019-08-04 22:49 ` [PATCH v2 23/34] uprobes: " john.hubbard
2019-08-04 22:49 ` [PATCH v2 24/34] futex: " john.hubbard
2019-08-04 22:49 ` [PATCH v2 25/34] mm/frame_vector.c: " john.hubbard
2019-08-04 22:49 ` [PATCH v2 26/34] mm/gup_benchmark.c: " john.hubbard
2019-08-04 22:49 ` [PATCH v2 27/34] mm/memory.c: " john.hubbard
2019-08-04 22:49 ` [PATCH v2 28/34] mm/madvise.c: " john.hubbard
2019-08-04 22:49 ` [PATCH v2 29/34] mm/process_vm_access.c: " john.hubbard
2019-08-04 22:49 ` [PATCH v2 30/34] crypt: " john.hubbard
2019-08-04 22:49 ` [PATCH v2 31/34] fs/nfs: " john.hubbard
2019-08-05 0:26 ` Calum Mackay
2019-08-04 22:49 ` [PATCH v2 32/34] goldfish_pipe: " john.hubbard
2019-08-04 22:49 ` [PATCH v2 33/34] kernel/events/core.c: " john.hubbard
2019-08-04 22:49 ` [PATCH v2 34/34] fs/binfmt_elf: " john.hubbard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190806173945.GA4748@iweiny-DESK2.sc.intel.com \
--to=ira.weiny@intel.com \
--cc=akpm@linux-foundation.org \
--cc=amd-gfx@lists.freedesktop.org \
--cc=ceph-devel@vger.kernel.org \
--cc=dan.j.williams@intel.com \
--cc=dave.hansen@linux.intel.com \
--cc=david@fromorbit.com \
--cc=devel@driverdev.osuosl.org \
--cc=devel@lists.orangefs.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=hch@infradead.org \
--cc=hch@lst.de \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jack@suse.cz \
--cc=jgg@ziepe.ca \
--cc=jglisse@redhat.com \
--cc=jhubbard@nvidia.com \
--cc=john.hubbard@gmail.com \
--cc=kvm@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-crypto@vger.kernel.org \
--cc=linux-fbdev@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-nfs@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=linux-rpi-kernel@lists.infradead.org \
--cc=linux-xfs@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=rds-devel@oss.oracle.com \
--cc=sparclinux@vger.kernel.org \
--cc=willy@infradead.org \
--cc=x86@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).