From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerin Jacob Subject: Re: [PATCH 1/6] ethdev: add Tx preparation Date: Fri, 9 Sep 2016 11:28:17 +0530 Message-ID: <20160909055816.GA6012@localhost.localdomain> References: <1472228578-6980-1-git-send-email-tomaszx.kulasek@intel.com> <1472228578-6980-2-git-send-email-tomaszx.kulasek@intel.com> <20160908072845.GA7333@localhost.localdomain> <3042915272161B4EB253DA4D77EB373A14F050BA@IRSMSX102.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: "dev@dpdk.org" To: "Kulasek, TomaszX" Return-path: Received: from NAM01-BN3-obe.outbound.protection.outlook.com (mail-bn3nam01on0055.outbound.protection.outlook.com [104.47.33.55]) by dpdk.org (Postfix) with ESMTP id 655C52C0A for ; Fri, 9 Sep 2016 07:58:44 +0200 (CEST) Content-Disposition: inline In-Reply-To: <3042915272161B4EB253DA4D77EB373A14F050BA@IRSMSX102.ger.corp.intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, Sep 08, 2016 at 04:09:05PM +0000, Kulasek, TomaszX wrote: > Hi Jerin, Hi TomaszX, > > > -----Original Message----- > > From: Jerin Jacob [mailto:jerin.jacob@caviumnetworks.com] > > Sent: Thursday, September 8, 2016 09:29 > > To: Kulasek, TomaszX > > Cc: dev@dpdk.org > > Subject: Re: [dpdk-dev] [PATCH 1/6] ethdev: add Tx preparation > > > > [...] > > > > +static inline uint16_t > > > +rte_eth_tx_prep(uint8_t port_id, uint16_t queue_id, struct rte_mbuf > > **tx_pkts, > > > + uint16_t nb_pkts) > > > +{ > > > + struct rte_eth_dev *dev = &rte_eth_devices[port_id]; > > > + > > > + if (!dev->tx_pkt_prep) { > > > + rte_errno = -ENOTSUP; > > > > rte_errno update may not be necessary here. see below > > > > > + return 0; > > IMO, We should return "nb_pkts" here instead of 0(i.e, all the packets are > > valid in-case PMD does not have tx_prep function) and in-case of "0" > > the following check in the application also will fail for no reason if > > (nb_prep < nb_pkts) { > > printf("tx_prep failed\n"); > > } > > > > Yes, it seems to be reasonable. > > > > > > + } > > > + > > > +#ifdef RTE_LIBRTE_ETHDEV_DEBUG > > > + if (queue_id >= dev->data->nb_tx_queues) { > > > + RTE_PMD_DEBUG_TRACE("Invalid TX queue_id=%d\n", queue_id); > > > + rte_errno = -EINVAL; > > > + return 0; > > > + } > > > +#endif > > > + > > > + return (*dev->tx_pkt_prep)(dev->data->tx_queues[queue_id], > > > + tx_pkts, nb_pkts); > > > +} > > > + > > > > IMO, We need to provide a compile time option for rte_eth_tx_prep as NOOP. > > Default option should be non NOOP but incase a _target_ want to override > > to NOOP it should be possible, the reasons is: > > > > - Low-end ARMv7,ARMv8 targets may not have PCIE-RC support and it may have > > only integrated NIC controller. On those targets, where integrated NIC > > controller does not use tx_prep service it can made it as NOOP to save > > cycles on following "rte_eth_tx_prep" and associated "if (unlikely(nb_prep > > < nb_rx))" checks in the application. > > > > /* Prepare burst of TX packets */ > > nb_prep = rte_eth_tx_prep(fs->rx_port, 0, pkts_burst, nb_rx); > > > > if (unlikely(nb_prep < nb_rx)) { > > int i; > > for (i = nb_prep; i < nb_rx; i++) > > rte_pktmbuf_free(pkts_burst[i]); } > > > > You mean to have a code for NOOP like: > > > /* Prepare burst of TX packets */ > nb_prep = nb_rx; /* rte_eth_tx_prep(fs->rx_port, 0, pkts_burst, nb_rx); */ > > if (unlikely(nb_prep < nb_rx)) { > int i; > for (i = nb_prep; i < nb_rx; i++) > rte_pktmbuf_free(pkts_burst[i]); } > > > and let optimizer to remove unused parts? I thought of creating compile time NOOP like this, CONFIG_RTE_LIBRTE_ETHDEV_TXPREP_SUPPORT=y in config/common_base and and have two flavors of definitions for rte_eth_tx_prep #ifdef RTE_LIBRTE_ETHDEV_TXPREP_SUPPORT static inline uint16_t rte_eth_tx_prep(uint8_t port_id, uint16_t queue_id, struct rte_mbuf **tx_pkts, uint16_t nb_pkts) { Proposed implementation } #else static inline uint16_t rte_eth_tx_prep(uint8_t port_id, uint16_t queue_id, struct rte_mbuf **tx_pkts, uint16_t nb_pkts) { (void)port_id; (void)queue_id; .. } #endif > > > IMHO it should be an application issue to use tx_prep or not. Some cases even _target_(example: config/defconfig_arm64-*) can also decides that. An example of such target is: Low-end ARMv7,ARMv8 targets may not have PCIE-RC support and it may have only integrated NIC controller. On those targets/configs, where integrated NIC controller does not use tx_prep service it can made it as NOOP to save cycles on following "rte_eth_tx_prep" and associated "if (unlikely(nb_prep < nb_rx))" checks in the application. > > While part of the job is done by the driver (verification and preparation), and part by application (error handling), such a global compile time option can introduce inconsistency, if application will not handle both cases. Each DPDK application build/compile against the target/config so I think it is OK. > > If someone wants to turn off this functionality, it should be done on application level, e.g. with compilation option. >