Re: [PATCH 1/4] mm/highmem: Lift memcpy_[to|from]_page to core

2021-02-07 Thread Chaitanya Kulkarni
On 2/7/21 19:13, Ira Weiny wrote:
>>> +static inline void memcpy_from_page(char *to, struct page *page, size_t 
>>> offset, size_t len)
>> How about following ?
>> static inline void memcpy_from_page(char *to, struct page *page, size_t
>> offset,
>> size_t len)  
> It is an easy change and It is up to Andrew.  But I thought we were making the
> line length limit longer now.
>
> Ira
>
True, not sure what is the right thing going forward especially when new
changes
are mixed with the old ones, I'll leave it to the maintainer to decide.


Re: [PATCH 1/4] mm/highmem: Lift memcpy_[to|from]_page to core

2021-02-07 Thread Ira Weiny
On Sun, Feb 07, 2021 at 01:46:47AM +, Chaitanya Kulkarni wrote:
> On 2/5/21 18:35, ira.we...@intel.com wrote:
> > +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_local_page(dst_page);
> > +   char *src = kmap_local_page(src_page);
> > +
> > +   BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
> > +   memmove(dst + dst_off, src + src_off, len);
> > +   kunmap_local(src);
> > +   kunmap_local(dst);
> > +}
> > +
> > +static inline void memcpy_from_page(char *to, struct page *page, size_t 
> > offset, size_t len)
> How about following ?
> static inline void memcpy_from_page(char *to, struct page *page, size_t
> offset,
> size_t len)  

It is an easy change and It is up to Andrew.  But I thought we were making the
line length limit longer now.

Ira

> > +{
> > +   char *from = kmap_local_page(page);
> > +
> > +   BUG_ON(offset + len > PAGE_SIZE);
> > +   memcpy(to, from + offset, len);
> > +   kunmap_local(from);
> > +}
> > +
> > +static inline void memcpy_to_page(struct page *page, size_t offset, const 
> > char *from, size_t len)
> How about following ?
> static inline void memcpy_to_page(struct page *page, size_t offset,
>   const char *from, size_t len)
> > +{
> > +   char *to = kmap_local_page(page);
> > +
> > +   BUG_ON(offset + len > PAGE_SIZE);
> > +   memcpy(to + offset, from, len);
> > +   kunmap_local(to);
> > +}
> > +
> > +static inline void memset_page(struct page *page, size_t offset, int val, 
> > size_t len)
> How about following ?
> static inline void memset_page(struct page *page, size_t offset, int val,
>size_t len)  
> > +{
> > +   char *addr = kmap_local_page(page);
> > +
> > +   BUG_ON(offset + len > PAGE_SIZE);
> > +   memset(addr + offset, val, len);
> > +   kunmap_local(addr);
> > +}
> > +


Re: [PATCH 1/4] mm/highmem: Lift memcpy_[to|from]_page to core

2021-02-06 Thread Chaitanya Kulkarni
On 2/5/21 18:35, ira.we...@intel.com wrote:
> +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_local_page(dst_page);
> + char *src = kmap_local_page(src_page);
> +
> + BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
> + memmove(dst + dst_off, src + src_off, len);
> + kunmap_local(src);
> + kunmap_local(dst);
> +}
> +
> +static inline void memcpy_from_page(char *to, struct page *page, size_t 
> offset, size_t len)
How about following ?
static inline void memcpy_from_page(char *to, struct page *page, size_t
offset,
size_t len)  
> +{
> + char *from = kmap_local_page(page);
> +
> + BUG_ON(offset + len > PAGE_SIZE);
> + memcpy(to, from + offset, len);
> + kunmap_local(from);
> +}
> +
> +static inline void memcpy_to_page(struct page *page, size_t offset, const 
> char *from, size_t len)
How about following ?
static inline void memcpy_to_page(struct page *page, size_t offset,
  const char *from, size_t len)
> +{
> + char *to = kmap_local_page(page);
> +
> + BUG_ON(offset + len > PAGE_SIZE);
> + memcpy(to + offset, from, len);
> + kunmap_local(to);
> +}
> +
> +static inline void memset_page(struct page *page, size_t offset, int val, 
> size_t len)
How about following ?
static inline void memset_page(struct page *page, size_t offset, int val,
   size_t len)  
> +{
> + char *addr = kmap_local_page(page);
> +
> + BUG_ON(offset + len > PAGE_SIZE);
> + memset(addr + offset, val, len);
> + kunmap_local(addr);
> +}
> +


[PATCH 1/4] mm/highmem: Lift memcpy_[to|from]_page to core

2021-02-05 Thread ira . weiny
From: Ira Weiny 

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.

Remove memzero_page() in favor of zero_user() to zero a page.

Add a memcpy_page(), memmove_page, and memset_page() to cover more
kmap/mem*/kunmap. patterns.

Add BUG_ON bounds checks to ensure the newly lifted page memory
operations do not result in corrupted data in neighbor pages and to make
them consistent with zero_user().[4]

Finally use kmap_local_page() in all the new calls.

[1] https://lore.kernel.org/lkml/20201013200149.gi3576...@zeniv.linux.org.uk/
https://lore.kernel.org/lkml/20201013112544.ga5...@infradead.org/

[2] https://lore.kernel.org/lkml/20201208122316.gh7...@casper.infradead.org/

[3] https://lore.kernel.org/lkml/20201013200149.gi3576...@zeniv.linux.org.uk/#t

https://lore.kernel.org/lkml/20201208163814.gn1563...@iweiny-desk2.sc.intel.com/

[4] 
https://lore.kernel.org/lkml/20201210053502.gs1563...@iweiny-desk2.sc.intel.com/

Cc: Boris Pismenny 
Cc: Or Gerlitz 
Cc: Dave Hansen 
Suggested-by: Matthew Wilcox 
Suggested-by: Christoph Hellwig 
Suggested-by: Dan Williams 
Suggested-by: Al Viro 
Suggested-by: Eric Biggers 
Signed-off-by: Ira Weiny 

---
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 | 53 +
 lib/iov_iter.c  | 26 +++-
 2 files changed, 56 insertions(+), 23 deletions(-)

diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index d2c70d3772a3..c642dd64ea3f 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -276,4 +276,57 @@ static inline void copy_highpage(struct page *to, struct 
page *from)
 
 #endif
 
+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_local_page(dst_page);
+   char *src = kmap_local_page(src_page);
+
+   BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
+   memcpy(dst + dst_off, src + src_off, len);
+   kunmap_local(src);
+   kunmap_local(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_local_page(dst_page);
+   char *src = kmap_local_page(src_page);
+
+   BUG_ON(dst_off + len > PAGE_SIZE || src_off + len > PAGE_SIZE);
+   memmove(dst + dst_off, src + src_off, len);
+   kunmap_local(src);
+   kunmap_local(dst);
+}
+
+static inline void memcpy_from_page(char *to, struct page *page, size_t 
offset, size_t len)
+{
+   char *from = kmap_local_page(page);
+
+   BUG_ON(offset + len > PAGE_SIZE);
+   memcpy(to, from + offset, len);
+   kunmap_local(from);
+}
+
+static inline void memcpy_to_page(struct page *page, size_t offset, const char 
*from, size_t len)
+{
+   char *to = kmap_local_page(page);
+
+   BUG_ON(offset + len > PAGE_SIZE);
+   memcpy(to + offset, from, len);
+   kunmap_local(to);
+}
+
+static inline void memset_page(struct page *page, size_t offset, int val,