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

[Thomas Monjalon]

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.

> [...]
> > -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