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

Kulasek, TomaszX tomaszx.kulasek at intel.com
Thu Oct 27 18:29:59 CEST 2016


Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Thursday, October 27, 2016 17:01
> To: Kulasek, TomaszX <tomaszx.kulasek at intel.com>
> Cc: dev at dpdk.org; olivier.matz at 6wind.com
> Subject: Re: [dpdk-dev] [PATCH v11 1/6] ethdev: add Tx preparation
> 
> Hi Tomasz,
> 
> This is a major new function in the API and I still have some comments.
> 
> 2016-10-26 14:56, Tomasz Kulasek:
> > --- a/config/common_base
> > +++ b/config/common_base
> > +CONFIG_RTE_ETHDEV_TX_PREP=y
> 
> We cannot enable it until it is implemented in every drivers.
> 

For most of drivers it's safe to enable it by default and if this feature is not supported, no checks/modifications are done. In that meaning the processing path is the same as without using Tx preparation. 

Introducing this macro was discussed in the threads:

http://dpdk.org/ml/archives/dev/2016-September/046437.html
http://dpdk.org/dev/patchwork/patch/15770/

Short conclusion:

Jerin Jacob pointed, that it can have significant impact on some architectures (such a low-end ARMv7, ARMv8 targets which may not have PCIE-RC support and have only integrated NIC controller), even if this feature is not implemented.

We've added this macro to provide an ability to use NOOP operation and allow turn off this feature if will have adverse effect on specific configuration/hardware.

> >  struct rte_eth_dev {
> >  	eth_rx_burst_t rx_pkt_burst; /**< Pointer to PMD receive function.
> */
> >  	eth_tx_burst_t tx_pkt_burst; /**< Pointer to PMD transmit function.
> */
> > +	eth_tx_prep_t tx_pkt_prep; /**< Pointer to PMD transmit prepare
> function. */
> >  	struct rte_eth_dev_data *data;  /**< Pointer to device data */
> >  	const struct eth_driver *driver;/**< Driver for this device */
> >  	const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */
> 
> Could you confirm why tx_pkt_prep is not in dev_ops?
> I guess we want to have several implementations?
> 

Yes, the implementation may vary on selected tx_burst path (e.g. vector implementation, simple implementation, full featured, and so on, and can have another requirements, such a implemented features, performance requirements for each implementation).
The path is chosen based on the application requirements transparently and we have a pair of callbacks -- tx_burst and corresponding callback (which depends directly on tx_burst path).

> Shouldn't we have a const struct control_dev_ops and a struct
> datapath_dev_ops?
> 
> > +rte_eth_tx_prep(uint8_t port_id, uint16_t queue_id, struct rte_mbuf
> **tx_pkts,
> > +               uint16_t nb_pkts)
> 
> The word "prep" can be understood as "prepend".
> Why not rte_eth_tx_prepare?
> 

I do not mind.

> > +/**
> > + * Fix pseudo header checksum
> > + *
> > + * This function fixes pseudo header checksum for TSO and non-TSO
> tcp/udp in
> > + * provided mbufs packet data.
> > + *
> > + * - for non-TSO tcp/udp packets full pseudo-header checksum is counted
> and set
> > + *   in packet data,
> > + * - for TSO the IP payload length is not included in pseudo header.
> > + *
> > + * This function expects that used headers are in the first data
> segment of
> > + * mbuf, are not fragmented and can be safely modified.
> 
> What happens otherwise?
> 

There are requirements for this helper function. For Tx preparation callback we check this requirement and if it fails, -NOTSUP errno is returned.

> > + *
> > + * @param m
> > + *   The packet mbuf to be fixed.
> > + * @return
> > + *   0 if checksum is initialized properly
> > + */
> > +static inline int
> > +rte_phdr_cksum_fix(struct rte_mbuf *m)
> 
> Could we find a better name for this function?
> - About the prefix, rte_ip_ ?
> - About the scope, where this phdr_cksum is specified?
> Isn't it an intel_phdr_cksum to match what hardware expects?
> - About the verb, is it really fixing something broken?
> Or just writing into a mbuf?
> I would suggest rte_ip_intel_cksum_prepare.

Fixes in the meaning of requirements for offloads, which states e.g. that to use specific Tx offload we should to fill checksums in a proper way, if not, thee settings are not valid and should be fixed.

But you're right, prepare is better word.

About the function name, maybe rte_net_intel_chksum_prepare will be better while it prepares also tcp/udp headers and is placed in rte_net.h?

Tomasz


More information about the dev mailing list