[DTrace-devel] [PATCH v2 18/20] Optimize the memcpy implementation and make it more robust

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>

The commit message says "resolves this."  Presumably, it's referring to 
a specific case.  So, perhaps qualify the claim, as in "this patch has 
demonstrated improvement in some cases."  Or, is it indeed the case that 
the BPF verifier will now always be able to guarantee the function is safe?

Incidentally, when will the 256-byte cap on memcpy be lifted? Even 
assignment by reference will need longer copies.  Not just strings.

On 6/3/21 11:20 AM, Kris Van Hees wrote:
> The memcpy() implementation in BPF was more complicated than it needed
> to be.  As a result, the BPF verifier was not able to always guarantee
> that this function was being used in a safe way.  The simplification of
> the implementation resolves this.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   bpf/memcpy.c | 49 +++++++++++++++++++++++++++----------------------
>   1 file changed, 27 insertions(+), 22 deletions(-)
>
> diff --git a/bpf/memcpy.c b/bpf/memcpy.c
> index 6e0d17f4..42a688f3 100644
> --- a/bpf/memcpy.c
> +++ b/bpf/memcpy.c
> @@ -1,6 +1,6 @@
>   // SPDX-License-Identifier: GPL-2.0
>   /*
> - * Copyright (c) 2019, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2019, 2021, Oracle and/or its affiliates. All rights reserved.
>    */
>   #include <stddef.h>
>   #include <stdint.h>
> @@ -9,16 +9,17 @@
>   # define noinline	__attribute__((noinline))
>   #endif
>   
> -noinline int dt_memcpy_int(void *dst, void *src, size_t n)
> +noinline int dt_memcpy_int(void *dst, const void *src, size_t n)
>   {
>   	uint64_t	*d = dst;
> -	uint64_t	*s = src;
> -	size_t		r, i;
> +	const uint64_t	*s = src;
> +	size_t		q;
>   
>   	if (n > 128)
>   		return -1;
>   
> -	switch (n / 8) {
> +	q = n / 8;
> +	switch (q) {
>   	case 16:
>   		d[15] = s[15];
>   	case 15:
> @@ -53,21 +54,24 @@ noinline int dt_memcpy_int(void *dst, void *src, size_t n)
>   		d[0] = s[0];
>   	}
>   
> -	r = n % 8;
> -	if (r >= 4) {
> -		i = (n / 4) - 1;
> -		((uint32_t *)dst)[i] = ((uint32_t *)src)[i];
> -		r -= 4;
> -	}
> -	if (r >= 2) {
> -		i = (n / 2) - 1;
> -		((uint16_t *)dst)[i] = ((uint16_t *)src)[i];
> -		r -= 2;
> +	if ((n % 8) == 0)
> +		return 0;
> +
> +	dst = &d[q];
> +	src = &s[q];
> +
> +	if (n & 4) {
> +		*(uint32_t *)dst = *(uint32_t *)src;
> +		dst += 4;
> +		src += 4;
>   	}
> -	if (r) {
> -		i = n - 1;
> -		((uint8_t *)dst)[i] = ((uint8_t *)src)[i];
> +	if (n & 2) {
> +		*(uint16_t *)dst = *(uint16_t *)src;
> +		dst += 2;
> +		src += 2;
>   	}
> +	if (n & 1)
> +		*(uint8_t *)dst = *(uint8_t *)src;
>   
>   	return 0;
>   }
> @@ -79,17 +83,18 @@ noinline int dt_memcpy_int(void *dst, void *src, size_t n)
>    *
>    * The size (n) must be no more than 256.
>    */
> -noinline int dt_memcpy(void *dst, void *src, size_t n)
> +noinline int dt_memcpy(void *dst, const void *src, size_t n)
>   {
>   	uint64_t	*d = dst;
> -	uint64_t	*s = src;
> +	const uint64_t	*s = src;
> +
> +	if (n == 0)
> +		return 0;
>   
>   	if (n > 128) {
>   		if (dt_memcpy_int(d, s, 128))
>   			return -1;
>   		n -= 128;
> -		if (n == 0)
> -			return 0;
>   
>   		d += 16;
>   		s += 16;