RE: [PATCH v2 01/33] bpf: replace deprecated SMP barriers with C11 fences

[Konstantin Ananyev <konstantin.ananyev@huawei.com>]

> The use counter is a seqcount, not a reference count: odd means the
> datapath is inside the callback, even means it is not. The two
> barriers around it play different roles.
> 
> In bpf_eth_cbi_inuse() the counter goes odd and the following loads
> of cb, bpf and jit must not be hoisted above that store. Ordering a
> store against later loads needs a full barrier, so rte_smp_mb()
> becomes a seq_cst thread fence.
> 
> In bpf_eth_cbi_unuse() the read barrier becomes an acquire fence.
> What must not happen there is the critical section loads sinking
> past the store that makes the counter even, which is load/store
> ordering, and that is what a standalone acquire fence provides.

Hmm.., as I remember 'acquire' serves as a hoist barrier.
For sink barrier we do need 'release' semantics, no? 

> acq_rel would additionally order prior stores, but the read side
> only loads from the cbi and so has nothing to publish; it would
> only upgrade dmb ishld to dmb ish on the datapath for no benefit.
> 
> Same code generated on x86 and arm64.
> 
> Use relaxed loads and stores for the counter itself. With
> enable_stdatomic, the plain increment of an RTE_ATOMIC() field
> compiled to a seq_cst add, i.e. two locked operations per burst.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/bpf/bpf_pkt.c | 25 +++++++++++++++++--------
>  1 file changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/bpf/bpf_pkt.c b/lib/bpf/bpf_pkt.c
> index f072fdaaed..c7b1019239 100644
> --- a/lib/bpf/bpf_pkt.c
> +++ b/lib/bpf/bpf_pkt.c
> @@ -80,9 +80,11 @@ static struct bpf_eth_cbh tx_cbh = {
>  static __rte_always_inline void
>  bpf_eth_cbi_inuse(struct bpf_eth_cbi *cbi)
>  {
> -	cbi->use++;
> +	rte_atomic_store_explicit(&cbi->use,
> +		rte_atomic_load_explicit(&cbi->use, rte_memory_order_relaxed)
> + 1,
> +		rte_memory_order_relaxed);

if we are using atomic_store anyway, might be we can unite it with explicit barrier below? i.e:
rte_atomic_store_explicit(&cbi->use,
	rte_atomic_load_explicit(&cbi->use, rte_memory_order_relaxed) + 1,
	rte_memory_order_seq_cst);
?	

I think Marat also mentioned such possibility here.

>  	/* make sure no store/load reordering could happen */
> -	rte_smp_mb();
> +	rte_atomic_thread_fence(rte_memory_order_seq_cst);
>  }
> 
>  /*
> @@ -91,9 +93,16 @@ bpf_eth_cbi_inuse(struct bpf_eth_cbi *cbi)
>  static __rte_always_inline void
>  bpf_eth_cbi_unuse(struct bpf_eth_cbi *cbi)
>  {
> -	/* make sure all previous loads are completed */
> -	rte_smp_rmb();
> -	cbi->use++;
> +	/*
> +	 * Make sure all previous loads are completed before the counter
> +	 * goes even. Acquire is enough: the read side only loads from the
> +	 * cbi, so there are no stores to publish and acq_rel would just
> +	 * cost a stronger barrier on weakly ordered CPUs.
> +	 */
> +	rte_atomic_thread_fence(rte_memory_order_acquire);

As it is a sink barrier, I think it should be 'release'.
Again we probably unite it with atomic store below.
rte_atomic_store_explicit(&cbi->use,
	 rte_atomic_load_explicit(&cbi->use, rte_memory_order_relaxed) + 1,
	rte_memory_order_release);

> +	rte_atomic_store_explicit(&cbi->use,
> +		rte_atomic_load_explicit(&cbi->use, rte_memory_order_relaxed)
> + 1,
> +		rte_memory_order_relaxed);
>  }
> 
>  /*
> @@ -105,9 +114,9 @@ bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi)
>  	uint32_t puse;
> 
>  	/* make sure all previous loads and stores are completed */
> -	rte_smp_mb();
> +	rte_atomic_thread_fence(rte_memory_order_seq_cst);
> 
> -	puse = cbi->use;
> +	puse = rte_atomic_load_explicit(&cbi->use, rte_memory_order_relaxed);
> 
>  	/* in use, busy wait till current RX/TX iteration is finished */
>  	if ((puse & BPF_ETH_CBI_INUSE) != 0) {
> @@ -439,7 +448,7 @@ bpf_eth_cbi_unload(struct bpf_eth_cbi *bc)
>  {
>  	/* mark this cbi as empty */
>  	bc->cb = NULL;
> -	rte_smp_mb();
> +	rte_atomic_thread_fence(rte_memory_order_seq_cst);

In fact, that one seems unnecessary here (and smb_mb() too).
As we call bpf_eth_cbi_wait() straight after that, and first thing it does - calls smb_mb() itself.
So we probably can just safely remove it here.

> 
>  	/* make sure datapath doesn't use bpf anymore, then destroy bpf */
>  	bpf_eth_cbi_wait(bc);
> --
> 2.53.0