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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Jerin,

> 
> Hi Konstantin,
> 
> > +/*
> > + * 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();
> > +}
> > +
> > +/*
> > + * 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();
> 
> We earlier discussed this barrier. Will following scheme works out to
> fix the bpf_eth_cbi_wait() without cbi->use scheme?
> 
> #ie. We need to exit from jitted or interpreted code irrespective of its
> state. IMO, We can do that by an _arch_ specific function to fill jitted  memory with
> "exit" opcode(value:0x95, exit, return r0),so that above code needs to be come out i n anycase,
> on next instruction execution. I know, jitted memory is read-only in your
> design, I think, we can change the permission to "write" to the fill
> "exit" opcode(both jitted or interpreted case) for termination.
>
> What you think?

Not sure I understand your proposal...
Are you suggesting to change bpf_exec() and bpf_jit() to make them execute sync primitives in an arch specific manner?
But some users probably will use bpf_exec/jitted program in the environment that wouldn't require such synchronization.
For these people it would be just unnecessary slowdown.

If you are looking for a ways to replace 'smp_rmb'  in bpf_eth_cbi_unuse() with something arch specific, then
I can make cbi_inuse/cbi_unuse - arch specific with keeping current implementation as generic one.
Would that help?

Konstantin

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