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'.
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.
The patch 5/12 has the below check to see if the connected ethernet dev(s) have
the capability to do thread safe Tx
+ for (i = 0; i < rte_eth_dev_count(); i++) {
+ struct rte_eth_dev_info dev_info;
....
+ rte_eth_dev_info_get(i, &dev_info);
+ mt_state = !(dev_info.tx_offload_capa &
+ DEV_TX_OFFLOAD_MT_LOCKFREE);
....
+ t->mt_unsafe |= mt_state;
+ }
Based on the value of t->mt_unsafe the appropriate worker function is selected.
> Hope I'm making some sense here..
>
Hope this clears thing up.
Cheers,
Pavan.
>
> >
> > > +static int
> > > +pipeline_queue_worker_single_stage_safe(void *arg)
> > > +{
> > > + struct worker_data *w = arg;
> > > + struct test_pipeline *t = w->t;
> > > + const uint8_t dev = w->dev_id;
> > > + const uint8_t port = w->port_id;
> > > + struct rte_event ev;
> > > +
> > > + while (t->done == false) {
> > > + uint16_t event = rte_event_dequeue_burst(dev, port, &ev, 1, 0);
> > > +
> > > + if (!event) {
> > > + rte_pause();
> > > + continue;
> > > + }
> > > +
> > > + if (ev.sched_type == RTE_SCHED_TYPE_ATOMIC) {
> > > + pipeline_tx_pkt_safe(ev.mbuf);
> >
> > I guess that means that the functions where they're used are inverted in
> > name too.
> >
> > <snip>
