[DTrace-devel] [PATCH 1/4] bpf: introduce atomic_add()

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
with usual comments/questions...

On 5/9/23 03:16, Kris Van Hees via DTrace-devel wrote:
> The GCC BPF support does not provide a way to emit an atomic add
> instruction (BPF xadd) without the fetch-flag.  But since older
> kernels do not support xadd with the fetch flag, the generated
> BPF programs are rejected by the BPF verifier.
>
> The aotmic_add() macro emits a raw xadd instruction that works on
> all kernels.

aotmic

> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
>   bpf/speculation.c |  9 +++------
>   include/bpf-lib.h | 27 ++++++++++++++++++++++++++-
>   2 files changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/bpf/speculation.c b/bpf/speculation.c
> index 90433753..0a19ac33 100644
> --- a/bpf/speculation.c
> +++ b/bpf/speculation.c
> @@ -1,10 +1,11 @@
>   // SPDX-License-Identifier: GPL-2.0
>   /*
> - * Copyright (c) 2021, 2022, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2021, 2023, Oracle and/or its affiliates. All rights reserved.
>    */
>   #include <linux/bpf.h>
>   #include <stdint.h>
>   #include <bpf-helpers.h>
> +#include <bpf-lib.h>
>   #include <dt_bpf_maps.h>
>   
>   #include <dtrace/faults_defines.h>
> @@ -96,11 +97,7 @@ dt_speculation_speculate(uint32_t id)
>   	if (spec->draining)
>   		return -1;
>   
> -	spec->written++;
> -	/* Use atomics once GCC/binutils can emit them in forms that older
> -	 * kernels support:
> -	 * __atomic_add_fetch(&spec->written, 1, __ATOMIC_RELAXED);
> -	 */
> +	atomic_add(&spec->written, 1);
>   	return 0;
>   }
>   
> diff --git a/include/bpf-lib.h b/include/bpf-lib.h
> index d7078da5..fc3fe4fa 100644
> --- a/include/bpf-lib.h
> +++ b/include/bpf-lib.h
> @@ -1,6 +1,6 @@
>   /*
>    * Oracle Linux DTrace.
> - * Copyright (c) 2021, 2022, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2021, 2023, Oracle and/or its affiliates. All rights reserved.
>    * Licensed under the Universal Permissive License v 1.0 as shown at
>    * http://oss.oracle.com/licenses/upl.
>    */
> @@ -61,5 +61,30 @@
>                   : /* no clobbers */ \
>           );
>   
> +/*
> + * Explicit inline assembler to implement atomic add:
> + *
> + *	*ptr += val;
> + */
> +#define atomic_add(valp, val) \
> +	do { \
> +		register uint64_t *ptr asm("%r0") = (valp); \
> +		register uint64_t tmp asm("%r1") = (val); \
> +		asm (".byte 0xdb, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00" \
> +			: /* no outputs */ \
> +			: "r" (ptr), "r" (tmp) \
> +			: "memory" \
> +		); \
> +	} while (0)
> +#define atomic_add32(valp, val) \
> +	do { \
> +		register uint32_t *ptr asm("%r0") = (valp); \
> +		register uint32_t tmp asm("%r1") = (val); \
> +		asm (".byte 0xc3, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00" \
> +			: /* no outputs */ \
> +			: "r" (ptr), "r" (tmp) \
> +			: "memory" \
> +		); \
> +	} while (0)
>   
>   #endif /* BPF_LIB_H */

It might be nice to have some comments.  E.g.,
*) 0xdb==(BPF_XADD|BPF_DW|BPF_STX) and 0xc3==(BPF_XADD|BPF_W|BPF_STX)
*) iiuc, that %r0 and %r1 will be clobbered... is that a concern? (Or we 
expect to get lucky?  Or user beware?)

Also, BPF_XADD allows an offset to the pointer... worth adding that 
generality?  The focus on this patch is presumably adding xadd 
functionality, which has an offset, and not the specific spec+=1 change, 
which doesn't need it.