| Message ID | 20190308213633.28978-2-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: introduce put_user_page*(), placeholder versions | expand |
On Tue, Mar 19, 2019 at 04:36:44PM +0100, Jan Kara wrote: > On Tue 19-03-19 17:29:18, Kirill A. Shutemov wrote: > > On Tue, Mar 19, 2019 at 10:14:16AM -0400, Jerome Glisse wrote: > > > On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: > > > > On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: > > > > > On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: > > > > > > From: John Hubbard <jhubbard@nvidia.com> > > > > > > > > [...] > > > > > > > > > > diff --git a/mm/gup.c b/mm/gup.c > > > > > > index f84e22685aaa..37085b8163b1 100644 > > > > > > --- a/mm/gup.c > > > > > > +++ b/mm/gup.c > > > > > > @@ -28,6 +28,88 @@ 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]); > > > > > > + > > > > > > + if (!PageDirty(page)) > > > > > > + sdf(page); > > > > > > > > > > How is this safe? What prevents the page to be cleared under you? > > > > > > > > > > If it's safe to race clear_page_dirty*() it has to be stated explicitly > > > > > with a reason why. It's not very clear to me as it is. > > > > > > > > The PageDirty() optimization above is fine to race with clear the > > > > page flag as it means it is racing after a page_mkclean() and the > > > > GUP user is done with the page so page is about to be write back > > > > ie if (!PageDirty(page)) see the page as dirty and skip the sdf() > > > > call while a split second after TestClearPageDirty() happens then > > > > it means the racing clear is about to write back the page so all > > > > is fine (the page was dirty and it is being clear for write back). > > > > > > > > If it does call the sdf() while racing with write back then we > > > > just redirtied the page just like clear_page_dirty_for_io() would > > > > do if page_mkclean() failed so nothing harmful will come of that > > > > neither. Page stays dirty despite write back it just means that > > > > the page might be write back twice in a row. > > > > > > Forgot to mention one thing, we had a discussion with Andrea and Jan > > > about set_page_dirty() and Andrea had the good idea of maybe doing > > > the set_page_dirty() at GUP time (when GUP with write) not when the > > > GUP user calls put_page(). We can do that by setting the dirty bit > > > in the pte for instance. They are few bonus of doing things that way: > > > - amortize the cost of calling set_page_dirty() (ie one call for > > > GUP and page_mkclean() > > > - it is always safe to do so at GUP time (ie the pte has write > > > permission and thus the page is in correct state) > > > - safe from truncate race > > > - no need to ever lock the page > > > > > > Extra bonus from my point of view, it simplify thing for my generic > > > page protection patchset (KSM for file back page). > > > > > > So maybe we should explore that ? It would also be a lot less code. > > > > Yes, please. It sounds more sensible to me to dirty the page on get, not > > on put. > > I fully agree this is a desirable final state of affairs. I'm glad to see this presented because it has crossed my mind more than once that effectively a GUP pinned page should be considered "dirty" at all times until the pin is removed. This is especially true in the RDMA case. > And with changes > to how we treat pinned pages during writeback there won't have to be any > explicit dirtying at all in the end because the page is guaranteed to be > dirty after a write page fault and pin would make sure it stays dirty until > unpinned. However initially I want the helpers to be as close to code they > are replacing as possible. Because it will be hard to catch all the bugs > due to driver conversions even in that situation. So I still think that > these helpers as they are a good first step. Then we need to convert > GUP users to use them and then it is much easier to modify the behavior > since it is no longer opencoded in two hudred or how many places... Agreed. I continue to test with these patches and RDMA and have not seen any problems thus far. Ira > > Honza > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR
On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: > From: John Hubbard <jhubbard@nvidia.com> > > Introduces put_user_page(), which simply calls put_page(). > This provides a way to update all get_user_pages*() callers, > so that they call put_user_page(), instead of put_page(). > > Also introduces put_user_pages(), and a few dirty/locked variations, > as a replacement for release_pages(), and also as a replacement > for open-coded loops that release multiple pages. > These may be used for subsequent performance improvements, > via batching of pages to be released. > > This is the first step of fixing a problem (also described in [1] and > [2]) with interactions between get_user_pages ("gup") and filesystems. > > Problem description: let's start with a bug report. Below, is what happens > sometimes, under memory pressure, when a driver pins some pages via gup, > and then marks those pages dirty, and releases them. Note that the gup > documentation actually recommends that pattern. The problem is that the > filesystem may do a writeback while the pages were gup-pinned, and then the > filesystem believes that the pages are clean. So, when the driver later > marks the pages as dirty, that conflicts with the filesystem's page > tracking and results in a BUG(), like this one that I experienced: > > kernel BUG at /build/linux-fQ94TU/linux-4.4.0/fs/ext4/inode.c:1899! > backtrace: > ext4_writepage > __writepage > write_cache_pages > ext4_writepages > do_writepages > __writeback_single_inode > writeback_sb_inodes > __writeback_inodes_wb > wb_writeback > wb_workfn > process_one_work > worker_thread > kthread > ret_from_fork > > ...which is due to the file system asserting that there are still buffer > heads attached: > > ({ \ > BUG_ON(!PagePrivate(page)); \ > ((struct buffer_head *)page_private(page)); \ > }) > > Dave Chinner's description of this is very clear: > > "The fundamental issue is that ->page_mkwrite must be called on every > write access to a clean file backed page, not just the first one. > How long the GUP reference lasts is irrelevant, if the page is clean > and you need to dirty it, you must call ->page_mkwrite before it is > marked writeable and dirtied. Every. Time." > > This is just one symptom of the larger design problem: real filesystems > that actually write to a backing device, do not actually support > get_user_pages() being called on their pages, and letting hardware write > directly to those pages--even though that pattern has been going on since > about 2005 or so. > > The steps are to fix it are: > > 1) (This patch): provide put_user_page*() routines, intended to be used > for releasing pages that were pinned via get_user_pages*(). > > 2) Convert all of the call sites for get_user_pages*(), to > invoke put_user_page*(), instead of put_page(). This involves dozens of > call sites, and will take some time. > > 3) After (2) is complete, use get_user_pages*() and put_user_page*() to > implement tracking of these pages. This tracking will be separate from > the existing struct page refcounting. > > 4) Use the tracking and identification of these pages, to implement > special handling (especially in writeback paths) when the pages are > backed by a filesystem. > > [1] https://lwn.net/Articles/774411/ : "DMA and get_user_pages()" > [2] https://lwn.net/Articles/753027/ : "The Trouble with get_user_pages()" > > Cc: Al Viro <viro@zeniv.linux.org.uk> > Cc: Christoph Hellwig <hch@infradead.org> > Cc: Christopher Lameter <cl@linux.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Cc: Dave Chinner <david@fromorbit.com> > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Jan Kara <jack@suse.cz> > Cc: Jason Gunthorpe <jgg@ziepe.ca> > Cc: Jerome Glisse <jglisse@redhat.com> > Cc: Matthew Wilcox <willy@infradead.org> > Cc: Michal Hocko <mhocko@kernel.org> > Cc: Mike Rapoport <rppt@linux.ibm.com> > Cc: Ralph Campbell <rcampbell@nvidia.com> > > Reviewed-by: Jan Kara <jack@suse.cz> > Reviewed-by: Mike Rapoport <rppt@linux.ibm.com> # docs > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Reviewed-by: Jérôme Glisse <jglisse@redhat.com> > Signed-off-by: John Hubbard <jhubbard@nvidia.com> > --- > include/linux/mm.h | 24 ++++++++++++++ > mm/gup.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 106 insertions(+) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 5801ee849f36..353035c8b115 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -993,6 +993,30 @@ 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_pages_dirty(struct page **pages, unsigned long npages); > +void put_user_pages_dirty_lock(struct page **pages, unsigned long npages); > +void put_user_pages(struct page **pages, unsigned long npages); > + > #if defined(CONFIG_SPARSEMEM) && !defined(CONFIG_SPARSEMEM_VMEMMAP) > #define SECTION_IN_PAGE_FLAGS > #endif > diff --git a/mm/gup.c b/mm/gup.c > index f84e22685aaa..37085b8163b1 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -28,6 +28,88 @@ 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]); > + > + if (!PageDirty(page)) > + sdf(page); How is this safe? What prevents the page to be cleared under you? If it's safe to race clear_page_dirty*() it has to be stated explicitly with a reason why. It's not very clear to me as it is. > + > + 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. > + * @npages: number of pages in the @pages array. > + * > + * "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(). > + * > + * 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. > + * > + */ > +void put_user_pages_dirty(struct page **pages, unsigned long npages) > +{ > + __put_user_pages_dirty(pages, npages, set_page_dirty); Have you checked if compiler is clever enough eliminate indirect function call here? Maybe it's better to go with an opencodded approach and get rid of callbacks? > +} > +EXPORT_SYMBOL(put_user_pages_dirty); > + > +/** > + * 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); > +} > +EXPORT_SYMBOL(put_user_pages_dirty_lock); > + > +/** > + * put_user_pages() - release 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, release the page using put_user_page(). > + * > + * Please see the put_user_page() documentation for details. > + */ > +void put_user_pages(struct page **pages, unsigned long npages) > +{ > + unsigned long index; > + > + for (index = 0; index < npages; index++) > + put_user_page(pages[index]); I believe there's an room for improvement for compound pages. If there's multiple consequential pages in the array that belong to the same compound page we can get away with a single atomic operation to handle them all. > +} > +EXPORT_SYMBOL(put_user_pages); > + > static struct page *no_page_table(struct vm_area_struct *vma, > unsigned int flags) > { > -- > 2.21.0 >
On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: > On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: > > From: John Hubbard <jhubbard@nvidia.com> [...] > > diff --git a/mm/gup.c b/mm/gup.c > > index f84e22685aaa..37085b8163b1 100644 > > --- a/mm/gup.c > > +++ b/mm/gup.c > > @@ -28,6 +28,88 @@ 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]); > > + > > + if (!PageDirty(page)) > > + sdf(page); > > How is this safe? What prevents the page to be cleared under you? > > If it's safe to race clear_page_dirty*() it has to be stated explicitly > with a reason why. It's not very clear to me as it is. The PageDirty() optimization above is fine to race with clear the page flag as it means it is racing after a page_mkclean() and the GUP user is done with the page so page is about to be write back ie if (!PageDirty(page)) see the page as dirty and skip the sdf() call while a split second after TestClearPageDirty() happens then it means the racing clear is about to write back the page so all is fine (the page was dirty and it is being clear for write back). If it does call the sdf() while racing with write back then we just redirtied the page just like clear_page_dirty_for_io() would do if page_mkclean() failed so nothing harmful will come of that neither. Page stays dirty despite write back it just means that the page might be write back twice in a row. > > + > > + 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. > > + * @npages: number of pages in the @pages array. > > + * > > + * "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(). > > + * > > + * 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. > > + * > > + */ > > +void put_user_pages_dirty(struct page **pages, unsigned long npages) > > +{ > > + __put_user_pages_dirty(pages, npages, set_page_dirty); > > Have you checked if compiler is clever enough eliminate indirect function > call here? Maybe it's better to go with an opencodded approach and get rid > of callbacks? > Good point, dunno if John did check that. > > > +} > > +EXPORT_SYMBOL(put_user_pages_dirty); > > + > > +/** > > + * 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); > > +} > > +EXPORT_SYMBOL(put_user_pages_dirty_lock); > > + > > +/** > > + * put_user_pages() - release 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, release the page using put_user_page(). > > + * > > + * Please see the put_user_page() documentation for details. > > + */ > > +void put_user_pages(struct page **pages, unsigned long npages) > > +{ > > + unsigned long index; > > + > > + for (index = 0; index < npages; index++) > > + put_user_page(pages[index]); > > I believe there's an room for improvement for compound pages. > > If there's multiple consequential pages in the array that belong to the > same compound page we can get away with a single atomic operation to > handle them all. Yes maybe just add a comment with that for now and leave this kind of optimization to latter ?
On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: > On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: > > On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: > > > From: John Hubbard <jhubbard@nvidia.com> > > [...] > > > > diff --git a/mm/gup.c b/mm/gup.c > > > index f84e22685aaa..37085b8163b1 100644 > > > --- a/mm/gup.c > > > +++ b/mm/gup.c > > > @@ -28,6 +28,88 @@ 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]); > > > + > > > + if (!PageDirty(page)) > > > + sdf(page); > > > > How is this safe? What prevents the page to be cleared under you? > > > > If it's safe to race clear_page_dirty*() it has to be stated explicitly > > with a reason why. It's not very clear to me as it is. > > The PageDirty() optimization above is fine to race with clear the > page flag as it means it is racing after a page_mkclean() and the > GUP user is done with the page so page is about to be write back > ie if (!PageDirty(page)) see the page as dirty and skip the sdf() > call while a split second after TestClearPageDirty() happens then > it means the racing clear is about to write back the page so all > is fine (the page was dirty and it is being clear for write back). > > If it does call the sdf() while racing with write back then we > just redirtied the page just like clear_page_dirty_for_io() would > do if page_mkclean() failed so nothing harmful will come of that > neither. Page stays dirty despite write back it just means that > the page might be write back twice in a row. Fair enough. Should we get it into a comment here? > > > +void put_user_pages(struct page **pages, unsigned long npages) > > > +{ > > > + unsigned long index; > > > + > > > + for (index = 0; index < npages; index++) > > > + put_user_page(pages[index]); > > > > I believe there's an room for improvement for compound pages. > > > > If there's multiple consequential pages in the array that belong to the > > same compound page we can get away with a single atomic operation to > > handle them all. > > Yes maybe just add a comment with that for now and leave this kind of > optimization to latter ? Sounds good to me.
On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: > On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: > > On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: > > > From: John Hubbard <jhubbard@nvidia.com> > > [...] > > > > diff --git a/mm/gup.c b/mm/gup.c > > > index f84e22685aaa..37085b8163b1 100644 > > > --- a/mm/gup.c > > > +++ b/mm/gup.c > > > @@ -28,6 +28,88 @@ 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]); > > > + > > > + if (!PageDirty(page)) > > > + sdf(page); > > > > How is this safe? What prevents the page to be cleared under you? > > > > If it's safe to race clear_page_dirty*() it has to be stated explicitly > > with a reason why. It's not very clear to me as it is. > > The PageDirty() optimization above is fine to race with clear the > page flag as it means it is racing after a page_mkclean() and the > GUP user is done with the page so page is about to be write back > ie if (!PageDirty(page)) see the page as dirty and skip the sdf() > call while a split second after TestClearPageDirty() happens then > it means the racing clear is about to write back the page so all > is fine (the page was dirty and it is being clear for write back). > > If it does call the sdf() while racing with write back then we > just redirtied the page just like clear_page_dirty_for_io() would > do if page_mkclean() failed so nothing harmful will come of that > neither. Page stays dirty despite write back it just means that > the page might be write back twice in a row. Forgot to mention one thing, we had a discussion with Andrea and Jan about set_page_dirty() and Andrea had the good idea of maybe doing the set_page_dirty() at GUP time (when GUP with write) not when the GUP user calls put_page(). We can do that by setting the dirty bit in the pte for instance. They are few bonus of doing things that way: - amortize the cost of calling set_page_dirty() (ie one call for GUP and page_mkclean() - it is always safe to do so at GUP time (ie the pte has write permission and thus the page is in correct state) - safe from truncate race - no need to ever lock the page Extra bonus from my point of view, it simplify thing for my generic page protection patchset (KSM for file back page). So maybe we should explore that ? It would also be a lot less code. Cheers, Jérôme
On Tue, Mar 19, 2019 at 05:06:23PM +0300, Kirill A. Shutemov wrote: > On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: > > On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: > > > On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: > > > > From: John Hubbard <jhubbard@nvidia.com> > > > > [...] > > > > > > diff --git a/mm/gup.c b/mm/gup.c > > > > index f84e22685aaa..37085b8163b1 100644 > > > > --- a/mm/gup.c > > > > +++ b/mm/gup.c > > > > @@ -28,6 +28,88 @@ 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]); > > > > + > > > > + if (!PageDirty(page)) > > > > + sdf(page); > > > > > > How is this safe? What prevents the page to be cleared under you? > > > > > > If it's safe to race clear_page_dirty*() it has to be stated explicitly > > > with a reason why. It's not very clear to me as it is. > > > > The PageDirty() optimization above is fine to race with clear the > > page flag as it means it is racing after a page_mkclean() and the > > GUP user is done with the page so page is about to be write back > > ie if (!PageDirty(page)) see the page as dirty and skip the sdf() > > call while a split second after TestClearPageDirty() happens then > > it means the racing clear is about to write back the page so all > > is fine (the page was dirty and it is being clear for write back). > > > > If it does call the sdf() while racing with write back then we > > just redirtied the page just like clear_page_dirty_for_io() would > > do if page_mkclean() failed so nothing harmful will come of that > > neither. Page stays dirty despite write back it just means that > > the page might be write back twice in a row. > > Fair enough. Should we get it into a comment here? Yes definitly also i just sent an email with an alternative that is slightly better from my POV as it simplify my life for other things :) Cheers, Jérôme
On Tue, Mar 19, 2019 at 10:14:16AM -0400, Jerome Glisse wrote: > On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: > > On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: > > > On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: > > > > From: John Hubbard <jhubbard@nvidia.com> > > > > [...] > > > > > > diff --git a/mm/gup.c b/mm/gup.c > > > > index f84e22685aaa..37085b8163b1 100644 > > > > --- a/mm/gup.c > > > > +++ b/mm/gup.c > > > > @@ -28,6 +28,88 @@ 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]); > > > > + > > > > + if (!PageDirty(page)) > > > > + sdf(page); > > > > > > How is this safe? What prevents the page to be cleared under you? > > > > > > If it's safe to race clear_page_dirty*() it has to be stated explicitly > > > with a reason why. It's not very clear to me as it is. > > > > The PageDirty() optimization above is fine to race with clear the > > page flag as it means it is racing after a page_mkclean() and the > > GUP user is done with the page so page is about to be write back > > ie if (!PageDirty(page)) see the page as dirty and skip the sdf() > > call while a split second after TestClearPageDirty() happens then > > it means the racing clear is about to write back the page so all > > is fine (the page was dirty and it is being clear for write back). > > > > If it does call the sdf() while racing with write back then we > > just redirtied the page just like clear_page_dirty_for_io() would > > do if page_mkclean() failed so nothing harmful will come of that > > neither. Page stays dirty despite write back it just means that > > the page might be write back twice in a row. > > Forgot to mention one thing, we had a discussion with Andrea and Jan > about set_page_dirty() and Andrea had the good idea of maybe doing > the set_page_dirty() at GUP time (when GUP with write) not when the > GUP user calls put_page(). We can do that by setting the dirty bit > in the pte for instance. They are few bonus of doing things that way: > - amortize the cost of calling set_page_dirty() (ie one call for > GUP and page_mkclean() > - it is always safe to do so at GUP time (ie the pte has write > permission and thus the page is in correct state) > - safe from truncate race > - no need to ever lock the page > > Extra bonus from my point of view, it simplify thing for my generic > page protection patchset (KSM for file back page). > > So maybe we should explore that ? It would also be a lot less code. Yes, please. It sounds more sensible to me to dirty the page on get, not on put.
On Tue 19-03-19 17:29:18, Kirill A. Shutemov wrote: > On Tue, Mar 19, 2019 at 10:14:16AM -0400, Jerome Glisse wrote: > > On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: > > > On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: > > > > On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: > > > > > From: John Hubbard <jhubbard@nvidia.com> > > > > > > [...] > > > > > > > > diff --git a/mm/gup.c b/mm/gup.c > > > > > index f84e22685aaa..37085b8163b1 100644 > > > > > --- a/mm/gup.c > > > > > +++ b/mm/gup.c > > > > > @@ -28,6 +28,88 @@ 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]); > > > > > + > > > > > + if (!PageDirty(page)) > > > > > + sdf(page); > > > > > > > > How is this safe? What prevents the page to be cleared under you? > > > > > > > > If it's safe to race clear_page_dirty*() it has to be stated explicitly > > > > with a reason why. It's not very clear to me as it is. > > > > > > The PageDirty() optimization above is fine to race with clear the > > > page flag as it means it is racing after a page_mkclean() and the > > > GUP user is done with the page so page is about to be write back > > > ie if (!PageDirty(page)) see the page as dirty and skip the sdf() > > > call while a split second after TestClearPageDirty() happens then > > > it means the racing clear is about to write back the page so all > > > is fine (the page was dirty and it is being clear for write back). > > > > > > If it does call the sdf() while racing with write back then we > > > just redirtied the page just like clear_page_dirty_for_io() would > > > do if page_mkclean() failed so nothing harmful will come of that > > > neither. Page stays dirty despite write back it just means that > > > the page might be write back twice in a row. > > > > Forgot to mention one thing, we had a discussion with Andrea and Jan > > about set_page_dirty() and Andrea had the good idea of maybe doing > > the set_page_dirty() at GUP time (when GUP with write) not when the > > GUP user calls put_page(). We can do that by setting the dirty bit > > in the pte for instance. They are few bonus of doing things that way: > > - amortize the cost of calling set_page_dirty() (ie one call for > > GUP and page_mkclean() > > - it is always safe to do so at GUP time (ie the pte has write > > permission and thus the page is in correct state) > > - safe from truncate race > > - no need to ever lock the page > > > > Extra bonus from my point of view, it simplify thing for my generic > > page protection patchset (KSM for file back page). > > > > So maybe we should explore that ? It would also be a lot less code. > > Yes, please. It sounds more sensible to me to dirty the page on get, not > on put. I fully agree this is a desirable final state of affairs. And with changes to how we treat pinned pages during writeback there won't have to be any explicit dirtying at all in the end because the page is guaranteed to be dirty after a write page fault and pin would make sure it stays dirty until unpinned. However initially I want the helpers to be as close to code they are replacing as possible. Because it will be hard to catch all the bugs due to driver conversions even in that situation. So I still think that these helpers as they are a good first step. Then we need to convert GUP users to use them and then it is much easier to modify the behavior since it is no longer opencoded in two hudred or how many places... Honza
On 3/19/19 8:36 AM, Jan Kara wrote: > On Tue 19-03-19 17:29:18, Kirill A. Shutemov wrote: >> On Tue, Mar 19, 2019 at 10:14:16AM -0400, Jerome Glisse wrote: >>> On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: >>>> On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: >>>>> On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: >>>>>> From: John Hubbard <jhubbard@nvidia.com> >>>> [...] >>> Forgot to mention one thing, we had a discussion with Andrea and Jan >>> about set_page_dirty() and Andrea had the good idea of maybe doing >>> the set_page_dirty() at GUP time (when GUP with write) not when the >>> GUP user calls put_page(). We can do that by setting the dirty bit >>> in the pte for instance. They are few bonus of doing things that way: >>> - amortize the cost of calling set_page_dirty() (ie one call for >>> GUP and page_mkclean() >>> - it is always safe to do so at GUP time (ie the pte has write >>> permission and thus the page is in correct state) >>> - safe from truncate race >>> - no need to ever lock the page >>> >>> Extra bonus from my point of view, it simplify thing for my generic >>> page protection patchset (KSM for file back page). >>> >>> So maybe we should explore that ? It would also be a lot less code. >> >> Yes, please. It sounds more sensible to me to dirty the page on get, not >> on put. > > I fully agree this is a desirable final state of affairs. And with changes > to how we treat pinned pages during writeback there won't have to be any > explicit dirtying at all in the end because the page is guaranteed to be > dirty after a write page fault and pin would make sure it stays dirty until > unpinned. However initially I want the helpers to be as close to code they > are replacing as possible. Because it will be hard to catch all the bugs > due to driver conversions even in that situation. So I still think that > these helpers as they are a good first step. Then we need to convert > GUP users to use them and then it is much easier to modify the behavior > since it is no longer opencoded in two hudred or how many places... > > Honza In fact, we had this very same question come up last month [1]: I was also wondering if we should just jump directly to the final step, and not do the dirtying call, but it is true that during the conversion process, (which effectively wraps put_page(), without changing anything else), it's safer to avoid changing things. The whole system is fragile because it's running something that has some latent bugs in this area, so probably best to do it the way Jan says, and avoid causing any new instances of reproducing this problem, even though there is a bit more churn involved. [1] https://lore.kernel.org/r/20190205112107.GB3872@quack2.suse.cz thanks,
On 3/19/19 6:47 AM, Jerome Glisse wrote: > On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: >> On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: >>> From: John Hubbard <jhubbard@nvidia.com> > > [...] >>> +void put_user_pages_dirty(struct page **pages, unsigned long npages) >>> +{ >>> + __put_user_pages_dirty(pages, npages, set_page_dirty); >> >> Have you checked if compiler is clever enough eliminate indirect function >> call here? Maybe it's better to go with an opencodded approach and get rid >> of callbacks? >> > > Good point, dunno if John did check that. Hi Kirill, Jerome, The compiler does *not* eliminate the indirect function call, at least unless I'm misunderstanding things. The __put_user_pages_dirty() function calls the appropriate set_page_dirty*() call, via __x86_indirect_thunk_r12, which seems pretty definitive. ffffffff81a00ef0 <__x86_indirect_thunk_r12>: ffffffff81a00ef0: 41 ff e4 jmpq *%r12 ffffffff81a00ef3: 90 nop ffffffff81a00ef4: 90 nop ffffffff81a00ef5: 90 nop ffffffff81a00ef6: 90 nop ffffffff81a00ef7: 90 nop ffffffff81a00ef8: 90 nop ffffffff81a00ef9: 90 nop ffffffff81a00efa: 90 nop ffffffff81a00efb: 90 nop ffffffff81a00efc: 90 nop ffffffff81a00efd: 90 nop ffffffff81a00efe: 90 nop ffffffff81a00eff: 90 nop ffffffff81a00f00: 90 nop ffffffff81a00f01: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1) ffffffff81a00f08: 00 00 00 00 ffffffff81a00f0c: 0f 1f 40 00 nopl 0x0(%rax) However, there is no visible overhead to doing so, at a macro level. An fio O_DIRECT run with and without the full conversion patchset shows the same numbers: cat fio.conf [reader] direct=1 ioengine=libaio blocksize=4096 size=1g numjobs=1 rw=read iodepth=64 ===================== Before (baseline): ===================== reader: (g=0): rw=read, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T) 4096B-4096B, ioengine=libaio, iodepth=64 fio-3.3 Starting 1 process reader: (groupid=0, jobs=1): err= 0: pid=1828: Mon Mar 18 14:56:22 2019 read: IOPS=192k, BW=751MiB/s (787MB/s)(1024MiB/1364msec) slat (nsec): min=1274, max=42375, avg=1564.12, stdev=682.65 clat (usec): min=168, max=12209, avg=331.01, stdev=184.95 lat (usec): min=171, max=12215, avg=332.61, stdev=185.11 clat percentiles (usec): | 1.00th=[ 326], 5.00th=[ 326], 10.00th=[ 326], 20.00th=[ 326], | 30.00th=[ 326], 40.00th=[ 326], 50.00th=[ 326], 60.00th=[ 326], | 70.00th=[ 326], 80.00th=[ 326], 90.00th=[ 326], 95.00th=[ 326], | 99.00th=[ 519], 99.50th=[ 523], 99.90th=[ 537], 99.95th=[ 594], | 99.99th=[12125] bw ( KiB/s): min=755280, max=783016, per=100.00%, avg=769148.00, stdev=19612.31, samples=2 iops : min=188820, max=195754, avg=192287.00, stdev=4903.08, samples=2 lat (usec) : 250=0.14%, 500=98.59%, 750=1.25% lat (msec) : 20=0.02% cpu : usr=12.69%, sys=48.20%, ctx=248836, majf=0, minf=73 IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=0.1%, >=64=100.0% submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0% complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.1%, >=64=0.0% issued rwts: total=262144,0,0,0 short=0,0,0,0 dropped=0,0,0,0 latency : target=0, window=0, percentile=100.00%, depth=64 Run status group 0 (all jobs): READ: bw=751MiB/s (787MB/s), 751MiB/s-751MiB/s (787MB/s-787MB/s), io=1024MiB (1074MB), run=1364-1364msec Disk stats (read/write): nvme0n1: ios=220106/0, merge=0/0, ticks=70136/0, in_queue=704, util=91.19% ================================================== After (with enough callsites converted to run fio: ================================================== reader: (g=0): rw=read, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T) 4096B-4096B, ioengine=libaio, iodepth=64 fio-3.3 Starting 1 process reader: (groupid=0, jobs=1): err= 0: pid=2026: Mon Mar 18 14:35:07 2019 read: IOPS=192k, BW=751MiB/s (787MB/s)(1024MiB/1364msec) slat (nsec): min=1263, max=41861, avg=1591.99, stdev=692.09 clat (usec): min=154, max=12205, avg=330.82, stdev=184.98 lat (usec): min=157, max=12212, avg=332.45, stdev=185.14 clat percentiles (usec): | 1.00th=[ 322], 5.00th=[ 326], 10.00th=[ 326], 20.00th=[ 326], | 30.00th=[ 326], 40.00th=[ 326], 50.00th=[ 326], 60.00th=[ 326], | 70.00th=[ 326], 80.00th=[ 326], 90.00th=[ 326], 95.00th=[ 326], | 99.00th=[ 502], 99.50th=[ 510], 99.90th=[ 523], 99.95th=[ 570], | 99.99th=[12125] bw ( KiB/s): min=746848, max=783088, per=99.51%, avg=764968.00, stdev=25625.55, samples=2 iops : min=186712, max=195772, avg=191242.00, stdev=6406.39, samples=2 lat (usec) : 250=0.09%, 500=98.88%, 750=1.01% lat (msec) : 20=0.02% cpu : usr=14.38%, sys=48.64%, ctx=248037, majf=0, minf=73 IO depths : 1=0.1%, 2=0.1%, 4=0.1%, 8=0.1%, 16=0.1%, 32=0.1%, >=64=100.0% submit : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.0%, >=64=0.0% complete : 0=0.0%, 4=100.0%, 8=0.0%, 16=0.0%, 32=0.0%, 64=0.1%, >=64=0.0% issued rwts: total=262144,0,0,0 short=0,0,0,0 dropped=0,0,0,0 latency : target=0, window=0, percentile=100.00%, depth=64 Run status group 0 (all jobs): READ: bw=751MiB/s (787MB/s), 751MiB/s-751MiB/s (787MB/s-787MB/s), io=1024MiB (1074MB), run=1364-1364msec Disk stats (read/write): nvme0n1: ios=220228/0, merge=0/0, ticks=70426/0, in_queue=704, util=91.27% So, I could be persuaded either way. But given the lack of an visible perf effects, and given that this could will get removed anyway because we'll likely end up with set_page_dirty() called at GUP time instead...it seems like it's probably OK to just leave it as is. thanks,
On 3/19/19 7:06 AM, Kirill A. Shutemov wrote: > On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: >> On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: >>> On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: >>>> From: John Hubbard <jhubbard@nvidia.com> >> >> [...] >> >>>> diff --git a/mm/gup.c b/mm/gup.c >>>> index f84e22685aaa..37085b8163b1 100644 >>>> --- a/mm/gup.c >>>> +++ b/mm/gup.c >>>> @@ -28,6 +28,88 @@ 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]); >>>> + >>>> + if (!PageDirty(page)) >>>> + sdf(page); >>> >>> How is this safe? What prevents the page to be cleared under you? >>> >>> If it's safe to race clear_page_dirty*() it has to be stated explicitly >>> with a reason why. It's not very clear to me as it is. >> >> The PageDirty() optimization above is fine to race with clear the >> page flag as it means it is racing after a page_mkclean() and the >> GUP user is done with the page so page is about to be write back >> ie if (!PageDirty(page)) see the page as dirty and skip the sdf() >> call while a split second after TestClearPageDirty() happens then >> it means the racing clear is about to write back the page so all >> is fine (the page was dirty and it is being clear for write back). >> >> If it does call the sdf() while racing with write back then we >> just redirtied the page just like clear_page_dirty_for_io() would >> do if page_mkclean() failed so nothing harmful will come of that >> neither. Page stays dirty despite write back it just means that >> the page might be write back twice in a row. > > Fair enough. Should we get it into a comment here? How's this read to you? I reworded and slightly expanded Jerome's description: diff --git a/mm/gup.c b/mm/gup.c index d1df7b8ba973..86397ae23922 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -61,6 +61,24 @@ static void __put_user_pages_dirty(struct page **pages, 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); > >>>> +void put_user_pages(struct page **pages, unsigned long npages) >>>> +{ >>>> + unsigned long index; >>>> + >>>> + for (index = 0; index < npages; index++) >>>> + put_user_page(pages[index]); >>> >>> I believe there's an room for improvement for compound pages. >>> >>> If there's multiple consequential pages in the array that belong to the >>> same compound page we can get away with a single atomic operation to >>> handle them all. >> >> Yes maybe just add a comment with that for now and leave this kind of >> optimization to latter ? > > Sounds good to me. > Here's a comment for that: @@ -127,6 +145,11 @@ 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]); } thanks,
On 3/19/2019 4:03 AM, Ira Weiny wrote: > On Tue, Mar 19, 2019 at 04:36:44PM +0100, Jan Kara wrote: >> On Tue 19-03-19 17:29:18, Kirill A. Shutemov wrote: >>> On Tue, Mar 19, 2019 at 10:14:16AM -0400, Jerome Glisse wrote: >>>> On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: >>>>> On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: >>>>>> On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: >>>>>>> From: John Hubbard <jhubbard@nvidia.com> >>>>> >>>>> [...] >>>>> >>>>>>> diff --git a/mm/gup.c b/mm/gup.c >>>>>>> index f84e22685aaa..37085b8163b1 100644 >>>>>>> --- a/mm/gup.c >>>>>>> +++ b/mm/gup.c >>>>>>> @@ -28,6 +28,88 @@ 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]); >>>>>>> + >>>>>>> + if (!PageDirty(page)) >>>>>>> + sdf(page); >>>>>> >>>>>> How is this safe? What prevents the page to be cleared under you? >>>>>> >>>>>> If it's safe to race clear_page_dirty*() it has to be stated explicitly >>>>>> with a reason why. It's not very clear to me as it is. >>>>> >>>>> The PageDirty() optimization above is fine to race with clear the >>>>> page flag as it means it is racing after a page_mkclean() and the >>>>> GUP user is done with the page so page is about to be write back >>>>> ie if (!PageDirty(page)) see the page as dirty and skip the sdf() >>>>> call while a split second after TestClearPageDirty() happens then >>>>> it means the racing clear is about to write back the page so all >>>>> is fine (the page was dirty and it is being clear for write back). >>>>> >>>>> If it does call the sdf() while racing with write back then we >>>>> just redirtied the page just like clear_page_dirty_for_io() would >>>>> do if page_mkclean() failed so nothing harmful will come of that >>>>> neither. Page stays dirty despite write back it just means that >>>>> the page might be write back twice in a row. >>>> >>>> Forgot to mention one thing, we had a discussion with Andrea and Jan >>>> about set_page_dirty() and Andrea had the good idea of maybe doing >>>> the set_page_dirty() at GUP time (when GUP with write) not when the >>>> GUP user calls put_page(). We can do that by setting the dirty bit >>>> in the pte for instance. They are few bonus of doing things that way: >>>> - amortize the cost of calling set_page_dirty() (ie one call for >>>> GUP and page_mkclean() >>>> - it is always safe to do so at GUP time (ie the pte has write >>>> permission and thus the page is in correct state) >>>> - safe from truncate race >>>> - no need to ever lock the page >>>> >>>> Extra bonus from my point of view, it simplify thing for my generic >>>> page protection patchset (KSM for file back page). >>>> >>>> So maybe we should explore that ? It would also be a lot less code. >>> >>> Yes, please. It sounds more sensible to me to dirty the page on get, not >>> on put. >> >> I fully agree this is a desirable final state of affairs. > > I'm glad to see this presented because it has crossed my mind more than once > that effectively a GUP pinned page should be considered "dirty" at all times > until the pin is removed. This is especially true in the RDMA case. But, what if the RDMA registration is readonly? That's not uncommon, and marking dirty unconditonally would add needless overhead to such pages. Tom.
On Tue, Mar 19, 2019 at 03:43:44PM -0500, Tom Talpey wrote: > On 3/19/2019 4:03 AM, Ira Weiny wrote: > > On Tue, Mar 19, 2019 at 04:36:44PM +0100, Jan Kara wrote: > > > On Tue 19-03-19 17:29:18, Kirill A. Shutemov wrote: > > > > On Tue, Mar 19, 2019 at 10:14:16AM -0400, Jerome Glisse wrote: > > > > > On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: > > > > > > On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: > > > > > > > On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: > > > > > > > > From: John Hubbard <jhubbard@nvidia.com> > > > > > > > > > > > > [...] > > > > > > > > > > > > > > diff --git a/mm/gup.c b/mm/gup.c > > > > > > > > index f84e22685aaa..37085b8163b1 100644 > > > > > > > > --- a/mm/gup.c > > > > > > > > +++ b/mm/gup.c > > > > > > > > @@ -28,6 +28,88 @@ 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]); > > > > > > > > + > > > > > > > > + if (!PageDirty(page)) > > > > > > > > + sdf(page); > > > > > > > > > > > > > > How is this safe? What prevents the page to be cleared under you? > > > > > > > > > > > > > > If it's safe to race clear_page_dirty*() it has to be stated explicitly > > > > > > > with a reason why. It's not very clear to me as it is. > > > > > > > > > > > > The PageDirty() optimization above is fine to race with clear the > > > > > > page flag as it means it is racing after a page_mkclean() and the > > > > > > GUP user is done with the page so page is about to be write back > > > > > > ie if (!PageDirty(page)) see the page as dirty and skip the sdf() > > > > > > call while a split second after TestClearPageDirty() happens then > > > > > > it means the racing clear is about to write back the page so all > > > > > > is fine (the page was dirty and it is being clear for write back). > > > > > > > > > > > > If it does call the sdf() while racing with write back then we > > > > > > just redirtied the page just like clear_page_dirty_for_io() would > > > > > > do if page_mkclean() failed so nothing harmful will come of that > > > > > > neither. Page stays dirty despite write back it just means that > > > > > > the page might be write back twice in a row. > > > > > > > > > > Forgot to mention one thing, we had a discussion with Andrea and Jan > > > > > about set_page_dirty() and Andrea had the good idea of maybe doing > > > > > the set_page_dirty() at GUP time (when GUP with write) not when the > > > > > GUP user calls put_page(). We can do that by setting the dirty bit > > > > > in the pte for instance. They are few bonus of doing things that way: > > > > > - amortize the cost of calling set_page_dirty() (ie one call for > > > > > GUP and page_mkclean() > > > > > - it is always safe to do so at GUP time (ie the pte has write > > > > > permission and thus the page is in correct state) > > > > > - safe from truncate race > > > > > - no need to ever lock the page > > > > > > > > > > Extra bonus from my point of view, it simplify thing for my generic > > > > > page protection patchset (KSM for file back page). > > > > > > > > > > So maybe we should explore that ? It would also be a lot less code. > > > > > > > > Yes, please. It sounds more sensible to me to dirty the page on get, not > > > > on put. > > > > > > I fully agree this is a desirable final state of affairs. > > > > I'm glad to see this presented because it has crossed my mind more than once > > that effectively a GUP pinned page should be considered "dirty" at all times > > until the pin is removed. This is especially true in the RDMA case. > > But, what if the RDMA registration is readonly? That's not uncommon, and > marking dirty unconditonally would add needless overhead to such pages. Yes and this is only when FOLL_WRITE is set ie when you are doing GUP and asking for write. Doing GUP and asking for read is always safe. Cheers, Jérôme
On 3/19/2019 3:45 PM, Jerome Glisse wrote: > On Tue, Mar 19, 2019 at 03:43:44PM -0500, Tom Talpey wrote: >> On 3/19/2019 4:03 AM, Ira Weiny wrote: >>> On Tue, Mar 19, 2019 at 04:36:44PM +0100, Jan Kara wrote: >>>> On Tue 19-03-19 17:29:18, Kirill A. Shutemov wrote: >>>>> On Tue, Mar 19, 2019 at 10:14:16AM -0400, Jerome Glisse wrote: >>>>>> On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: >>>>>>> On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: >>>>>>>> On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: >>>>>>>>> From: John Hubbard <jhubbard@nvidia.com> >>>>>>> >>>>>>> [...] >>>>>>> >>>>>>>>> diff --git a/mm/gup.c b/mm/gup.c >>>>>>>>> index f84e22685aaa..37085b8163b1 100644 >>>>>>>>> --- a/mm/gup.c >>>>>>>>> +++ b/mm/gup.c >>>>>>>>> @@ -28,6 +28,88 @@ 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]); >>>>>>>>> + >>>>>>>>> + if (!PageDirty(page)) >>>>>>>>> + sdf(page); >>>>>>>> >>>>>>>> How is this safe? What prevents the page to be cleared under you? >>>>>>>> >>>>>>>> If it's safe to race clear_page_dirty*() it has to be stated explicitly >>>>>>>> with a reason why. It's not very clear to me as it is. >>>>>>> >>>>>>> The PageDirty() optimization above is fine to race with clear the >>>>>>> page flag as it means it is racing after a page_mkclean() and the >>>>>>> GUP user is done with the page so page is about to be write back >>>>>>> ie if (!PageDirty(page)) see the page as dirty and skip the sdf() >>>>>>> call while a split second after TestClearPageDirty() happens then >>>>>>> it means the racing clear is about to write back the page so all >>>>>>> is fine (the page was dirty and it is being clear for write back). >>>>>>> >>>>>>> If it does call the sdf() while racing with write back then we >>>>>>> just redirtied the page just like clear_page_dirty_for_io() would >>>>>>> do if page_mkclean() failed so nothing harmful will come of that >>>>>>> neither. Page stays dirty despite write back it just means that >>>>>>> the page might be write back twice in a row. >>>>>> >>>>>> Forgot to mention one thing, we had a discussion with Andrea and Jan >>>>>> about set_page_dirty() and Andrea had the good idea of maybe doing >>>>>> the set_page_dirty() at GUP time (when GUP with write) not when the >>>>>> GUP user calls put_page(). We can do that by setting the dirty bit >>>>>> in the pte for instance. They are few bonus of doing things that way: >>>>>> - amortize the cost of calling set_page_dirty() (ie one call for >>>>>> GUP and page_mkclean() >>>>>> - it is always safe to do so at GUP time (ie the pte has write >>>>>> permission and thus the page is in correct state) >>>>>> - safe from truncate race >>>>>> - no need to ever lock the page >>>>>> >>>>>> Extra bonus from my point of view, it simplify thing for my generic >>>>>> page protection patchset (KSM for file back page). >>>>>> >>>>>> So maybe we should explore that ? It would also be a lot less code. >>>>> >>>>> Yes, please. It sounds more sensible to me to dirty the page on get, not >>>>> on put. >>>> >>>> I fully agree this is a desirable final state of affairs. >>> >>> I'm glad to see this presented because it has crossed my mind more than once >>> that effectively a GUP pinned page should be considered "dirty" at all times >>> until the pin is removed. This is especially true in the RDMA case. >> >> But, what if the RDMA registration is readonly? That's not uncommon, and >> marking dirty unconditonally would add needless overhead to such pages. > > Yes and this is only when FOLL_WRITE is set ie when you are doing GUP and > asking for write. Doing GUP and asking for read is always safe. Aha, ok great. I guess it does introduce something for callers to be aware of, if they GUP very large regions. I suppose if they're sufficiently aware of the situation, e.g. pnfs LAYOUT_COMMIT notifications, they could walk lists and reset page_dirty for untouched pages before releasing. That's their issue though, and agreed it's safest for the GUP layer to mark. Tom.
On Tue, Mar 19, 2019 at 10:14:16AM -0400, Jerome Glisse wrote: > On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: > > On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: > > > On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: > > > > From: John Hubbard <jhubbard@nvidia.com> > > > > [...] > > > > > > diff --git a/mm/gup.c b/mm/gup.c > > > > index f84e22685aaa..37085b8163b1 100644 > > > > --- a/mm/gup.c > > > > +++ b/mm/gup.c > > > > @@ -28,6 +28,88 @@ 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]); > > > > + > > > > + if (!PageDirty(page)) > > > > + sdf(page); > > > > > > How is this safe? What prevents the page to be cleared under you? > > > > > > If it's safe to race clear_page_dirty*() it has to be stated explicitly > > > with a reason why. It's not very clear to me as it is. > > > > The PageDirty() optimization above is fine to race with clear the > > page flag as it means it is racing after a page_mkclean() and the > > GUP user is done with the page so page is about to be write back > > ie if (!PageDirty(page)) see the page as dirty and skip the sdf() > > call while a split second after TestClearPageDirty() happens then > > it means the racing clear is about to write back the page so all > > is fine (the page was dirty and it is being clear for write back). > > > > If it does call the sdf() while racing with write back then we > > just redirtied the page just like clear_page_dirty_for_io() would > > do if page_mkclean() failed so nothing harmful will come of that > > neither. Page stays dirty despite write back it just means that > > the page might be write back twice in a row. > > Forgot to mention one thing, we had a discussion with Andrea and Jan > about set_page_dirty() and Andrea had the good idea of maybe doing > the set_page_dirty() at GUP time (when GUP with write) not when the > GUP user calls put_page(). We can do that by setting the dirty bit > in the pte for instance. They are few bonus of doing things that way: > - amortize the cost of calling set_page_dirty() (ie one call for > GUP and page_mkclean() > - it is always safe to do so at GUP time (ie the pte has write > permission and thus the page is in correct state) > - safe from truncate race > - no need to ever lock the page I seem to have missed this conversation, so please excuse me for asking a stupid question: if it's a file backed page, what prevents background writeback from cleaning the dirty page ~30s into a long term pin? i.e. I don't see anything in this proposal that prevents the page from being cleaned by writeback and putting us straight back into the situation where a long term RDMA is writing to a clean page.... Cheers, Dave.
On Wed, Mar 20, 2019 at 08:23:46AM +1100, Dave Chinner wrote: > On Tue, Mar 19, 2019 at 10:14:16AM -0400, Jerome Glisse wrote: > > On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: > > > On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: > > > > On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: > > > > > From: John Hubbard <jhubbard@nvidia.com> > > > > > > [...] > > > > > > > > diff --git a/mm/gup.c b/mm/gup.c > > > > > index f84e22685aaa..37085b8163b1 100644 > > > > > --- a/mm/gup.c > > > > > +++ b/mm/gup.c > > > > > @@ -28,6 +28,88 @@ 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]); > > > > > + > > > > > + if (!PageDirty(page)) > > > > > + sdf(page); > > > > > > > > How is this safe? What prevents the page to be cleared under you? > > > > > > > > If it's safe to race clear_page_dirty*() it has to be stated explicitly > > > > with a reason why. It's not very clear to me as it is. > > > > > > The PageDirty() optimization above is fine to race with clear the > > > page flag as it means it is racing after a page_mkclean() and the > > > GUP user is done with the page so page is about to be write back > > > ie if (!PageDirty(page)) see the page as dirty and skip the sdf() > > > call while a split second after TestClearPageDirty() happens then > > > it means the racing clear is about to write back the page so all > > > is fine (the page was dirty and it is being clear for write back). > > > > > > If it does call the sdf() while racing with write back then we > > > just redirtied the page just like clear_page_dirty_for_io() would > > > do if page_mkclean() failed so nothing harmful will come of that > > > neither. Page stays dirty despite write back it just means that > > > the page might be write back twice in a row. > > > > Forgot to mention one thing, we had a discussion with Andrea and Jan > > about set_page_dirty() and Andrea had the good idea of maybe doing > > the set_page_dirty() at GUP time (when GUP with write) not when the > > GUP user calls put_page(). We can do that by setting the dirty bit > > in the pte for instance. They are few bonus of doing things that way: > > - amortize the cost of calling set_page_dirty() (ie one call for > > GUP and page_mkclean() > > - it is always safe to do so at GUP time (ie the pte has write > > permission and thus the page is in correct state) > > - safe from truncate race > > - no need to ever lock the page > > I seem to have missed this conversation, so please excuse me for The set_page_dirty() at GUP was in a private discussion (it started on another topic and drifted away to set_page_dirty()). > asking a stupid question: if it's a file backed page, what prevents > background writeback from cleaning the dirty page ~30s into a long > term pin? i.e. I don't see anything in this proposal that prevents > the page from being cleaned by writeback and putting us straight > back into the situation where a long term RDMA is writing to a clean > page.... So this patchset does not solve this issue. The plan is multi-step (with different patchset for each steps): [1] convert all places that do gup() and then put_page() to use gup_put_page() instead. This is what this present patchset is about. [2] use bias pin count so that we can identify GUPed page from non GUPed page. So instead of incrementing page refcount by 1 on GUP we increment it by GUP_BIAS and in gup_put_page() we decrement it by GUP_BIAS. This means that page with a refcount > GUP_BIAS can be considered as GUP (more on false positive below) [3..N] decide what to do for GUPed page, so far the plans seems to be to keep the page always dirty and never allow page write back to restore the page in a clean state. This does disable thing like COW and other fs feature but at least it seems to be the best thing we can do. For race between clear_page_for_io() and GUP this was extensively discuss and IIRC you were on that thread. Basicly we can only race with page_mkclean() (as GUP can only succeed if there is a pte with write permission) and page cleaning happens after page_mkclean() and they are barrier between page_mkclean() and what's after. Hence we will be able to see the page as GUPed before cleaning it without any race. For false positive i think we agreed that it is something we can live with. It could only happen to page that are share more than GUP_BIAS times, it should be rare enough and false positive means you get the same treatement as a GUPed page. Cheers, Jérôme
On Tue, Mar 19, 2019 at 06:06:55PM -0400, Jerome Glisse wrote: > On Wed, Mar 20, 2019 at 08:23:46AM +1100, Dave Chinner wrote: > > On Tue, Mar 19, 2019 at 10:14:16AM -0400, Jerome Glisse wrote: > > > On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: > > > > On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: > > > > > On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: > > > > > > From: John Hubbard <jhubbard@nvidia.com> > > > > > > > > [...] > > > > > > > > > > diff --git a/mm/gup.c b/mm/gup.c > > > > > > index f84e22685aaa..37085b8163b1 100644 > > > > > > --- a/mm/gup.c > > > > > > +++ b/mm/gup.c > > > > > > @@ -28,6 +28,88 @@ 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]); > > > > > > + > > > > > > + if (!PageDirty(page)) > > > > > > + sdf(page); > > > > > > > > > > How is this safe? What prevents the page to be cleared under you? > > > > > > > > > > If it's safe to race clear_page_dirty*() it has to be stated explicitly > > > > > with a reason why. It's not very clear to me as it is. > > > > > > > > The PageDirty() optimization above is fine to race with clear the > > > > page flag as it means it is racing after a page_mkclean() and the > > > > GUP user is done with the page so page is about to be write back > > > > ie if (!PageDirty(page)) see the page as dirty and skip the sdf() > > > > call while a split second after TestClearPageDirty() happens then > > > > it means the racing clear is about to write back the page so all > > > > is fine (the page was dirty and it is being clear for write back). > > > > > > > > If it does call the sdf() while racing with write back then we > > > > just redirtied the page just like clear_page_dirty_for_io() would > > > > do if page_mkclean() failed so nothing harmful will come of that > > > > neither. Page stays dirty despite write back it just means that > > > > the page might be write back twice in a row. > > > > > > Forgot to mention one thing, we had a discussion with Andrea and Jan > > > about set_page_dirty() and Andrea had the good idea of maybe doing > > > the set_page_dirty() at GUP time (when GUP with write) not when the > > > GUP user calls put_page(). We can do that by setting the dirty bit > > > in the pte for instance. They are few bonus of doing things that way: > > > - amortize the cost of calling set_page_dirty() (ie one call for > > > GUP and page_mkclean() > > > - it is always safe to do so at GUP time (ie the pte has write > > > permission and thus the page is in correct state) > > > - safe from truncate race > > > - no need to ever lock the page > > > > I seem to have missed this conversation, so please excuse me for > > The set_page_dirty() at GUP was in a private discussion (it started > on another topic and drifted away to set_page_dirty()). > > > asking a stupid question: if it's a file backed page, what prevents > > background writeback from cleaning the dirty page ~30s into a long > > term pin? i.e. I don't see anything in this proposal that prevents > > the page from being cleaned by writeback and putting us straight > > back into the situation where a long term RDMA is writing to a clean > > page.... > > So this patchset does not solve this issue. OK, so it just kicks the can further down the road. > [3..N] decide what to do for GUPed page, so far the plans seems > to be to keep the page always dirty and never allow page > write back to restore the page in a clean state. This does > disable thing like COW and other fs feature but at least > it seems to be the best thing we can do. So the plan for GUP vs writeback so far is "break fsync()"? :) We might need to work on that a bit more... Cheers, Dave.
On Wed, Mar 20, 2019 at 10:57:52AM +1100, Dave Chinner wrote: > On Tue, Mar 19, 2019 at 06:06:55PM -0400, Jerome Glisse wrote: > > On Wed, Mar 20, 2019 at 08:23:46AM +1100, Dave Chinner wrote: > > > On Tue, Mar 19, 2019 at 10:14:16AM -0400, Jerome Glisse wrote: > > > > On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: > > > > > On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: > > > > > > On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: > > > > > > > From: John Hubbard <jhubbard@nvidia.com> > > > > > > > > > > [...] > > > > > > > > > > > > diff --git a/mm/gup.c b/mm/gup.c > > > > > > > index f84e22685aaa..37085b8163b1 100644 > > > > > > > --- a/mm/gup.c > > > > > > > +++ b/mm/gup.c > > > > > > > @@ -28,6 +28,88 @@ 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]); > > > > > > > + > > > > > > > + if (!PageDirty(page)) > > > > > > > + sdf(page); > > > > > > > > > > > > How is this safe? What prevents the page to be cleared under you? > > > > > > > > > > > > If it's safe to race clear_page_dirty*() it has to be stated explicitly > > > > > > with a reason why. It's not very clear to me as it is. > > > > > > > > > > The PageDirty() optimization above is fine to race with clear the > > > > > page flag as it means it is racing after a page_mkclean() and the > > > > > GUP user is done with the page so page is about to be write back > > > > > ie if (!PageDirty(page)) see the page as dirty and skip the sdf() > > > > > call while a split second after TestClearPageDirty() happens then > > > > > it means the racing clear is about to write back the page so all > > > > > is fine (the page was dirty and it is being clear for write back). > > > > > > > > > > If it does call the sdf() while racing with write back then we > > > > > just redirtied the page just like clear_page_dirty_for_io() would > > > > > do if page_mkclean() failed so nothing harmful will come of that > > > > > neither. Page stays dirty despite write back it just means that > > > > > the page might be write back twice in a row. > > > > > > > > Forgot to mention one thing, we had a discussion with Andrea and Jan > > > > about set_page_dirty() and Andrea had the good idea of maybe doing > > > > the set_page_dirty() at GUP time (when GUP with write) not when the > > > > GUP user calls put_page(). We can do that by setting the dirty bit > > > > in the pte for instance. They are few bonus of doing things that way: > > > > - amortize the cost of calling set_page_dirty() (ie one call for > > > > GUP and page_mkclean() > > > > - it is always safe to do so at GUP time (ie the pte has write > > > > permission and thus the page is in correct state) > > > > - safe from truncate race > > > > - no need to ever lock the page > > > > > > I seem to have missed this conversation, so please excuse me for > > > > The set_page_dirty() at GUP was in a private discussion (it started > > on another topic and drifted away to set_page_dirty()). > > > > > asking a stupid question: if it's a file backed page, what prevents > > > background writeback from cleaning the dirty page ~30s into a long > > > term pin? i.e. I don't see anything in this proposal that prevents > > > the page from being cleaned by writeback and putting us straight > > > back into the situation where a long term RDMA is writing to a clean > > > page.... > > > > So this patchset does not solve this issue. > > OK, so it just kicks the can further down the road. > > > [3..N] decide what to do for GUPed page, so far the plans seems > > to be to keep the page always dirty and never allow page > > write back to restore the page in a clean state. This does > > disable thing like COW and other fs feature but at least > > it seems to be the best thing we can do. > > So the plan for GUP vs writeback so far is "break fsync()"? :) > > We might need to work on that a bit more... Sorry forgot to say that we still do write back using a bounce page so that at least we write something to disk that is just a snapshot of the GUPed page everytime writeback kicks in (so either through radix tree dirty page write back or fsync or any other sync events). So many little details that i forgot the big chunk :) Cheers, Jérôme
On 3/19/19 4:57 PM, Dave Chinner wrote: > On Tue, Mar 19, 2019 at 06:06:55PM -0400, Jerome Glisse wrote: >> On Wed, Mar 20, 2019 at 08:23:46AM +1100, Dave Chinner wrote: >>> On Tue, Mar 19, 2019 at 10:14:16AM -0400, Jerome Glisse wrote: >>>> On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: >>>>> On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: >>>>>> On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: >>>>>>> From: John Hubbard <jhubbard@nvidia.com> >>>>> [...] >>>> Forgot to mention one thing, we had a discussion with Andrea and Jan >>>> about set_page_dirty() and Andrea had the good idea of maybe doing >>>> the set_page_dirty() at GUP time (when GUP with write) not when the >>>> GUP user calls put_page(). We can do that by setting the dirty bit >>>> in the pte for instance. They are few bonus of doing things that way: >>>> - amortize the cost of calling set_page_dirty() (ie one call for >>>> GUP and page_mkclean() >>>> - it is always safe to do so at GUP time (ie the pte has write >>>> permission and thus the page is in correct state) >>>> - safe from truncate race >>>> - no need to ever lock the page >>> >>> I seem to have missed this conversation, so please excuse me for >> >> The set_page_dirty() at GUP was in a private discussion (it started >> on another topic and drifted away to set_page_dirty()). >> >>> asking a stupid question: if it's a file backed page, what prevents >>> background writeback from cleaning the dirty page ~30s into a long >>> term pin? i.e. I don't see anything in this proposal that prevents >>> the page from being cleaned by writeback and putting us straight >>> back into the situation where a long term RDMA is writing to a clean >>> page.... >> >> So this patchset does not solve this issue. > > OK, so it just kicks the can further down the road. Hi Dave, My take on this is that all of the viable solution proposals so far require tracking of gup-pinned pages. That's why I'm trying to get started now on at least the tracking aspects: it seems like the tracking part is now well understood. And it does have some lead time, because I expect the call site conversions probably have to go through various maintainers' trees. However, if you are thinking that this is unwise, and that's it's smarter to wait until the entire design is completely worked out, I'm open to that, too. Thoughts? thanks,
On Wed, 20 Mar 2019, Dave Chinner wrote:
> So the plan for GUP vs writeback so far is "break fsync()"? :)
Well if its an anonymous page and not a file backed page then the
semantics are preserved. Disallow GUP long term pinning (marking stuff like in this
patchset may make that possible) and its clean.
On 3/19/19 5:08 PM, Jerome Glisse wrote: > On Wed, Mar 20, 2019 at 10:57:52AM +1100, Dave Chinner wrote: >> On Tue, Mar 19, 2019 at 06:06:55PM -0400, Jerome Glisse wrote: >>> On Wed, Mar 20, 2019 at 08:23:46AM +1100, Dave Chinner wrote: >>>> On Tue, Mar 19, 2019 at 10:14:16AM -0400, Jerome Glisse wrote: >>>>> On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: >>>>>> On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: >>>>>>> On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: >>>>>>>> From: John Hubbard <jhubbard@nvidia.com> >>>>>> [...] >>>>> Forgot to mention one thing, we had a discussion with Andrea and Jan >>>>> about set_page_dirty() and Andrea had the good idea of maybe doing >>>>> the set_page_dirty() at GUP time (when GUP with write) not when the >>>>> GUP user calls put_page(). We can do that by setting the dirty bit >>>>> in the pte for instance. They are few bonus of doing things that way: >>>>> - amortize the cost of calling set_page_dirty() (ie one call for >>>>> GUP and page_mkclean() >>>>> - it is always safe to do so at GUP time (ie the pte has write >>>>> permission and thus the page is in correct state) >>>>> - safe from truncate race >>>>> - no need to ever lock the page >>>> >>>> I seem to have missed this conversation, so please excuse me for >>> >>> The set_page_dirty() at GUP was in a private discussion (it started >>> on another topic and drifted away to set_page_dirty()). >>> >>>> asking a stupid question: if it's a file backed page, what prevents >>>> background writeback from cleaning the dirty page ~30s into a long >>>> term pin? i.e. I don't see anything in this proposal that prevents >>>> the page from being cleaned by writeback and putting us straight >>>> back into the situation where a long term RDMA is writing to a clean >>>> page.... >>> >>> So this patchset does not solve this issue. >> >> OK, so it just kicks the can further down the road. >> >>> [3..N] decide what to do for GUPed page, so far the plans seems >>> to be to keep the page always dirty and never allow page >>> write back to restore the page in a clean state. This does >>> disable thing like COW and other fs feature but at least >>> it seems to be the best thing we can do. >> >> So the plan for GUP vs writeback so far is "break fsync()"? :) >> >> We might need to work on that a bit more... > > Sorry forgot to say that we still do write back using a bounce page > so that at least we write something to disk that is just a snapshot > of the GUPed page everytime writeback kicks in (so either through > radix tree dirty page write back or fsync or any other sync events). > So many little details that i forgot the big chunk :) > > Cheers, > Jérôme > Dave, Jan, Jerome, Bounce pages for periodic data integrity still seem viable. But for the question of things like fsync or truncate, I think we were zeroing in on file leases as a nice building block. Can we revive the file lease discussion? By going all the way out to user space and requiring file leases to be coordinated at a high level in the software call chain, it seems like we could routinely avoid some of the worst conflicts that the kernel code has to resolve. For example: Process A ========= gets a lease on file_a that allows gup usage on a range within file_a sets up writable DMA: get_user_pages() on the file_a range start DMA (independent hardware ops) hw is reading and writing to range Process B ========= truncate(file_a) ... __break_lease() handle SIGIO from __break_lease if unhandled, process gets killed and put_user_pages should get called at some point here ...and so this way, user space gets to decide the proper behavior, instead of leaving the kernel in the dark with an impossible decision (kill process A? Block process B? User space knows the preference, per app, but kernel does not.) thanks,
On Tue, Mar 19, 2019 at 06:43:45PM -0700, John Hubbard wrote: > On 3/19/19 5:08 PM, Jerome Glisse wrote: > > On Wed, Mar 20, 2019 at 10:57:52AM +1100, Dave Chinner wrote: > >> On Tue, Mar 19, 2019 at 06:06:55PM -0400, Jerome Glisse wrote: > >>> On Wed, Mar 20, 2019 at 08:23:46AM +1100, Dave Chinner wrote: > >>>> On Tue, Mar 19, 2019 at 10:14:16AM -0400, Jerome Glisse wrote: > >>>>> On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: > >>>>>> On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: > >>>>>>> On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: > >>>>>>>> From: John Hubbard <jhubbard@nvidia.com> > >>>>>> [...] > >>>>> Forgot to mention one thing, we had a discussion with Andrea and Jan > >>>>> about set_page_dirty() and Andrea had the good idea of maybe doing > >>>>> the set_page_dirty() at GUP time (when GUP with write) not when the > >>>>> GUP user calls put_page(). We can do that by setting the dirty bit > >>>>> in the pte for instance. They are few bonus of doing things that way: > >>>>> - amortize the cost of calling set_page_dirty() (ie one call for > >>>>> GUP and page_mkclean() > >>>>> - it is always safe to do so at GUP time (ie the pte has write > >>>>> permission and thus the page is in correct state) > >>>>> - safe from truncate race > >>>>> - no need to ever lock the page > >>>> > >>>> I seem to have missed this conversation, so please excuse me for > >>> > >>> The set_page_dirty() at GUP was in a private discussion (it started > >>> on another topic and drifted away to set_page_dirty()). > >>> > >>>> asking a stupid question: if it's a file backed page, what prevents > >>>> background writeback from cleaning the dirty page ~30s into a long > >>>> term pin? i.e. I don't see anything in this proposal that prevents > >>>> the page from being cleaned by writeback and putting us straight > >>>> back into the situation where a long term RDMA is writing to a clean > >>>> page.... > >>> > >>> So this patchset does not solve this issue. > >> > >> OK, so it just kicks the can further down the road. > >> > >>> [3..N] decide what to do for GUPed page, so far the plans seems > >>> to be to keep the page always dirty and never allow page > >>> write back to restore the page in a clean state. This does > >>> disable thing like COW and other fs feature but at least > >>> it seems to be the best thing we can do. > >> > >> So the plan for GUP vs writeback so far is "break fsync()"? :) > >> > >> We might need to work on that a bit more... > > > > Sorry forgot to say that we still do write back using a bounce page > > so that at least we write something to disk that is just a snapshot > > of the GUPed page everytime writeback kicks in (so either through > > radix tree dirty page write back or fsync or any other sync events). > > So many little details that i forgot the big chunk :) > > > > Cheers, > > Jérôme > > > > Dave, Jan, Jerome, > > Bounce pages for periodic data integrity still seem viable. But for the > question of things like fsync or truncate, I think we were zeroing in > on file leases as a nice building block. > > Can we revive the file lease discussion? By going all the way out to user > space and requiring file leases to be coordinated at a high level in the > software call chain, it seems like we could routinely avoid some of the > worst conflicts that the kernel code has to resolve. > > For example: > > Process A > ========= > gets a lease on file_a that allows gup > usage on a range within file_a > > sets up writable DMA: > get_user_pages() on the file_a range > start DMA (independent hardware ops) > hw is reading and writing to range > > Process B > ========= > truncate(file_a) > ... > __break_lease() > > handle SIGIO from __break_lease > if unhandled, process gets killed > and put_user_pages should get called > at some point here > > ...and so this way, user space gets to decide the proper behavior, > instead of leaving the kernel in the dark with an impossible decision > (kill process A? Block process B? User space knows the preference, > per app, but kernel does not.) There is no need to kill anything here ... if truncate happens then the GUP user is just GUPing page that do not correspond to anything anymore. This is the current behavior and it is what GUP always has been. By the time you get the page from GUP there is no garantee that they correspond to anything. If a device really want to mirror process address faithfully then the hardware need to make little effort either have something like ATS/ PASID or be able to abide mmu notifier. If we start blocking existing syscall just because someone is doing a GUP we are opening a pandora box. It is not just truncate, it is a whole range of syscall that deals with either file or virtual address. The semantic of GUP is really the semantic of direct I/O and the virtual address you are direct I/O-ing to/from and the rule there is: do not do anything stupid to those virtual addresses while you are doing direct I/O with them (no munmap, mremap, madvise, truncate, ...). Same logic apply to file, when two process do thing to same file there the kernel never get in the way of one process doing something the other process did not expect. For instance one process mmaping the file the other process truncating the file, if the first process try to access the file through the mmap after the truncation it will get a sigbus. So i believe best we could do is send a SIGBUS to the process that has GUPed a range of a file that is being truncated this would match what we do for CPU acces. There is no reason access through GUP should be handled any differently. Cheers, Jérôme
On Wed, Mar 20, 2019 at 12:33:20AM -0400, Jerome Glisse wrote: > On Tue, Mar 19, 2019 at 06:43:45PM -0700, John Hubbard wrote: > > On 3/19/19 5:08 PM, Jerome Glisse wrote: > > > On Wed, Mar 20, 2019 at 10:57:52AM +1100, Dave Chinner wrote: > > >> On Tue, Mar 19, 2019 at 06:06:55PM -0400, Jerome Glisse wrote: > > >>> On Wed, Mar 20, 2019 at 08:23:46AM +1100, Dave Chinner wrote: > > >>>> On Tue, Mar 19, 2019 at 10:14:16AM -0400, Jerome Glisse wrote: > > >>>>> On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: > > >>>>>> On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: > > >>>>>>> On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: > > >>>>>>>> From: John Hubbard <jhubbard@nvidia.com> > > >>>>>> [...] > > >>>>> Forgot to mention one thing, we had a discussion with Andrea and Jan > > >>>>> about set_page_dirty() and Andrea had the good idea of maybe doing > > >>>>> the set_page_dirty() at GUP time (when GUP with write) not when the > > >>>>> GUP user calls put_page(). We can do that by setting the dirty bit > > >>>>> in the pte for instance. They are few bonus of doing things that way: > > >>>>> - amortize the cost of calling set_page_dirty() (ie one call for > > >>>>> GUP and page_mkclean() > > >>>>> - it is always safe to do so at GUP time (ie the pte has write > > >>>>> permission and thus the page is in correct state) > > >>>>> - safe from truncate race > > >>>>> - no need to ever lock the page > > >>>> > > >>>> I seem to have missed this conversation, so please excuse me for > > >>> > > >>> The set_page_dirty() at GUP was in a private discussion (it started > > >>> on another topic and drifted away to set_page_dirty()). > > >>> > > >>>> asking a stupid question: if it's a file backed page, what prevents > > >>>> background writeback from cleaning the dirty page ~30s into a long > > >>>> term pin? i.e. I don't see anything in this proposal that prevents > > >>>> the page from being cleaned by writeback and putting us straight > > >>>> back into the situation where a long term RDMA is writing to a clean > > >>>> page.... > > >>> > > >>> So this patchset does not solve this issue. > > >> > > >> OK, so it just kicks the can further down the road. > > >> > > >>> [3..N] decide what to do for GUPed page, so far the plans seems > > >>> to be to keep the page always dirty and never allow page > > >>> write back to restore the page in a clean state. This does > > >>> disable thing like COW and other fs feature but at least > > >>> it seems to be the best thing we can do. > > >> > > >> So the plan for GUP vs writeback so far is "break fsync()"? :) > > >> > > >> We might need to work on that a bit more... > > > > > > Sorry forgot to say that we still do write back using a bounce page > > > so that at least we write something to disk that is just a snapshot > > > of the GUPed page everytime writeback kicks in (so either through > > > radix tree dirty page write back or fsync or any other sync events). > > > So many little details that i forgot the big chunk :) > > > > > > Cheers, > > > Jérôme > > > > > > > Dave, Jan, Jerome, > > > > Bounce pages for periodic data integrity still seem viable. But for the > > question of things like fsync or truncate, I think we were zeroing in > > on file leases as a nice building block. > > > > Can we revive the file lease discussion? By going all the way out to user > > space and requiring file leases to be coordinated at a high level in the > > software call chain, it seems like we could routinely avoid some of the > > worst conflicts that the kernel code has to resolve. > > > > For example: > > > > Process A > > ========= > > gets a lease on file_a that allows gup > > usage on a range within file_a > > > > sets up writable DMA: > > get_user_pages() on the file_a range > > start DMA (independent hardware ops) > > hw is reading and writing to range > > > > Process B > > ========= > > truncate(file_a) > > ... > > __break_lease() > > > > handle SIGIO from __break_lease > > if unhandled, process gets killed > > and put_user_pages should get called > > at some point here > > > > ...and so this way, user space gets to decide the proper behavior, > > instead of leaving the kernel in the dark with an impossible decision > > (kill process A? Block process B? User space knows the preference, > > per app, but kernel does not.) > > There is no need to kill anything here ... if truncate happens then > the GUP user is just GUPing page that do not correspond to anything > anymore. This is the current behavior and it is what GUP always has > been. By the time you get the page from GUP there is no garantee that > they correspond to anything. > > If a device really want to mirror process address faithfully then the > hardware need to make little effort either have something like ATS/ > PASID or be able to abide mmu notifier. > > If we start blocking existing syscall just because someone is doing a > GUP we are opening a pandora box. It is not just truncate, it is a > whole range of syscall that deals with either file or virtual address. > > The semantic of GUP is really the semantic of direct I/O and the > virtual address you are direct I/O-ing to/from and the rule there is: > do not do anything stupid to those virtual addresses while you are > doing direct I/O with them (no munmap, mremap, madvise, truncate, ...). > > > Same logic apply to file, when two process do thing to same file there > the kernel never get in the way of one process doing something the > other process did not expect. For instance one process mmaping the file > the other process truncating the file, if the first process try to access > the file through the mmap after the truncation it will get a sigbus. > > So i believe best we could do is send a SIGBUS to the process that has > GUPed a range of a file that is being truncated this would match what > we do for CPU acces. There is no reason access through GUP should be > handled any differently. I agree in sending SIGBUS but the fact is most "Process A"'s will not be handling SIGBUS and will then result in that process dying. Ira > > Cheers, > Jérôme >
On Tue, Mar 19, 2019 at 01:01:01PM -0700, John Hubbard wrote: > On 3/19/19 7:06 AM, Kirill A. Shutemov wrote: > > On Tue, Mar 19, 2019 at 09:47:24AM -0400, Jerome Glisse wrote: > >> On Tue, Mar 19, 2019 at 03:04:17PM +0300, Kirill A. Shutemov wrote: > >>> On Fri, Mar 08, 2019 at 01:36:33PM -0800, john.hubbard@gmail.com wrote: > >>>> From: John Hubbard <jhubbard@nvidia.com> > >> > >> [...] > >> > >>>> diff --git a/mm/gup.c b/mm/gup.c > >>>> index f84e22685aaa..37085b8163b1 100644 > >>>> --- a/mm/gup.c > >>>> +++ b/mm/gup.c > >>>> @@ -28,6 +28,88 @@ 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]); > >>>> + > >>>> + if (!PageDirty(page)) > >>>> + sdf(page); > >>> > >>> How is this safe? What prevents the page to be cleared under you? > >>> > >>> If it's safe to race clear_page_dirty*() it has to be stated explicitly > >>> with a reason why. It's not very clear to me as it is. > >> > >> The PageDirty() optimization above is fine to race with clear the > >> page flag as it means it is racing after a page_mkclean() and the > >> GUP user is done with the page so page is about to be write back > >> ie if (!PageDirty(page)) see the page as dirty and skip the sdf() > >> call while a split second after TestClearPageDirty() happens then > >> it means the racing clear is about to write back the page so all > >> is fine (the page was dirty and it is being clear for write back). > >> > >> If it does call the sdf() while racing with write back then we > >> just redirtied the page just like clear_page_dirty_for_io() would > >> do if page_mkclean() failed so nothing harmful will come of that > >> neither. Page stays dirty despite write back it just means that > >> the page might be write back twice in a row. > > > > Fair enough. Should we get it into a comment here? > > How's this read to you? I reworded and slightly expanded Jerome's > description: > > diff --git a/mm/gup.c b/mm/gup.c > index d1df7b8ba973..86397ae23922 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -61,6 +61,24 @@ static void __put_user_pages_dirty(struct page **pages, > 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); Looks good to me. Other nit: effectively the same type of callback called 'spd' in set_page_dirty(). Should we rename 'sdf' to 'sdp' here too? > >>>> +void put_user_pages(struct page **pages, unsigned long npages) > >>>> +{ > >>>> + unsigned long index; > >>>> + > >>>> + for (index = 0; index < npages; index++) > >>>> + put_user_page(pages[index]); > >>> > >>> I believe there's an room for improvement for compound pages. > >>> > >>> If there's multiple consequential pages in the array that belong to the > >>> same compound page we can get away with a single atomic operation to > >>> handle them all. > >> > >> Yes maybe just add a comment with that for now and leave this kind of > >> optimization to latter ? > > > > Sounds good to me. > > > > Here's a comment for that: > > @@ -127,6 +145,11 @@ 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 Comound pages are always physically contiguous. I initially ment that the optimization makes sense if they are next to each other in 'pages' array. > + * single operation to the head page should suffice. > + */ > for (index = 0; index < npages; index++) > put_user_page(pages[index]); > } > > > thanks, > -- > John Hubbard > NVIDIA
On Tue, Mar 19, 2019 at 12:24:00PM -0700, John Hubbard wrote: > So, I could be persuaded either way. But given the lack of an visible perf > effects, and given that this could will get removed anyway because we'll > likely end up with set_page_dirty() called at GUP time instead...it seems > like it's probably OK to just leave it as is. Apart from ugly code generated, other argument might be Spectre-like attacks on these call. I would rather avoid indirect function calls whenever possible. And I don't think opencodded versions of these functions would look much worse.
> On Mar 19, 2019, at 10:33 PM, Jerome Glisse <jglisse@redhat.com> wrote: > > So i believe best we could do is send a SIGBUS to the process that has > GUPed a range of a file that is being truncated this would match what > we do for CPU acces. There is no reason access through GUP should be > handled any differently. This should be done lazily, as there's no need to send the SIGBUS unless the GUPed page is actually accessed post-truncate.
On Wed, Mar 20, 2019 at 08:55:17AM -0600, William Kucharski wrote: > > > > On Mar 19, 2019, at 10:33 PM, Jerome Glisse <jglisse@redhat.com> wrote: > > > > So i believe best we could do is send a SIGBUS to the process that has > > GUPed a range of a file that is being truncated this would match what > > we do for CPU acces. There is no reason access through GUP should be > > handled any differently. > > This should be done lazily, as there's no need to send the SIGBUS unless > the GUPed page is actually accessed post-truncate. Issue is that unlike CPU access we might not be able to detect device access and thus it is not something we can do lazily for everyone. Cheers, Jérôme
diff --git a/include/linux/mm.h b/include/linux/mm.h index 5801ee849f36..353035c8b115 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -993,6 +993,30 @@ 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_pages_dirty(struct page **pages, unsigned long npages); +void put_user_pages_dirty_lock(struct page **pages, unsigned long npages); +void put_user_pages(struct page **pages, unsigned long npages); + #if defined(CONFIG_SPARSEMEM) && !defined(CONFIG_SPARSEMEM_VMEMMAP) #define SECTION_IN_PAGE_FLAGS #endif diff --git a/mm/gup.c b/mm/gup.c index f84e22685aaa..37085b8163b1 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -28,6 +28,88 @@ 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]); + + 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. + * @npages: number of pages in the @pages array. + * + * "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(). + * + * 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. + * + */ +void put_user_pages_dirty(struct page **pages, unsigned long npages) +{ + __put_user_pages_dirty(pages, npages, set_page_dirty); +} +EXPORT_SYMBOL(put_user_pages_dirty); + +/** + * 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); +} +EXPORT_SYMBOL(put_user_pages_dirty_lock); + +/** + * put_user_pages() - release 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, release the page using put_user_page(). + * + * Please see the put_user_page() documentation for details. + */ +void put_user_pages(struct page **pages, unsigned long npages) +{ + unsigned long index; + + for (index = 0; index < npages; index++) + put_user_page(pages[index]); +} +EXPORT_SYMBOL(put_user_pages); + static struct page *no_page_table(struct vm_area_struct *vma, unsigned int flags) {