On Mon, 8 Oct 2018 14:16:22 -0700 john.hubbard@xxxxxxxxx wrote: > From: John Hubbard <jhubbard@xxxxxxxxxx> > > 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 prepares for eventually fixing the problem described > in [1], and is following a plan listed in [2], [3], [4]. > > [1] https://lwn.net/Articles/753027/ : "The Trouble with get_user_pages()" > > [2] https://lkml.kernel.org/r/20180709080554.21931-1-jhubbard@xxxxxxxxxx > Proposed steps for fixing get_user_pages() + DMA problems. > > [3]https://lkml.kernel.org/r/20180710082100.mkdwngdv5kkrcz6n@xxxxxxxxxxxxxx > Bounce buffers (otherwise [2] is not really viable). > > [4] https://lkml.kernel.org/r/20181003162115.GG24030@xxxxxxxxxxxxxx > Follow-up discussions. > > ... > > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -137,6 +137,8 @@ extern int overcommit_ratio_handler(struct ctl_table *, int, void __user *, > size_t *, loff_t *); > extern int overcommit_kbytes_handler(struct ctl_table *, int, void __user *, > size_t *, loff_t *); > +int set_page_dirty(struct page *page); > +int set_page_dirty_lock(struct page *page); > > #define nth_page(page,n) pfn_to_page(page_to_pfn((page)) + (n)) > > @@ -943,6 +945,51 @@ static inline void put_page(struct page *page) > __put_page(page); > } > > +/* > + * Pages that were pinned via get_user_pages*() should be released via > + * either put_user_page(), or one of the put_user_pages*() routines > + * below. > + */ > +static inline void put_user_page(struct page *page) > +{ > + put_page(page); > +} > + > +static inline void put_user_pages_dirty(struct page **pages, > + unsigned long npages) > +{ > + unsigned long index; > + > + for (index = 0; index < npages; index++) { > + if (!PageDirty(pages[index])) Both put_page() and set_page_dirty() handle compound pages. But because of the above statement, put_user_pages_dirty() might misbehave? Or maybe it won't - perhaps the intent here is to skip dirtying the head page if the sub page is clean? Please clarify, explain and add comment if so. > + set_page_dirty(pages[index]); > + > + put_user_page(pages[index]); > + } > +} > + > +static inline void put_user_pages_dirty_lock(struct page **pages, > + unsigned long npages) > +{ > + unsigned long index; > + > + for (index = 0; index < npages; index++) { > + if (!PageDirty(pages[index])) > + set_page_dirty_lock(pages[index]); Ditto. > + put_user_page(pages[index]); > + } > +} > + > +static inline void put_user_pages(struct page **pages, > + unsigned long npages) > +{ > + unsigned long index; > + > + for (index = 0; index < npages; index++) > + put_user_page(pages[index]); > +} > + Otherwise looks OK. Ish. But it would be nice if that comment were to explain *why* get_user_pages() pages must be released with put_user_page(). Also, maintainability. What happens if someone now uses put_page() by mistake? Kernel fails in some mysterious fashion? How can we prevent this from occurring as code evolves? Is there a cheap way of detecting this bug at runtime?
