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

[Kulasek, TomaszX]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Thursday, December 22, 2016 15:25
> To: Kulasek, TomaszX <tomaszx.kulasek at intel.com>
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v14 1/8] ethdev: add Tx preparation
> 
> Hi Tomasz,
> 
> 2016-12-22 14:05, Tomasz Kulasek:
> > Added API for `rte_eth_tx_prepare`
> >
> > uint16_t rte_eth_tx_prepare(uint8_t port_id, uint16_t queue_id,
> > 	struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
> 
> As discussed earlier and agreed by Konstantin, please mark this API
> as experimental.
> We could make some changes in 17.05 to improve error description
> or add some flags to modify the behaviour.
> 

Is it enough to add

/**
 * @warning
 * @b EXPERIMENTAL: this API may change without prior notice

?

> 
> > int rte_net_intel_cksum_prepare(struct rte_mbuf *m)
> >
> >   to prepare pseudo header checksum for TSO and non-TSO tcp/udp packets
> >   before hardware tx checksum offload.
> >    - for non-TSO tcp/udp packets full pseudo-header checksum is
> >      counted and set.
> >    - for TSO the IP payload length is not included.
> >
> >
> > int
> > rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
> >
> >   this function uses same logic as rte_net_intel_cksum_prepare, but
> >   allows application to choose which offloads should be taken into
> >   account, if full preparation is not required.
> 
> How the application knows which offload flag should be taken into account?
>

This new API is used in ena driver:

+		/* ENA doesn't need different phdr cskum for TSO */
+		ret = rte_net_intel_cksum_flags_prepare(m,
+			ol_flags & ~PKT_TX_TCP_SEG);
+		if (ret != 0) {
+			rte_errno = ret;
+			return i;
+		}

It's more useful to mask offloads which didn't should be used. 

> 
> >  #
> > +# Use real NOOP to turn off TX preparation stage
> > +#
> > +# While the behaviour of ``rte_ethdev_tx_prepare`` may change after
> turning on
> > +# real NOOP, this configuration shouldn't be never enabled globaly, and
> can be
> > +# used in appropriate target configuration file with a following
> restrictions
> > +#
> > +CONFIG_RTE_ETHDEV_TX_PREPARE_NOOP=n
> 
> As discussed earlier, it would be easier to not call tx_prepare at all.
> However, this option allows an optimization when compiling DPDK for a
> known environment without modifying the application.
> So it is worth to keep it.
> 
> The text explaining the option should be improved.
> I suggest this text:
> 
> # Turn off Tx preparation stage
> #
> # Warning: rte_ethdev_tx_prepare() can be safely disabled only if using a
> # driver which do not implement any Tx preparation.
> 
> 
> > +	uint16_t nb_seg_max;  /**< Max number of segments per whole packet.
> */
> > +	uint16_t nb_mtu_seg_max; /**< Max number of segments per one MTU */
> 
> In another mail, you've added this explanation:
> * For non-TSO packet, a single transmit packet may span up to
> "nb_mtu_seg_max" buffers.
> * For TSO packet the total number of data descriptors is "nb_seg_max", and
> each segment within the TSO may span up to "nb_mtu_seg_max".
> 
> Maybe you can try to mix these comments to improve the doxygen.

Ok, I will.

Tomasz