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

[Kris Van Hees]

On Tue, May 09, 2023 at 12:40:55PM -0400, Eugene Loh via DTrace-devel wrote:
> 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

Thanks.

> > 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?)

%r0 and %r1 do not get clobbered by the asm statement, and the fact that I
declare ptr and tmp as explicitly to be stored in %r0 and %r1 automatically
marks them as clobbered by this code block.

> 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.

Well, yes and no.  For now, this is enough functionality, and I sincerely
hope that we will have a bpf-gcc (and binutils) that can generate these
xadd instructions correctly before too long.  So, rather than messing with
the extra complexity of also encoding an offset that is not needed right
now, I'd rather get in what we need.

If it turns out that we need offset support as well, we can add it.

	Kris