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

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

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 <tomaszx.kulasek@intel.com>
> > 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.
>