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

Kulasek, TomaszX tomaszx.kulasek at intel.com
Thu Oct 13 12:47:24 CEST 2016


Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Thursday, October 13, 2016 09:09
> To: Kulasek, TomaszX <tomaszx.kulasek at intel.com>
> Cc: dev at dpdk.org; Ananyev, Konstantin <konstantin.ananyev at intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/6] ethdev: add Tx preparation
> 
> Hi Tomasz,
> 
> Any news?
> Sorry to speed up, we are very very late for RC1.
> 
> 2016-10-10 16:08, Thomas Monjalon:
> > Hi,
> >
> > Now that the feature seems to meet a consensus, I've looked at it more
> > closely before integrating. Sorry if it appears like a late review.
> >
> > 2016-09-30 11:00, Tomasz Kulasek:
> > > Added API for `rte_eth_tx_prep`
> >
> > I would love to read the usability and performance considerations here.
> > No need for something as long as the cover letter. Just few lines
> > about why it is needed and why it is a good choice for performance.
> >
> > > uint16_t rte_eth_tx_prep(uint8_t port_id, uint16_t queue_id,
> > > 	struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
> > >
> > > Added fields to the `struct rte_eth_desc_lim`:
> > >
> > > 	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 */
> > [...]
> > > +#else
> > > +
> > > +static inline uint16_t
> > > +rte_eth_tx_prep(uint8_t port_id __rte_unused, uint16_t queue_id
> __rte_unused,
> > > +		struct rte_mbuf **tx_pkts __rte_unused, uint16_t nb_pkts)
> >
> > Doxygen is failing here.
> > Have you tried to move __rte_unused before the identifier?
> >
> > [...]
> > > +#define PKT_TX_OFFLOAD_MASK (    \
> > > +		PKT_TX_IP_CKSUM |        \
> > > +		PKT_TX_L4_MASK |         \
> > > +		PKT_TX_OUTER_IP_CKSUM |  \
> > > +		PKT_TX_TCP_SEG |         \
> > > +		PKT_TX_QINQ_PKT |        \
> > > +		PKT_TX_VLAN_PKT)
> >
> > We should really stop adding some public constants without the proper
> > RTE prefix.
> > And by the way, should not we move such flags into rte_net?
> 
> Do not worry with this comment. It was just a thought which could be
> addressed in a separate patch by someone else.

I've used this name convention to be consistent with other offload flag names, and this is the only reason. The place of these flags was chosen for easier management (flags and mask in one place).

I will leave it as is.

> 
> > [...]
> > > -SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include := rte_ip.h rte_tcp.h
> > > rte_udp.h rte_sctp.h rte_icmp.h rte_arp.h
> > > +SYMLINK-$(CONFIG_RTE_LIBRTE_NET)-include := rte_ip.h rte_tcp.h
> > > +rte_udp.h rte_sctp.h rte_icmp.h rte_arp.h rte_pkt.h
> >
> > You can use the += operator on a new line for free :)
> >
> > No more comments, the rest seems OK. Thanks
> 

Some additional work was needed due to the new tx tunnel type flags and changes in csum engine.

I will send v5 today.

Tomasz


More information about the dev mailing list