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

[Van Haaren, Harry]

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..

Hope I'm making some sense here..


> 
> > +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>