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

["Ananyev, Konstantin" <konstantin.ananyev@intel.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?

Yep, in v2 will switch to using rte_malloc() for cbi allocation.

> 
> > +
> > +/*
> > + * 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.

This is a full barrier on x86 too.
Unfortunately I think it is unavoidable, though open for suggestions.

> 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

I thought about it, but such approach makes control-path function progress dependent
on simultaneous invocation of data-path functions.
In some cases it would cause control-path to hang.
Let say there is no traffic for that port/queue (i.e. tx_burst() wouldn't be called),
or user invokes control-path and data-path function from the same thread, i.e:

rte_bpf_eth_rx_elf_load(port, queue, ...);
....
rte_eth_rx_burst(port, queue, ...);
...
rte_bpf_eth_rx_unload(port,queue);

Konstantin

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