[102/147] lib/string: optimized memmove

Commit Message

Andrew Morton Sept. 8, 2021, 2:58 a.m. UTC

From: Matteo Croce <[e-mail]>
Subject: lib/string: optimized memmove

When the destination buffer is before the source one, or when the buffers
doesn't overlap, it's safe to use memcpy() instead, which is optimized to
use a bigger data size possible.

This "optimization" only covers a common case.  In future, proper code
which does the same thing as memcpy() does but backwards can be done.

Link: https://lkml.kernel.org/r/[e-mail]
Signed-off-by: Matteo Croce <[e-mail]>
Cc: Christoph Hellwig <[e-mail]>
Cc: David Laight <[e-mail]>
Cc: Drew Fustini <[e-mail]>
Cc: Emil Renner Berthing <[e-mail]>
Cc: Guo Ren <[e-mail]>
Cc: Nick Desaulniers <[e-mail]>
Cc: Nick Kossifidis <[e-mail]>
Cc: Palmer Dabbelt <[e-mail]>
Signed-off-by: Andrew Morton <[e-mail]>
---

 lib/string.c |   18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

Comments

Linus Torvalds Sept. 8, 2021, 6:29 p.m. UTC | #1

On Tue, Sep 7, 2021 at 7:58 PM Andrew Morton <[e-mail]> wrote:
>
> When the destination buffer is before the source one, or when the buffers
> doesn't overlap, it's safe to use memcpy() instead, which is optimized to
> use a bigger data size possible.

This one is actively buggy.

It depends on the possibly incorrect assumption that memcpy() always
copies upwards.

That is admittedly commonly true, but it's not something we can depend
on. Not even when the memcpy() implementation in the very same file
ends up doing so - because architectures can and should replace that
function with their own ones, and we have that __HAVE_ARCH_MEMCPY for
exactly that case.

Like 101/147, all reasonable architectures end up having their own
implementation anyway, but the immediate reason I'm dropping this
patch is that it's literally incorrect.

             Linus

David Laight Sept. 9, 2021, 8:28 a.m. UTC | #2

From: Linus Torvalds
> Sent: 08 September 2021 19:30
> 
> On Tue, Sep 7, 2021 at 7:58 PM Andrew Morton <[e-mail]> wrote:
> >
> > When the destination buffer is before the source one, or when the buffers
> > doesn't overlap, it's safe to use memcpy() instead, which is optimized to
> > use a bigger data size possible.
> 
> This one is actively buggy.
> 
> It depends on the possibly incorrect assumption that memcpy() always
> copies upwards.

Even if the memcpy() 'mostly' copies upwards it may copy the last
8 bytes first and then copy the rest of the buffer in 8 byte chunks.

OTOH the change to libc that made it do backwards is just stupid.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Patch

--- a/lib/string.c~lib-string-optimized-memmove
+++ a/lib/string.c
@@ -975,19 +975,13 @@  EXPORT_SYMBOL(memcpy);
  */
 void *memmove(void *dest, const void *src, size_t count)
 {
-	char *tmp;
-	const char *s;
+	if (dest < src || src + count <= dest)
+		return memcpy(dest, src, count);
+
+	if (dest > src) {
+		const char *s = src + count;
+		char *tmp = dest + count;
 
-	if (dest <= src) {
-		tmp = dest;
-		s = src;
-		while (count--)
-			*tmp++ = *s++;
-	} else {
-		tmp = dest;
-		tmp += count;
-		s = src;
-		s += count;
 		while (count--)
 			*--tmp = *--s;
 	}