lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3489505c-3e33-880e-6f19-1796ca897553@iogearbox.net>
Date:   Wed, 7 Dec 2022 00:48:28 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Magnus Karlsson <magnus.karlsson@...il.com>,
        magnus.karlsson@...el.com, bjorn@...nel.org, ast@...nel.org,
        netdev@...r.kernel.org, maciej.fijalkowski@...el.com,
        bpf@...r.kernel.org, yhs@...com, andrii@...nel.org,
        martin.lau@...ux.dev, song@...nel.org, john.fastabend@...il.com,
        kpsingh@...nel.org, sdf@...gle.com, haoluo@...gle.com,
        jolsa@...nel.org
Cc:     jonathan.lemon@...il.com
Subject: Re: [PATCH bpf-next 07/15] selftests/xsk: get rid of asm
 store/release implementations

On 12/6/22 10:08 AM, Magnus Karlsson wrote:
> From: Magnus Karlsson <magnus.karlsson@...el.com>
> 
> Get rid of our own homegrown assembly store/release and load/acquire
> implementations. Use the HW agnositic APIs the compiler offers
> instead.

The description is a bit terse. Could you add a bit more context, discussion
or reference on why it's safe to replace them with C11 atomics?

> Signed-off-by: Magnus Karlsson <magnus.karlsson@...el.com>
> ---
>   tools/testing/selftests/bpf/xsk.h | 80 ++-----------------------------
>   1 file changed, 4 insertions(+), 76 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/xsk.h b/tools/testing/selftests/bpf/xsk.h
> index 997723b0bfb2..24ee765aded3 100644
> --- a/tools/testing/selftests/bpf/xsk.h
> +++ b/tools/testing/selftests/bpf/xsk.h
> @@ -23,77 +23,6 @@
>   extern "C" {
>   #endif
>   
> -/* This whole API has been deprecated and moved to libxdp that can be found at
> - * https://github.com/xdp-project/xdp-tools. The APIs are exactly the same so
> - * it should just be linking with libxdp instead of libbpf for this set of
> - * functionality. If not, please submit a bug report on the aforementioned page.
> - */
> -
> -/* Load-Acquire Store-Release barriers used by the XDP socket
> - * library. The following macros should *NOT* be considered part of
> - * the xsk.h API, and is subject to change anytime.
> - *
> - * LIBRARY INTERNAL
> - */
> -
> -#define __XSK_READ_ONCE(x) (*(volatile typeof(x) *)&x)
> -#define __XSK_WRITE_ONCE(x, v) (*(volatile typeof(x) *)&x) = (v)
> -
> -#if defined(__i386__) || defined(__x86_64__)
> -# define libbpf_smp_store_release(p, v)					\
> -	do {								\
> -		asm volatile("" : : : "memory");			\
> -		__XSK_WRITE_ONCE(*p, v);				\
> -	} while (0)
> -# define libbpf_smp_load_acquire(p)					\
> -	({								\
> -		typeof(*p) ___p1 = __XSK_READ_ONCE(*p);			\
> -		asm volatile("" : : : "memory");			\
> -		___p1;							\
> -	})
> -#elif defined(__aarch64__)
> -# define libbpf_smp_store_release(p, v)					\
> -		asm volatile ("stlr %w1, %0" : "=Q" (*p) : "r" (v) : "memory")
> -# define libbpf_smp_load_acquire(p)					\
> -	({								\
> -		typeof(*p) ___p1;					\
> -		asm volatile ("ldar %w0, %1"				\
> -			      : "=r" (___p1) : "Q" (*p) : "memory");	\
> -		___p1;							\
> -	})
> -#elif defined(__riscv)
> -# define libbpf_smp_store_release(p, v)					\
> -	do {								\
> -		asm volatile ("fence rw,w" : : : "memory");		\
> -		__XSK_WRITE_ONCE(*p, v);				\
> -	} while (0)
> -# define libbpf_smp_load_acquire(p)					\
> -	({								\
> -		typeof(*p) ___p1 = __XSK_READ_ONCE(*p);			\
> -		asm volatile ("fence r,rw" : : : "memory");		\
> -		___p1;							\
> -	})
> -#endif
> -
> -#ifndef libbpf_smp_store_release
> -#define libbpf_smp_store_release(p, v)					\
> -	do {								\
> -		__sync_synchronize();					\
> -		__XSK_WRITE_ONCE(*p, v);				\
> -	} while (0)
> -#endif
> -
> -#ifndef libbpf_smp_load_acquire
> -#define libbpf_smp_load_acquire(p)					\
> -	({								\
> -		typeof(*p) ___p1 = __XSK_READ_ONCE(*p);			\
> -		__sync_synchronize();					\
> -		___p1;							\
> -	})
> -#endif
> -
> -/* LIBRARY INTERNAL -- END */
> -
>   /* Do not access these members directly. Use the functions below. */
>   #define DEFINE_XSK_RING(name) \
>   struct name { \
> @@ -168,7 +97,7 @@ static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32 nb)
>   	 * this function. Without this optimization it whould have been
>   	 * free_entries = r->cached_prod - r->cached_cons + r->size.
>   	 */
> -	r->cached_cons = libbpf_smp_load_acquire(r->consumer);
> +	r->cached_cons = __atomic_load_n(r->consumer, __ATOMIC_ACQUIRE);
>   	r->cached_cons += r->size;
>   
>   	return r->cached_cons - r->cached_prod;
> @@ -179,7 +108,7 @@ static inline __u32 xsk_cons_nb_avail(struct xsk_ring_cons *r, __u32 nb)
>   	__u32 entries = r->cached_prod - r->cached_cons;
>   
>   	if (entries == 0) {
> -		r->cached_prod = libbpf_smp_load_acquire(r->producer);
> +		r->cached_prod = __atomic_load_n(r->producer, __ATOMIC_ACQUIRE);
>   		entries = r->cached_prod - r->cached_cons;
>   	}
>   
> @@ -202,7 +131,7 @@ static inline void xsk_ring_prod__submit(struct xsk_ring_prod *prod, __u32 nb)
>   	/* Make sure everything has been written to the ring before indicating
>   	 * this to the kernel by writing the producer pointer.
>   	 */
> -	libbpf_smp_store_release(prod->producer, *prod->producer + nb);
> +	__atomic_store_n(prod->producer, *prod->producer + nb, __ATOMIC_RELEASE);
>   }
>   
>   static inline __u32 xsk_ring_cons__peek(struct xsk_ring_cons *cons, __u32 nb, __u32 *idx)
> @@ -227,8 +156,7 @@ static inline void xsk_ring_cons__release(struct xsk_ring_cons *cons, __u32 nb)
>   	/* Make sure data has been read before indicating we are done
>   	 * with the entries by updating the consumer pointer.
>   	 */
> -	libbpf_smp_store_release(cons->consumer, *cons->consumer + nb);
> -
> +	__atomic_store_n(cons->consumer, *cons->consumer + nb, __ATOMIC_RELEASE);
>   }
>   
>   static inline void *xsk_umem__get_data(void *umem_area, __u64 addr)
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ