From: Ira Weiny <[email protected]>
Working through a conversion to a call kmap_local_page() instead of
kmap() 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]
Various locations for the lifted functions were considered.
Headers like mm.h or string.h seem ok but don't really portray the
functionality well. pagemap.h made some sense but is for page cache
functionality.[2]
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.
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. However, highmem.h is
where all the current functions like this reside (zero_user(),
clear_highpage(), clear_user_highpage(), copy_user_highpage(), and
copy_highpage()). So it makes the most sense even though it is
distasteful for some.[3]
Lift memcpy_to_page() and memcpy_from_page() to pagemap.h.
[1] https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
[2] https://lore.kernel.org/lkml/[email protected]/
[3] https://lore.kernel.org/lkml/[email protected]/#t
https://lore.kernel.org/lkml/[email protected]/
Cc: Boris Pismenny <[email protected]>
Cc: Or Gerlitz <[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]>
---
Changes from v1 btrfs series:
https://lore.kernel.org/lkml/[email protected]/
Split out the BUG_ON()'s per Andrew
Split out the change to kmap_local_page() per Andrew
Split out the addition of memcpy_page, memmove_page, and memset_page
While we are refactoring adjust the line length down per Chaitanya
https://lore.kernel.org/lkml/BYAPR04MB49655E5BDB24A108FEFFE9C486B09@BYAPR04MB4965.namprd04.prod.outlook.com/
Chagnes for V4:
Update commit message to say kmap_local_page() since
kmap_thread() is no longer valid
Changes for V3:
From Matthew Wilcox
Move calls to highmem.h
Add BUG_ON()
Changes for V2:
From Thomas Gleixner
Change kmap_atomic() to kmap_local_page() after basing
on tip/core/mm
From Joonas Lahtinen
Reverse offset/val in memset_page()
---
include/linux/highmem.h | 18 ++++++++++++++++++
lib/iov_iter.c | 14 --------------
2 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index d2c70d3772a3..736b6a9f144d 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -276,4 +276,22 @@ static inline void copy_highpage(struct page *to, struct page *from)
#endif
+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);
+}
+
#endif /* _LINUX_HIGHMEM_H */
diff --git a/lib/iov_iter.c b/lib/iov_iter.c
index a21e6a5792c5..9889e9903cdf 100644
--- a/lib/iov_iter.c
+++ b/lib/iov_iter.c
@@ -466,20 +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);
--
2.28.0.rc0.12.gb6a658bd00c9
On 2/9/21 22:25, [email protected] wrote:
> From: Ira Weiny <[email protected]>
>
> Working through a conversion to a call kmap_local_page() instead of
> kmap() 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]
>
> Various locations for the lifted functions were considered.
>
> Headers like mm.h or string.h seem ok but don't really portray the
> functionality well. pagemap.h made some sense but is for page cache
> functionality.[2]
>
> 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.
>
> 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. However, highmem.h is
> where all the current functions like this reside (zero_user(),
> clear_highpage(), clear_user_highpage(), copy_user_highpage(), and
> copy_highpage()). So it makes the most sense even though it is
> distasteful for some.[3]
>
> Lift memcpy_to_page() and memcpy_from_page() to pagemap.h.
>
> [1] https://lore.kernel.org/lkml/[email protected]/
> https://lore.kernel.org/lkml/[email protected]/
>
> [2] https://lore.kernel.org/lkml/[email protected]/
>
> [3] https://lore.kernel.org/lkml/[email protected]/#t
> https://lore.kernel.org/lkml/[email protected]/
>
> Cc: Boris Pismenny <[email protected]>
> Cc: Or Gerlitz <[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]>
Thanks for adding a new line in the new calls after variable declaration.
Looks good.
Reviewed-by: Chaitanya Kulkarni <[email protected]>
Looks good,
Reviewed-by: Christoph Hellwig <[email protected]>