| Message ID | 20201124060755.1405602-2-ira.weiny@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kmap: Create mem*_page interfaces | expand |
On Mon, Nov 23, 2020 at 10:07:39PM -0800, ira.weiny@intel.com wrote: > +static inline void memzero_page(struct page *page, size_t offset, size_t len) > +{ > + memset_page(page, 0, offset, len); > +} This is a less-capable zero_user_segments().
On Tue, Nov 24, 2020 at 02:19:41PM +0000, Matthew Wilcox wrote: > On Mon, Nov 23, 2020 at 10:07:39PM -0800, ira.weiny@intel.com wrote: > > +static inline void memzero_page(struct page *page, size_t offset, size_t len) > > +{ > > + memset_page(page, 0, offset, len); > > +} > > This is a less-capable zero_user_segments(). Actually it is a duplicate of zero_user()... Sorry I did not notice those... :-( Why are they called '_user_'? Ira
On Tue, Nov 24, 2020 at 11:21:13AM -0800, Ira Weiny wrote: > On Tue, Nov 24, 2020 at 02:19:41PM +0000, Matthew Wilcox wrote: > > On Mon, Nov 23, 2020 at 10:07:39PM -0800, ira.weiny@intel.com wrote: > > > +static inline void memzero_page(struct page *page, size_t offset, size_t len) > > > +{ > > > + memset_page(page, 0, offset, len); > > > +} > > > > This is a less-capable zero_user_segments(). > > Actually it is a duplicate of zero_user()... Sorry I did not notice those... > :-( > > Why are they called '_user_'? git knows ... commit 01f2705daf5a36208e69d7cf95db9c330f843af6 Author: Nate Diller <nate.diller@gmail.com> Date: Wed May 9 02:35:07 2007 -0700 fs: convert core functions to zero_user_page It's very common for file systems to need to zero part or all of a page, the simplist way is just to use kmap_atomic() and memset(). There's actually a library function in include/linux/highmem.h that does exactly that, but it's confusingly named memclear_highpage_flush(), which is descriptive of *how* it does the work rather than what the *purpose* is. So this patchset renames the function to zero_user_page(), and calls it from the various places that currently open code it. This first patch introduces the new function call, and converts all the core kernel callsites, both the open-coded ones and the old memclear_highpage_flush() ones. Following this patch is a series of conversions for each file system individually, per AKPM, and finally a patch deprecating the old call. The diffstat below shows the entire patchset.
On Fri, Nov 27, 2020 at 03:06:24PM +0200, Joonas Lahtinen wrote: > Quoting ira.weiny@intel.com (2020-11-24 08:07:39) > > From: Ira Weiny <ira.weiny@intel.com> > > > > Working through a conversion to a call such as kmap_thread() revealed > > many places where the pattern kmap/memcpy/kunmap occurred. > > > > Eric Biggers, Matthew Wilcox, Christoph Hellwig, Dan Williams, and Al > > Viro all suggested putting this code into helper functions. Al Viro > > further pointed out that these functions already existed in the iov_iter > > code.[1] > > > > Placing these functions in 'highmem.h' is suboptimal especially with the > > changes being proposed in the functionality of kmap. From a caller > > perspective including/using 'highmem.h' implies that the functions > > defined in that header are only required when highmem is in use which is > > increasingly not the case with modern processors. Some headers like > > mm.h or string.h seem ok but don't really portray the functionality > > well. 'pagemap.h', on the other hand, makes sense and is already > > included in many of the places we want to convert. > > > > Another alternative would be to create a new header for the promoted > > memcpy functions, but it masks the fact that these are designed to copy > > to/from pages using the kernel direct mappings and complicates matters > > with a new header. > > > > Lift memcpy_to_page(), memcpy_from_page(), and memzero_page() to > > pagemap.h. > > > > Also, add a memcpy_page(), memmove_page, and memset_page() to cover more > > kmap/mem*/kunmap. patterns. > > > > [1] https://lore.kernel.org/lkml/20201013200149.GI3576660@ZenIV.linux.org.uk/ > > https://lore.kernel.org/lkml/20201013112544.GA5249@infradead.org/ > > > > Cc: Dave Hansen <dave.hansen@intel.com> > > Suggested-by: Matthew Wilcox <willy@infradead.org> > > Suggested-by: Christoph Hellwig <hch@infradead.org> > > Suggested-by: Dan Williams <dan.j.williams@intel.com> > > Suggested-by: Al Viro <viro@zeniv.linux.org.uk> > > Suggested-by: Eric Biggers <ebiggers@kernel.org> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > <SNIP> > > > +static inline void memset_page(struct page *page, int val, size_t offset, size_t len) > > +{ > > + char *addr = kmap_atomic(page); > > + memset(addr + offset, val, len); > > + kunmap_atomic(addr); > > +} > > Other functions have (page, offset) pair. Insertion of 'val' in the middle here required > to take a double look during review. Let's be explicit here. Your suggested order is: (page, offset, val, len) right? I think I would prefer that to (page, val, offset, len).
On Mon, Nov 30, 2020 at 11:22:32AM +0200, Joonas Lahtinen wrote: > Quoting Matthew Wilcox (2020-11-27 15:20:06) > > On Fri, Nov 27, 2020 at 03:06:24PM +0200, Joonas Lahtinen wrote: > > > Quoting ira.weiny@intel.com (2020-11-24 08:07:39) > > > > From: Ira Weiny <ira.weiny@intel.com> > > > > > > > > Working through a conversion to a call such as kmap_thread() revealed > > > > many places where the pattern kmap/memcpy/kunmap occurred. > > > > > > > > Eric Biggers, Matthew Wilcox, Christoph Hellwig, Dan Williams, and Al > > > > Viro all suggested putting this code into helper functions. Al Viro > > > > further pointed out that these functions already existed in the iov_iter > > > > code.[1] > > > > > > > > Placing these functions in 'highmem.h' is suboptimal especially with the > > > > changes being proposed in the functionality of kmap. From a caller > > > > perspective including/using 'highmem.h' implies that the functions > > > > defined in that header are only required when highmem is in use which is > > > > increasingly not the case with modern processors. Some headers like > > > > mm.h or string.h seem ok but don't really portray the functionality > > > > well. 'pagemap.h', on the other hand, makes sense and is already > > > > included in many of the places we want to convert. > > > > > > > > Another alternative would be to create a new header for the promoted > > > > memcpy functions, but it masks the fact that these are designed to copy > > > > to/from pages using the kernel direct mappings and complicates matters > > > > with a new header. > > > > > > > > Lift memcpy_to_page(), memcpy_from_page(), and memzero_page() to > > > > pagemap.h. > > > > > > > > Also, add a memcpy_page(), memmove_page, and memset_page() to cover more > > > > kmap/mem*/kunmap. patterns. > > > > > > > > [1] https://lore.kernel.org/lkml/20201013200149.GI3576660@ZenIV.linux.org.uk/ > > > > https://lore.kernel.org/lkml/20201013112544.GA5249@infradead.org/ > > > > > > > > Cc: Dave Hansen <dave.hansen@intel.com> > > > > Suggested-by: Matthew Wilcox <willy@infradead.org> > > > > Suggested-by: Christoph Hellwig <hch@infradead.org> > > > > Suggested-by: Dan Williams <dan.j.williams@intel.com> > > > > Suggested-by: Al Viro <viro@zeniv.linux.org.uk> > > > > Suggested-by: Eric Biggers <ebiggers@kernel.org> > > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > > > > > <SNIP> > > > > > > > +static inline void memset_page(struct page *page, int val, size_t offset, size_t len) > > > > +{ > > > > + char *addr = kmap_atomic(page); > > > > + memset(addr + offset, val, len); > > > > + kunmap_atomic(addr); > > > > +} > > > > > > Other functions have (page, offset) pair. Insertion of 'val' in the middle here required > > > to take a double look during review. > > > > Let's be explicit here. Your suggested order is: > > > > (page, offset, val, len) > > > > right? I think I would prefer that to (page, val, offset, len). > > Yeah, I think that would be most consistent order. Yes as I have been reworking these I have found it odd as well. I'm going to swap it around. Been learning Coccinelle which has helped find other instances... So V2 is taking a bit of time. Thanks, Ira
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h index c77b7c31b2e4..82a0af6bc843 100644 --- a/include/linux/pagemap.h +++ b/include/linux/pagemap.h @@ -1028,4 +1028,53 @@ unsigned int i_blocks_per_page(struct inode *inode, struct page *page) { return thp_size(page) >> inode->i_blkbits; } + +static inline void memcpy_page(struct page *dst_page, size_t dst_off, + struct page *src_page, size_t src_off, + size_t len) +{ + char *dst = kmap_atomic(dst_page); + char *src = kmap_atomic(src_page); + memcpy(dst + dst_off, src + src_off, len); + kunmap_atomic(src); + kunmap_atomic(dst); +} + +static inline void memmove_page(struct page *dst_page, size_t dst_off, + struct page *src_page, size_t src_off, + size_t len) +{ + char *dst = kmap_atomic(dst_page); + char *src = kmap_atomic(src_page); + memmove(dst + dst_off, src + src_off, len); + kunmap_atomic(src); + kunmap_atomic(dst); +} + +static inline void memcpy_from_page(char *to, struct page *page, size_t offset, size_t len) +{ + char *from = kmap_atomic(page); + memcpy(to, from + offset, len); + kunmap_atomic(from); +} + +static inline void memcpy_to_page(struct page *page, size_t offset, const char *from, size_t len) +{ + char *to = kmap_atomic(page); + memcpy(to + offset, from, len); + kunmap_atomic(to); +} + +static inline void memset_page(struct page *page, int val, size_t offset, size_t len) +{ + char *addr = kmap_atomic(page); + memset(addr + offset, val, len); + kunmap_atomic(addr); +} + +static inline void memzero_page(struct page *page, size_t offset, size_t len) +{ + memset_page(page, 0, offset, len); +} + #endif /* _LINUX_PAGEMAP_H */ diff --git a/lib/iov_iter.c b/lib/iov_iter.c index 1635111c5bd2..2439a8b4f0d2 100644 --- a/lib/iov_iter.c +++ b/lib/iov_iter.c @@ -466,27 +466,6 @@ void iov_iter_init(struct iov_iter *i, unsigned int direction, } EXPORT_SYMBOL(iov_iter_init); -static void memcpy_from_page(char *to, struct page *page, size_t offset, size_t len) -{ - char *from = kmap_atomic(page); - memcpy(to, from + offset, len); - kunmap_atomic(from); -} - -static void memcpy_to_page(struct page *page, size_t offset, const char *from, size_t len) -{ - char *to = kmap_atomic(page); - memcpy(to + offset, from, len); - kunmap_atomic(to); -} - -static void memzero_page(struct page *page, size_t offset, size_t len) -{ - char *addr = kmap_atomic(page); - memset(addr + offset, 0, len); - kunmap_atomic(addr); -} - static inline bool allocated(struct pipe_buffer *buf) { return buf->ops == &default_pipe_buf_ops;