[dpdk-dev] [PATCH 1/6] ethdev: add Tx preparation
Jerin Jacob
jerin.jacob at caviumnetworks.com
Fri Sep 9 07:58:17 CEST 2016
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.
>
More information about the dev
mailing list