[PATCH] mm: Add memcpy_from_file_folio()

[PATCH] mm: Add memcpy_from_file_folio()

[Matthew Wilcox (Oracle)]

This is the equivalent of memcpy_from_page().  It differs in that it
takes the position in a file instead of offset in a folio, it accepts
the total number of bytes to be copied (instead of the number of bytes
to be copied from this folio) and it returns how many bytes were copied
from the folio, rather than making the caller calculate that and then
checking if the caller got it right.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/highmem.h    | 29 +++++++++++++++++++++++++++++
 include/linux/page-flags.h |  1 +
 2 files changed, 30 insertions(+)

diff --git a/include/linux/highmem.h b/include/linux/highmem.h
index e22509420ac6..b517707b028d 100644
--- a/include/linux/highmem.h
+++ b/include/linux/highmem.h
@@ -413,6 +413,35 @@ static inline void memzero_page(struct page *page, size_t offset, size_t len)
 	kunmap_local(addr);
 }
 
+/**
+ * memcpy_from_file_folio - Copy some bytes from a file folio.
+ * @to: The destination buffer.
+ * @folio: The folio to copy from.
+ * @pos: The position in the file.
+ * @len: The maximum number of bytes to copy.
+ *
+ * Copy up to @len bytes from this folio.  This may be limited by PAGE_SIZE
+ * if the folio comes from HIGHMEM, and by the size of the folio.
+ *
+ * Return: The number of bytes copied from the folio.
+ */
+static inline size_t memcpy_from_file_folio(char *to, struct folio *folio,
+		loff_t pos, size_t len)
+{
+	size_t offset = offset_in_folio(folio, pos);
+	char *from = kmap_local_folio(folio, offset);
+
+	if (folio_test_highmem(folio))
+		len = min(len, PAGE_SIZE - offset);
+	else
+		len = min(len, folio_size(folio) - offset);
+
+	memcpy(to, from, len);
+	kunmap_local(from);
+
+	return len;
+}
+
 /**
  * folio_zero_segments() - Zero two byte ranges in a folio.
  * @folio: The folio to write to.
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 09804ad91927..0425f22a9c82 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -531,6 +531,7 @@ PAGEFLAG(Readahead, readahead, PF_NO_COMPOUND)
  * available at this point.
  */
 #define PageHighMem(__p) is_highmem_idx(page_zonenum(__p))
+#define folio_test_highmem(__f)	is_highmem_idx(folio_zonenum(__f))
 #else
 PAGEFLAG_FALSE(HighMem, highmem)
 #endif
-- 
2.35.1

Re: [PATCH] mm: Add memcpy_from_file_folio()

[Andrew Morton]

On Thu, 26 Jan 2023 20:15:52 +0000 "Matthew Wilcox (Oracle)" <willy@infradead.org> wrote:

> This is the equivalent of memcpy_from_page().  It differs in that it
> takes the position in a file instead of offset in a folio, it accepts
> the total number of bytes to be copied (instead of the number of bytes
> to be copied from this folio) and it returns how many bytes were copied
> from the folio, rather than making the caller calculate that and then
> checking if the caller got it right.
> 
> ...
>
> --- a/include/linux/highmem.h
> +++ b/include/linux/highmem.h
> @@ -413,6 +413,35 @@ static inline void memzero_page(struct page *page, size_t offset, size_t len)
>  	kunmap_local(addr);
>  }
>  
> +/**
> + * memcpy_from_file_folio - Copy some bytes from a file folio.
> + * @to: The destination buffer.
> + * @folio: The folio to copy from.
> + * @pos: The position in the file.
> + * @len: The maximum number of bytes to copy.
> + *
> + * Copy up to @len bytes from this folio.  This may be limited by PAGE_SIZE
> + * if the folio comes from HIGHMEM, and by the size of the folio.
> + *
> + * Return: The number of bytes copied from the folio.
> + */
> +static inline size_t memcpy_from_file_folio(char *to, struct folio *folio,
> +		loff_t pos, size_t len)
> +{
> +	size_t offset = offset_in_folio(folio, pos);
> +	char *from = kmap_local_folio(folio, offset);
> +
> +	if (folio_test_highmem(folio))
> +		len = min(len, PAGE_SIZE - offset);
> +	else
> +		len = min(len, folio_size(folio) - offset);

min() blows up on arm allnoconfig.

./include/linux/highmem.h: In function 'memcpy_from_file_folio':
./include/linux/minmax.h:20:35: warning: comparison of distinct pointer types lacks a cast
   20 |         (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
      |                                   ^~
./include/linux/minmax.h:26:18: note: in expansion of macro '__typecheck'
   26 |                 (__typecheck(x, y) && __no_side_effects(x, y))
      |                  ^~~~~~~~~~~
./include/linux/minmax.h:36:31: note: in expansion of macro '__safe_cmp'
   36 |         __builtin_choose_expr(__safe_cmp(x, y), \
      |                               ^~~~~~~~~~
./include/linux/minmax.h:67:25: note: in expansion of macro '__careful_cmp'
   67 | #define min(x, y)       __careful_cmp(x, y, <)
      |                         ^~~~~~~~~~~~~
./include/linux/highmem.h:435:23: note: in expansion of macro 'min'
  435 |                 len = min(len, PAGE_SIZE - offset);
      |                       ^~~

We could use min_t(), but perhaps and explanatorialy named variable is
nicer?

--- a/include/linux/highmem.h~mm-add-memcpy_from_file_folio-fix
+++ a/include/linux/highmem.h
@@ -430,13 +430,14 @@ static inline size_t memcpy_from_file_fo
 {
 	size_t offset = offset_in_folio(folio, pos);
 	char *from = kmap_local_folio(folio, offset);
+	size_t remaining;
 
 	if (folio_test_highmem(folio))
-		len = min(len, PAGE_SIZE - offset);
+		remaining = PAGE_SIZE - offset;
 	else
-		len = min(len, folio_size(folio) - offset);
+		remaining = folio_size(folio) - offset;
 
-	memcpy(to, from, len);
+	memcpy(to, from, min(len, remaining));
 	kunmap_local(from);
 
 	return len;
_

Re: [PATCH] mm: Add memcpy_from_file_folio()

[Matthew Wilcox]

On Thu, Jan 26, 2023 at 03:41:52PM -0800, Andrew Morton wrote:
> > + * Return: The number of bytes copied from the folio.

> > +static inline size_t memcpy_from_file_folio(char *to, struct folio *folio,
> > +		loff_t pos, size_t len)
> > +{
> > +	size_t offset = offset_in_folio(folio, pos);
> > +	char *from = kmap_local_folio(folio, offset);
> > +
> > +	if (folio_test_highmem(folio))
> > +		len = min(len, PAGE_SIZE - offset);
> > +	else
> > +		len = min(len, folio_size(folio) - offset);
> 
> min() blows up on arm allnoconfig.
> 
> ./include/linux/highmem.h: In function 'memcpy_from_file_folio':
> ./include/linux/minmax.h:20:35: warning: comparison of distinct pointer types lacks a cast
>    20 |         (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
>       |                                   ^~
> ./include/linux/minmax.h:26:18: note: in expansion of macro '__typecheck'
>    26 |                 (__typecheck(x, y) && __no_side_effects(x, y))
>       |                  ^~~~~~~~~~~
> ./include/linux/minmax.h:36:31: note: in expansion of macro '__safe_cmp'
>    36 |         __builtin_choose_expr(__safe_cmp(x, y), \
>       |                               ^~~~~~~~~~
> ./include/linux/minmax.h:67:25: note: in expansion of macro '__careful_cmp'
>    67 | #define min(x, y)       __careful_cmp(x, y, <)
>       |                         ^~~~~~~~~~~~~
> ./include/linux/highmem.h:435:23: note: in expansion of macro 'min'
>   435 |                 len = min(len, PAGE_SIZE - offset);
>       |                       ^~~

Oh, right, PAGE_SIZE is size_t everywhere except on ARM.

> We could use min_t(), but perhaps and explanatorialy named variable is
> nicer?

But buggy because we return the number of bytes copied.

> --- a/include/linux/highmem.h~mm-add-memcpy_from_file_folio-fix
> +++ a/include/linux/highmem.h
> @@ -430,13 +430,14 @@ static inline size_t memcpy_from_file_fo
>  {
>  	size_t offset = offset_in_folio(folio, pos);
>  	char *from = kmap_local_folio(folio, offset);
> +	size_t remaining;
>  
>  	if (folio_test_highmem(folio))
> -		len = min(len, PAGE_SIZE - offset);
> +		remaining = PAGE_SIZE - offset;
>  	else
> -		len = min(len, folio_size(folio) - offset);
> +		remaining = folio_size(folio) - offset;

I don't think remaining is a great name for this.  The key thing is
that for any platform we care about folio_test_highmem() is constant false,
so this just optimises away the min() call.

You could salvage this approach by doing

	len = min(remaining, len);
	memcpy(to, from, len);
	return len;

but I think it's probably better to just do min_t on the PAGE_SIZE line.
Stupid ARM.

> -	memcpy(to, from, len);
> +	memcpy(to, from, min(len, remaining));
>  	kunmap_local(from);
>  
>  	return len;
> _
>