> From: Pavan Nikhilesh [mailto:pbhagavat...@caviumnetworks.com] > Sent: Wednesday, January 10, 2018 8:17 PM > To: Van Haaren, Harry <harry.van.haa...@intel.com>; > jerin.ja...@caviumnetworks.com; santosh.shu...@caviumnetworks.com; Eads, > Gage <gage.e...@intel.com>; hemant.agra...@nxp.com; nipun.gu...@nxp.com; Ma, > Liang J <liang.j...@intel.com> > Cc: dev@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-boun...@dpdk.org] On Behalf Of Van Haaren, Harry > > > Sent: Wednesday, January 10, 2018 4:45 PM > > > To: Pavan Nikhilesh <pbhagavat...@caviumnetworks.com>; > > > jerin.ja...@caviumnetworks.com; santosh.shu...@caviumnetworks.com; Eads, > > > Gage <gage.e...@intel.com>; hemant.agra...@nxp.com; nipun.gu...@nxp.com; > Ma, > > > Liang J <liang.j...@intel.com> > > > Cc: dev@dpdk.org > > > Subject: Re: [dpdk-dev] [PATCH v3 09/12] app/eventdev: add pipeline > queue > > > worker functions > > > > > > > From: Pavan Nikhilesh [mailto:pbhagavat...@caviumnetworks.com] > > > > Sent: Wednesday, January 10, 2018 2:52 PM > > > > To: jerin.ja...@caviumnetworks.com; santosh.shu...@caviumnetworks.com; > Van > > > > Haaren, Harry <harry.van.haa...@intel.com>; Eads, Gage > > > > <gage.e...@intel.com>; hemant.agra...@nxp.com; nipun.gu...@nxp.com; > Ma, > > > > Liang J <liang.j...@intel.com> > > > > Cc: dev@dpdk.org; Pavan Nikhilesh <pbhagavat...@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>