Re: [dpdk-dev] [PATCH v1 3/5] bpf: introduce basic RX/TX BPF filters

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

-----Original Message-----
> Date: Fri, 9 Mar 2018 16:42:03 +0000
> From: Konstantin Ananyev <konstantin.ananyev@intel.com>
> To: dev@dpdk.org
> CC: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Subject: [dpdk-dev] [PATCH v1 3/5] bpf: introduce basic RX/TX BPF filters
> X-Mailer: git-send-email 1.7.0.7
> 
> Introduce API to install BPF based filters on ethdev RX/TX path.
> Current implementation is pure SW one, based on ethdev RX/TX
> callback mechanism.
> 
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
>  lib/librte_bpf/Makefile            |   2 +
>  lib/librte_bpf/bpf_pkt.c           | 524 +++++++++++++++++++++++++++++++++++++
>  lib/librte_bpf/rte_bpf_ethdev.h    |  50 ++++
>  lib/librte_bpf/rte_bpf_version.map |   4 +
>  4 files changed, 580 insertions(+)
>  create mode 100644 lib/librte_bpf/bpf_pkt.c
>  create mode 100644 lib/librte_bpf/rte_bpf_ethdev.h
> 
> diff --git a/lib/librte_bpf/Makefile b/lib/librte_bpf/Makefile
> +
> +/*
> + * information about all installed BPF rx/tx callbacks
> + */
> +
> +struct bpf_eth_cbi {
> +	uint32_t use;    /*usage counter */
> +	void *cb;        /* callback handle */
> +	struct rte_bpf *bpf;
> +	struct rte_bpf_jit jit;
> +} __rte_cache_aligned;
> +
> +/*
> + * 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
> +
> +static struct bpf_eth_cbi rx_cbi[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];
> +static struct bpf_eth_cbi tx_cbi[RTE_MAX_ETHPORTS][RTE_MAX_QUEUES_PER_PORT];

How about allocating this memory from huge page?

> +
> +/*
> + * 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();

This is an full barrier on non x86. How about a light version of this
logic? See below.

> +}
> +
> +/*
> + * Marks given callback list 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++;
> +}
> +
> +/*
> + * Waits till datapath finished using given callback.
> + */
> +static void
> +bpf_eth_cbi_wait(const struct bpf_eth_cbi *cbi)
> +{
> +	uint32_t nuse, puse;
> +
> +	/* make sure all previous loads and stores are completed */
> +	rte_smp_mb();
> +

Read conjunction with below change

#if 0
> +	puse = cbi->use;
> +
> +	/* in use, busy wait till current RX/TX iteration is finished */
> +	if ((puse & BPF_ETH_CBI_INUSE) != 0) {
> +		do {
> +			rte_pause();
> +			rte_compiler_barrier();
> +			nuse = cbi->use;
> +		} while (nuse == puse);
> +	}
#else
	cbi->cb = NULL;
	while (likely(cb->done != 1)) {
		rte_pause();
		rte_smb_rmb();
	}

or any other logic using flag to wait until callback completes.
#endif

> +}
> +
> +
> +/*
> + * RX/TX callbacks for raw data bpf.
> + */
> +
> +static uint16_t
> +bpf_rx_callback_vm(__rte_unused uint16_t port, __rte_unused uint16_t queue,
> +	struct rte_mbuf *pkt[], uint16_t nb_pkts,
> +	__rte_unused uint16_t max_pkts, void *user_param)
> +{
> +	struct bpf_eth_cbi *cbi;
> +	uint16_t rc;
> +
> +	cbi = user_param;
> +

Read conjunction with above change

#if 0
> +	bpf_eth_cbi_inuse(cbi);
> +	rc = (cbi->cb != NULL) ?
> +		pkt_filter_vm(cbi->bpf, pkt, nb_pkts, 1) :
> +		nb_pkts;
> +	bpf_eth_cbi_unuse(cbi);
#else
	if (likely(cbi->cb != NULL))
		return pkt_filter_vm(cbi->bpf, pkt, nb_pkts, 1) :
	else {
		cbi->done = 1;
		rte_smb_wmb();
		return nb_pkts;
	}
#endif

> +	return rc;
> +}
> +