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;
> _
>
