[PATCH v2 5/9] bpf: simplify callback inuse tracking to boolean flag

[Konstantin Ananyev]

> The even/odd usage counter semantics in struct bpf_eth_cbi was
> originally used to detect when in-flight packet callbacks complete.
> However, with at most one thread running the callback per port/queue
> pair and no risk of the ABA problem, a boolean flag (true for in use,
> false for idle) suffices.
>
> Change member use to bool and replace the counter increment in the
> datapath with stores of true and false. Simplify bpf_eth_cbi_wait()
> to a direct pause loop waiting until the flag is false, removing the
> now unnecessary RTE_WAIT_UNTIL_MASKED macro and casts. Update the
> documentation and synchronization sequence diagram accordingly.

As I explained in previous mail, it is not about even/odd.
It is about waiting while current 'quiscent' period will end.
By 'simplifying' that to use just boll values (0/1) you introduce
a live-lock in CP: it can be blocked for very long time waiting
while DP thread will keep processing packets.
So mu vote is: NACK.

As I said before - probably the best option to improving current BPF CB 
mechanism,
would be to make ethedev CB to obey RCU-like behavior.
Konstantin


>
> Signed-off-by: Marat Khalili <qm2k21 at gmail.com>
> ---
>   lib/bpf/bpf_pkt.c | 49 ++++++++++++++++++-----------------------------
>   1 file changed, 19 insertions(+), 30 deletions(-)
>
> diff --git a/lib/bpf/bpf_pkt.c b/lib/bpf/bpf_pkt.c
> index b1e25e4e7c..429feb76ad 100644
> --- a/lib/bpf/bpf_pkt.c
> +++ b/lib/bpf/bpf_pkt.c
> @@ -6,6 +6,7 @@
>   #include <string.h>
>   #include <errno.h>
>   #include <stdint.h>
> +#include <stdbool.h>
>   
>   #include <sys/queue.h>
>   
> @@ -36,12 +37,12 @@
>    * 2. Member `cb` of struct `bpf_eth_cbi`;
>    * 3. Member `use` of struct `bpf_eth_cbi`;
>    *
> - * BPF program is only executed if the first two are set, and increments `use` at the start and
> - * end of the callback execution so that in the safe state its value is even.
> + * BPF program is only executed if the first two are set, and sets `use` to true at the start and
> + * to false at the end of the callback execution.
>    * This relies on only one thread calling the per-port/queue callback at a time.
>    *
>    * Unload processing starts with clearing the first two entities,
> - * then waits for `use` to become even before destroying the BPF program.
> + * then waits for `use` to become false before destroying the BPF program.
>    * The whole operation is protected by a control-plane-only spinlock (not shown on the diagram).
>    *
>    * Typical sequence of synchronization actions between CP and DP on unload is shown below:
> @@ -52,7 +53,7 @@
>    *     |                      |               |              |              |                |
>    *     |                      |<---------------------- fetch callback -----------------------|
>    *     |                      |               |              |              |                |
> - *     |                      |               |<------------- increment to odd --------------|
> + *     |                      |               |<------------ store true ---------------------|
>    *     |                      |               |              |              |                |
>    *     |-- remove callback -->|               |              |              |       rte_smp_mb()
>    *     |                      |               |              |              |                |
> @@ -62,17 +63,17 @@
>    *     |                      |               |              |              |                |
>    *   rte_smp_mb()             |               |              |              |                |
>    *     |                      |               |              |              |                |
> - *     +-if use is odd        |               |              |              | if cb not NULL-+
> + *     +-while use is true    |               |              |              | if cb not NULL-+
>    *     | |                    |               |              |              |              | |
> - *     | +------------ fetch odd ------------>|              |              |<-- execute --+ |
> + *     | +----------- fetch true ------------>|              |              |<-- execute --+ |
>    *     | |                    |               |              |              |  BPF program | |
> - *     | +-while use is same  |               |              |              |              +-+
> - *     | | |                  |               |              |              |                |
> - *     | | +---------- fetch odd ------------>|              |              |      rte_smp_rmb()
> - *     | | |                  |               |              |              |                |
> - *     +-+-+                  |               |<------------- increment to even -------------|
> + *     | |                    |               |              |              |              +-+
> + *     | |                    |               |              |              |                |
> + *     | |                    |               |              |              |      rte_smp_rmb()
> + *     | |                    |               |              |              |                |
> + *     +-+                    |               |<------------ store false --------------------|
>    *     |                      |               |              |              |                |
> - *     |------------- fetch even ------------>|              |              |                |
> + *     |------------ fetch false ------------>|              |              |                |
>    *     |                      |               |              |              |                |
>    *     |----------------- destroy BPF program and cleanup ----------------->|                |
>    *     |                      |               |              |              |                |
> @@ -87,7 +88,7 @@
>    */
>   struct __rte_cache_aligned bpf_eth_cbi {
>   	/* used by both data & control path */
> -	RTE_ATOMIC(uint32_t) use;    /*usage counter */
> +	RTE_ATOMIC(bool) use;    /* usage flag: true when in use, false otherwise */
>   	const struct rte_eth_rxtx_callback *cb;  /* callback handle */
>   	struct rte_bpf *bpf;
>   	struct rte_bpf_jit jit;
> @@ -97,12 +98,6 @@ struct __rte_cache_aligned bpf_eth_cbi {
>   	uint16_t queue;
>   };
>   
> -/*
> - * Odd number means that callback is used by datapath.
> - * Even number means that callback is not used by datapath.
> - */
> -#define BPF_ETH_CBI_INUSE  1
> -
>   /*
>    * List to manage RX/TX installed callbacks.
>    */
> @@ -141,20 +136,20 @@ static struct bpf_eth_cbh tx_cbh = {
>   static __rte_always_inline void
>   bpf_eth_cbi_inuse(struct bpf_eth_cbi *cbi)
>   {
> -	cbi->use++;
> +	cbi->use = true;
>   	/* make sure no store/load reordering could happen */
>   	rte_smp_mb();
>   }
>   
>   /*
> - * Marks given callback list as not used by datapath.
> + * Marks given callback as not used by datapath.
>    */
>   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++;
> +	cbi->use = false;
>   }
>   
>   /*
> @@ -163,18 +158,12 @@ bpf_eth_cbi_unuse(struct bpf_eth_cbi *cbi)
>   static void
>   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();
>   
> -	puse = cbi->use;
> -
>   	/* in use, busy wait till current RX/TX iteration is finished */
> -	if ((puse & BPF_ETH_CBI_INUSE) != 0) {
> -		RTE_WAIT_UNTIL_MASKED((__rte_atomic uint32_t *)(uintptr_t)&cbi->use,
> -			UINT32_MAX, !=, puse, rte_memory_order_relaxed);
> -	}
> +	while (cbi->use)
> +		rte_pause();
>   }
>   
>   static void