From: Ira Weiny <[email protected]>
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/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
Cc: Dave Hansen <[email protected]>
Suggested-by: Matthew Wilcox <[email protected]>
Suggested-by: Christoph Hellwig <[email protected]>
Suggested-by: Dan Williams <[email protected]>
Suggested-by: Al Viro <[email protected]>
Suggested-by: Eric Biggers <[email protected]>
Signed-off-by: Ira Weiny <[email protected]>
---
include/linux/pagemap.h | 49 +++++++++++++++++++++++++++++++++++++++++
lib/iov_iter.c | 21 ------------------
2 files changed, 49 insertions(+), 21 deletions(-)
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;
--
2.28.0.rc0.12.gb6a658bd00c9
On Mon, Nov 23, 2020 at 10:07:39PM -0800, [email protected] 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, [email protected] 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, [email protected] 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 <[email protected]>
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 [email protected] (2020-11-24 08:07:39)
> > From: Ira Weiny <[email protected]>
> >
> > 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/[email protected]/
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > Cc: Dave Hansen <[email protected]>
> > Suggested-by: Matthew Wilcox <[email protected]>
> > Suggested-by: Christoph Hellwig <[email protected]>
> > Suggested-by: Dan Williams <[email protected]>
> > Suggested-by: Al Viro <[email protected]>
> > Suggested-by: Eric Biggers <[email protected]>
> > Signed-off-by: Ira Weiny <[email protected]>
>
> <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 [email protected] (2020-11-24 08:07:39)
> > > > From: Ira Weiny <[email protected]>
> > > >
> > > > 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/[email protected]/
> > > > https://lore.kernel.org/lkml/[email protected]/
> > > >
> > > > Cc: Dave Hansen <[email protected]>
> > > > Suggested-by: Matthew Wilcox <[email protected]>
> > > > Suggested-by: Christoph Hellwig <[email protected]>
> > > > Suggested-by: Dan Williams <[email protected]>
> > > > Suggested-by: Al Viro <[email protected]>
> > > > Suggested-by: Eric Biggers <[email protected]>
> > > > Signed-off-by: Ira Weiny <[email protected]>
> > >
> > > <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