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

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

Hi Jerin,

> > >
> > > > +/*
> > > > + * 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...
> 
> If I understand it correctly, bpf_eth_cbi_wait() is used to _wait_ until
> eBPF program exits? Right?

Kind off, but not only. 
After  bpf_eth_cbi_wait() finishes it is guaranteed that data-path wouldn't try
to access the resources associated with given bpf_eth_cbi (bpf, jit), so we
can proceed with freeing them. 

> . Instead of using bpf_eth_cbi_[un]use()
> scheme which involves the barrier. How about,
> 
> in bpf_eth_cbi_wait()
> {
> 
> memset the EBPF "program memory" with 0x95 value. Which is an "exit" and
> "return r0" EPBF opcode, Which makes program to terminate by it own
> as on 0x95 instruction, CPU decodes and it gets out from EPBF program.
> 
> }
> 
> In jitted case, it is not 0x95 instruction, which will be an arch
> specific instructions, We can have arch abstraction to generated
> such instruction for "exit" opcode. And use common code to fill the instructions
> to exit from EPBF program provided by arch code.
> 
> Does that makes sense?

There is no much point in doing it.
What we need is a guarantee that after some point data-path wouldn't try to access
given bpf context, so we can destroy it.
Konstantin

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