From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pavan Nikhilesh Subject: Re: [PATCH v3 09/12] app/eventdev: add pipeline queue worker functions Date: Thu, 11 Jan 2018 01:47:10 +0530 Message-ID: <20180110201710.3uolm2hwzwcowoif@Pavan-LT> References: <20171130072406.15605-1-pbhagavatula@caviumnetworks.com> <20180110145144.28403-1-pbhagavatula@caviumnetworks.com> <20180110145144.28403-9-pbhagavatula@caviumnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org To: "Van Haaren, Harry" , "jerin.jacob@caviumnetworks.com" , "santosh.shukla@caviumnetworks.com" , "Eads, Gage" , "hemant.agrawal@nxp.com" , "nipun.gupta@nxp.com" , "Ma, Liang J" Return-path: Received: from NAM03-DM3-obe.outbound.protection.outlook.com (mail-dm3nam03on0062.outbound.protection.outlook.com [104.47.41.62]) by dpdk.org (Postfix) with ESMTP id 9602B1B010 for ; Wed, 10 Jan 2018 21:17:35 +0100 (CET) Content-Disposition: inline In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Jan 10, 2018 at 04:53:53PM +0000, Van Haaren, Harry wrote: > Replying to self... > > > -----Original Message----- > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Van Haaren, Harry > > Sent: Wednesday, January 10, 2018 4:45 PM > > To: Pavan Nikhilesh ; > > jerin.jacob@caviumnetworks.com; santosh.shukla@caviumnetworks.com; Eads, > > Gage ; hemant.agrawal@nxp.com; nipun.gupta@nxp.com; Ma, > > Liang J > > Cc: dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH v3 09/12] app/eventdev: add pipeline queue > > worker functions > > > > > From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com] > > > Sent: Wednesday, January 10, 2018 2:52 PM > > > To: jerin.jacob@caviumnetworks.com; santosh.shukla@caviumnetworks.com; Van > > > Haaren, Harry ; Eads, Gage > > > ; hemant.agrawal@nxp.com; nipun.gupta@nxp.com; Ma, > > > Liang J > > > Cc: dev@dpdk.org; Pavan Nikhilesh > > > Subject: [dpdk-dev] [PATCH v3 09/12] app/eventdev: add pipeline queue > > worker > > > functions > > > > > > > > > > > > > > +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 .. ? > > > > > > > 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. > > > >