The rx/tx callback use counter relied on rte_smp_mb() and rte_smp_rmb(), which are deprecated. Convert it to C11 atomics.
The counter is odd while the datapath is inside the callback and even otherwise. Only one thread at a time does rx/tx burst on a queue, so the counter has a single writer and is updated with a plain load and store. - bpf_eth_cbi_inuse(): keep a full barrier after the store. The datapath stores the counter then loads cb; unload stores cb then loads the counter. A seq_cst fence on each side makes sure at least one of them sees the other's store. - bpf_eth_cbi_unuse(): use a release store instead of a read barrier before the increment. - bpf_eth_cbi_wait(): use acquire loads to pair with that store, so rte_bpf_destroy() is ordered after the datapath's last use of the program. - bpf_eth_cbi_unload(): drop the barrier; bpf_eth_cbi_wait() starts with one. With enable_stdatomic the old cbi->use++ was a locked add. On x86 that build now does one locked instruction per burst instead of three. Signed-off-by: Stephen Hemminger <[email protected]> --- lib/bpf/bpf_pkt.c | 45 ++++++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 15 deletions(-) diff --git a/lib/bpf/bpf_pkt.c b/lib/bpf/bpf_pkt.c index f072fdaaed..95f76e47c7 100644 --- a/lib/bpf/bpf_pkt.c +++ b/lib/bpf/bpf_pkt.c @@ -36,10 +36,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 /* @@ -74,15 +70,34 @@ static struct bpf_eth_cbh tx_cbh = { .type = BPF_ETH_TX, }; +/* + * Removing an rx/tx callback involves two steps (similar to RCU). + * The callback is first removed from the ethdev queue so that + * it will not be used by later burst. + * But the callback may still be in process or the core may have + * raced and seen the old callback. + * The use counter is used to indicate that it is not safe to + * free the BPF program yet. + * The datapath makes the counter odd on entry and even on exit. + * During unload, if the counter is odd then it indicates + * we must wait. + * This assumes that only one thread at a time may do rx/tx burst + * on a queue. Therefore the counter has a single writer and the + * increment need not be atomic. + */ + /* * Marks given callback as used by datapath. */ static __rte_always_inline void bpf_eth_cbi_inuse(struct bpf_eth_cbi *cbi) { - cbi->use++; - /* make sure no store/load reordering could happen */ - rte_smp_mb(); + rte_atomic_store_explicit(&cbi->use, + rte_atomic_load_explicit(&cbi->use, rte_memory_order_relaxed) + 1, + rte_memory_order_relaxed); + + /* full barrier: count must be visible before cb is read */ + rte_atomic_thread_fence(rte_memory_order_seq_cst); } /* @@ -91,9 +106,10 @@ 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++; + /* release: pairs with the acquire bpf_eth_cbi_wait() */ + rte_atomic_store_explicit(&cbi->use, + rte_atomic_load_explicit(&cbi->use, rte_memory_order_relaxed) + 1, + rte_memory_order_release); } /* @@ -104,15 +120,15 @@ 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(); + /* full barrier: cleared cb must be visible before counter is read */ + rte_atomic_thread_fence(rte_memory_order_seq_cst); - puse = cbi->use; + puse = rte_atomic_load_explicit(&cbi->use, rte_memory_order_acquire); /* 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); + UINT32_MAX, !=, puse, rte_memory_order_acquire); } } @@ -439,7 +455,6 @@ bpf_eth_cbi_unload(struct bpf_eth_cbi *bc) { /* mark this cbi as empty */ bc->cb = NULL; - rte_smp_mb(); /* make sure datapath doesn't use bpf anymore, then destroy bpf */ bpf_eth_cbi_wait(bc); -- 2.53.0
