From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from hqemgate14.nvidia.com (hqemgate14.nvidia.com [216.228.121.143]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8366821309D0A for ; Fri, 9 Aug 2019 17:32:42 -0700 (PDT) Subject: Re: [RFC PATCH v2 12/19] mm/gup: Prep put_user_pages() to take an vaddr_pin struct References: <20190809225833.6657-1-ira.weiny@intel.com> <20190809225833.6657-13-ira.weiny@intel.com> From: John Hubbard Message-ID: <12b6a576-7a64-102c-f4d7-7a4ad34df710@nvidia.com> Date: Fri, 9 Aug 2019 17:30:00 -0700 MIME-Version: 1.0 In-Reply-To: <20190809225833.6657-13-ira.weiny@intel.com> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: linux-nvdimm-bounces@lists.01.org Sender: "Linux-nvdimm" To: ira.weiny@intel.com, Andrew Morton Cc: Michal Hocko , Jan Kara , linux-nvdimm@lists.01.org, linux-rdma@vger.kernel.org, Dave Chinner , linux-kernel@vger.kernel.org, Matthew Wilcox , linux-xfs@vger.kernel.org, Jason Gunthorpe , linux-fsdevel@vger.kernel.org, Theodore Ts'o , linux-ext4@vger.kernel.org, linux-mm@kvack.org List-ID: On 8/9/19 3:58 PM, ira.weiny@intel.com wrote: > From: Ira Weiny > > Once callers start to use vaddr_pin the put_user_pages calls will need > to have access to this data coming in. Prep put_user_pages() for this > data. > > Signed-off-by: Ira Weiny > --- > include/linux/mm.h | 20 +------- > mm/gup.c | 122 ++++++++++++++++++++++++++++++++------------- > 2 files changed, 88 insertions(+), 54 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index befe150d17be..9d37cafbef9a 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1064,25 +1064,7 @@ static inline void put_page(struct page *page) > __put_page(page); > } > > -/** > - * put_user_page() - release a gup-pinned page > - * @page: pointer to page to be released > - * > - * Pages that were pinned via get_user_pages*() must be released via > - * either put_user_page(), or one of the put_user_pages*() routines > - * below. This is so that eventually, pages that are pinned via > - * get_user_pages*() can be separately tracked and uniquely handled. In > - * particular, interactions with RDMA and filesystems need special > - * handling. > - * > - * put_user_page() and put_page() are not interchangeable, despite this early > - * implementation that makes them look the same. put_user_page() calls must > - * be perfectly matched up with get_user_page() calls. > - */ > -static inline void put_user_page(struct page *page) > -{ > - put_page(page); > -} > +void put_user_page(struct page *page); > > void put_user_pages_dirty_lock(struct page **pages, unsigned long npages, > bool make_dirty); > diff --git a/mm/gup.c b/mm/gup.c > index a7a9d2f5278c..10cfd30ff668 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -24,30 +24,41 @@ > > #include "internal.h" > > -/** > - * put_user_pages_dirty_lock() - release and optionally dirty gup-pinned pages > - * @pages: array of pages to be maybe marked dirty, and definitely released. A couple comments from our circular review chain: some fellow with the same last name as you, recommended wording it like this: @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 @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. > - * > - * Please see the put_user_page() documentation for details. > - * > - * 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_lock(struct page **pages, unsigned long npages, > - bool make_dirty) > +static void __put_user_page(struct vaddr_pin *vaddr_pin, struct page *page) > +{ > + page = compound_head(page); > + > + /* > + * For devmap managed pages we need to catch refcount transition from > + * GUP_PIN_COUNTING_BIAS to 1, when refcount reach one it means the > + * page is free and we need to inform the device driver through > + * callback. See include/linux/memremap.h and HMM for details. > + */ > + if (put_devmap_managed_page(page)) > + return; > + > + if (put_page_testzero(page)) > + __put_page(page); > +} > + > +static void __put_user_pages(struct vaddr_pin *vaddr_pin, struct page **pages, > + unsigned long npages) > +{ > + unsigned long index; > + > + /* > + * 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. > + */ As discussed in the other review thread (""), let's just delete that comment, as long as you're moving things around. > + for (index = 0; index < npages; index++) > + __put_user_page(vaddr_pin, pages[index]); > +} > + > +static void __put_user_pages_dirty_lock(struct vaddr_pin *vaddr_pin, > + struct page **pages, > + unsigned long npages, > + bool make_dirty) Elsewhere in this series, we pass vaddr_pin at the end of the arg list. Here we pass it at the beginning, and it caused a minor jar when reading it. Obviously just bike shedding at this point, though. Either way. :) > { > unsigned long index; > > @@ -58,7 +69,7 @@ void put_user_pages_dirty_lock(struct page **pages, unsigned long npages, > */ > > if (!make_dirty) { > - put_user_pages(pages, npages); > + __put_user_pages(vaddr_pin, pages, npages); > return; > } > > @@ -86,9 +97,58 @@ void put_user_pages_dirty_lock(struct page **pages, unsigned long npages, > */ > if (!PageDirty(page)) > set_page_dirty_lock(page); > - put_user_page(page); > + __put_user_page(vaddr_pin, page); > } > } > + > +/** > + * put_user_page() - release a gup-pinned page > + * @page: pointer to page to be released > + * > + * Pages that were pinned via get_user_pages*() must be released via > + * either put_user_page(), or one of the put_user_pages*() routines > + * below. This is so that eventually, pages that are pinned via > + * get_user_pages*() can be separately tracked and uniquely handled. In > + * particular, interactions with RDMA and filesystems need special > + * handling. > + * > + * put_user_page() and put_page() are not interchangeable, despite this early > + * implementation that makes them look the same. put_user_page() calls must > + * be perfectly matched up with get_user_page() calls. > + */ > +void put_user_page(struct page *page) > +{ > + __put_user_page(NULL, page); > +} > +EXPORT_SYMBOL(put_user_page); > + > +/** > + * put_user_pages_dirty_lock() - release and optionally dirty gup-pinned pages > + * @pages: array of pages to be maybe marked dirty, and definitely released. Same here: @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 @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. > + * > + * Please see the put_user_page() documentation for details. > + * > + * 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_lock(struct page **pages, unsigned long npages, > + bool make_dirty) > +{ > + __put_user_pages_dirty_lock(NULL, pages, npages, make_dirty); > +} > EXPORT_SYMBOL(put_user_pages_dirty_lock); > > /** > @@ -102,15 +162,7 @@ EXPORT_SYMBOL(put_user_pages_dirty_lock); > */ > void put_user_pages(struct page **pages, unsigned long npages) > { > - unsigned long index; > - > - /* > - * 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. > - */ > - for (index = 0; index < npages; index++) > - put_user_page(pages[index]); > + __put_user_pages(NULL, pages, npages); > } > EXPORT_SYMBOL(put_user_pages); > > This all looks pretty good, so regardless of the outcome of the minor points above, Reviewed-by: John Hubbard thanks, -- John Hubbard NVIDIA _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm