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

[Thomas Monjalon]

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.

>  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?

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?

> +/**
> + * 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?

> + *
> + * @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.