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 at caviumnetworks.com] > > Sent: Thursday, September 8, 2016 09:29 > > To: Kulasek, TomaszX <tomaszx.kulasek at intel.com> > > Cc: dev at 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. >
