[dpdk-dev] [PATCH 1/6] ethdev: add Tx preparation

Kulasek, TomaszX tomaszx.kulasek at intel.com
Thu Sep 8 18:09:05 CEST 2016


Hi Jerin,

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


IMHO it should be an application issue to use tx_prep or not.

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.

If someone wants to turn off this functionality, it should be done on application level, e.g. with compilation option.
 
> 
> Jerin
> 



More information about the dev mailing list