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

[Thomas Monjalon <thomas.monjalon@6wind.com>]

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?

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