[dpdk-dev] [PATCH v3 09/12] app/eventdev: add pipeline queue worker functions

Van Haaren, Harry harry.van.haaren at intel.com
Thu Jan 11 13:17:38 CET 2018


> From: Pavan Nikhilesh [mailto:pbhagavatula at caviumnetworks.com]
> Sent: Wednesday, January 10, 2018 8:17 PM
> To: Van Haaren, Harry <harry.van.haaren at intel.com>;
> jerin.jacob at caviumnetworks.com; santosh.shukla at caviumnetworks.com; Eads,
> Gage <gage.eads at intel.com>; hemant.agrawal at nxp.com; nipun.gupta at nxp.com; Ma,
> Liang J <liang.j.ma at intel.com>
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v3 09/12] app/eventdev: add pipeline queue
> worker functions
> 
> On Wed, Jan 10, 2018 at 04:53:53PM +0000, Van Haaren, Harry wrote:
> > Replying to self...
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Van Haaren, Harry
> > > Sent: Wednesday, January 10, 2018 4:45 PM
> > > To: Pavan Nikhilesh <pbhagavatula at caviumnetworks.com>;
> > > jerin.jacob at caviumnetworks.com; santosh.shukla at caviumnetworks.com; Eads,
> > > Gage <gage.eads at intel.com>; hemant.agrawal at nxp.com; nipun.gupta at nxp.com;
> Ma,
> > > Liang J <liang.j.ma at intel.com>
> > > Cc: dev at dpdk.org
> > > Subject: Re: [dpdk-dev] [PATCH v3 09/12] app/eventdev: add pipeline
> queue
> > > worker functions
> > >
> > > > From: Pavan Nikhilesh [mailto:pbhagavatula at caviumnetworks.com]
> > > > Sent: Wednesday, January 10, 2018 2:52 PM
> > > > To: jerin.jacob at caviumnetworks.com; santosh.shukla at caviumnetworks.com;
> Van
> > > > Haaren, Harry <harry.van.haaren at intel.com>; Eads, Gage
> > > > <gage.eads at intel.com>; hemant.agrawal at nxp.com; nipun.gupta at nxp.com;
> Ma,
> > > > Liang J <liang.j.ma at intel.com>
> > > > Cc: dev at dpdk.org; Pavan Nikhilesh <pbhagavatula at caviumnetworks.com>
> > > > Subject: [dpdk-dev] [PATCH v3 09/12] app/eventdev: add pipeline queue
> > > worker
> > > > functions
> > > >
> > >
> > > <snip>
> > >
> > >
> > > > +static __rte_always_inline void
> > > > +pipeline_tx_pkt_safe(struct rte_mbuf *mbuf)
> > > > +{
> > > > +	while (rte_eth_tx_burst(mbuf->port, 0, &mbuf, 1) != 1)
> > > > +		rte_pause();
> > > > +}
> > >
> > > re safe, see comment below
> > >
> > > > +
> > > > +static __rte_always_inline void
> > > > +pipeline_tx_pkt_unsafe(struct rte_mbuf *mbuf, struct test_pipeline
> *t)
> > > > +{
> > > > +	rte_spinlock_t *lk = &t->tx_lk[mbuf->port];
> > > > +
> > > > +	rte_spinlock_lock(lk);
> > > > +	pipeline_tx_pkt_safe(mbuf);
> > > > +	rte_spinlock_unlock(lk);
> > > > +}
> > >
> > > IIRC usually the "Safe" version of a function has extra
> locks/protection,
> > > while the "normal" version has better performance, but less-error-
> checking.
> > >
> > > Here, the "unsafe" function does the extra locking. If looking from the
> HW
> > > POV, that makes sense, but I think its inverted from most existing
> code...
> > >
> > > Happy to be proved wrong here .. ?
> > >
> > > <snip>
> >
> >
> > Thinking a little more about this, also in light of patch 11/12 of this
> series.
> >
> > The code here has a "safe" and "unsafe" version of TX. This involves
> adding a spinlock inside the code, which is being locked/unlocked before
> doing the actual TX action.
> >
> > I don't understand why this is necessary? DPDK's general stance on locking
> for data-path is DPDK functions do not provide locks, and that application
> level must implement thread-synchronization if it is required.
> >
> > In this case, the app/eventdev can be considered an App, but I don't like
> the idea of providing a sample application and code that duplicates core
> functionality with safe/unsafe versions..
> >
> 
> Some PMD's (net/octeontx) have capability to do multi-thread safe Tx where
> no
> thread-synchronization is required. This is exposed via the offload flag
> 'DEV_TX_OFFLOAD_MT_LOCKFREE'.

Yes understood.


> So, the _safe Tx functions are selected based on the above offload
> capability
> and when the capability is absent _unsafe Tx functions are selected i.e.
> synchronized Tx via spin locks based on the Egress port id.


This part changes the current behavior of the sample app.

Currently there is a (SINGLE_LINK | ATOMIC) stage at the end of the pipeline, which performs this "many-to-one" action, allowing a single core to dequeue all TX traffic, and perform the TX operation in a lock-free manner.

Changing this to a locking mechanism is going to hurt performance on platforms that do not support TX_OFFLOAD_MT_LOCKFREE.

In my opinion, the correct fix is to alter the overall pipeline, and always use lockless TX. Examples below;

NO TX_OFFLOAD_MT_LOCKFREE:

   Eth RX adapter -> stage 1 -> stage 2...(N-1) -> stage N -> stage TX (Atomic | SINGLE_LINK) -> eth TX


WITH TX_OFFLOAD_MT_LOCKFREE:

   Eth RX adapter -> stage 1 -> stage 2...(N-1) -> stage N -> eth TX MT Capable


By configuring the pipeline based on MT_OFFLOAD_LOCKFREE capability flag, and adding the SINGLE_LINK at the end if required, we can support both models without resorting to locked TX functions.

I think this will lead to a cleaner and more performant solution.

<snip>


More information about the dev mailing list