Re: [dpdk-dev] [PATCH v2 1/2] ethdev: add buffered tx api

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

Hi,

It is an overlay on the tx burst API.
Probably it doesn't hurt to add it but we have to be really cautious
with the API definition to try keeping it stable in the future.

2016-02-24 18:08, Tomasz Kulasek:
> +/**
> + * Structure used to buffer packets for future TX
> + * Used by APIs rte_eth_tx_buffer and rte_eth_tx_buffer_flush
> + */
> +struct rte_eth_dev_tx_buffer {
> +	unsigned nb_pkts;

What about "length"?
Why is it unsigned and the size is uint16_t?

> +	uint64_t errors;
> +	/**< Total number of queue packets to sent that are dropped. */

The errors are passed as userdata to the default callback.
If we really want to have this kind of counter, we can define our
own callback. So why defining this field as standard?
I would like to keep it as simple as possible.

> +	buffer_tx_error_fn cbfn;

Why not simply "callback" as name?

> +	void *userdata;
> +	uint16_t size;           /**< Size of buffer for buffered tx */
> +	struct rte_mbuf *pkts[];
> +};

What is the benefit of exposing this structure in the API,
except that it is used in some inline functions?

> +static inline uint16_t
> +rte_eth_tx_buffer_flush(uint8_t port_id, uint16_t queue_id,
> +		struct rte_eth_dev_tx_buffer *buffer)
> +{
> +	uint16_t sent;
> +
> +	uint16_t to_send = buffer->nb_pkts;
> +
> +	if (to_send == 0)
> +		return 0;

Why this check is done in the lib?
What is the performance gain if we are idle?
It can be done outside if needed.

> +	sent = rte_eth_tx_burst(port_id, queue_id, buffer->pkts, to_send);
> +
> +	buffer->nb_pkts = 0;
> +
> +	/* All packets sent, or to be dealt with by callback below */
> +	if (unlikely(sent != to_send))
> +		buffer->cbfn(&buffer->pkts[sent], to_send - sent,
> +				buffer->userdata);
> +
> +	return sent;
> +}
[...]
> +/**
> + * Callback function for tracking unsent buffered packets.
> + *
> + * This function can be passed to rte_eth_tx_buffer_set_err_callback() to
> + * adjust the default behaviour when buffered packets cannot be sent. This
> + * function drops any unsent packets, but also updates a user-supplied counter
> + * to track the overall number of packets dropped. The counter should be an
> + * uint64_t variable.
> + *
> + * NOTE: this function should not be called directly, instead it should be used
> + *       as a callback for packet buffering.
> + *
> + * NOTE: when configuring this function as a callback with
> + *       rte_eth_tx_buffer_set_err_callback(), the final, userdata parameter
> + *       should point to an uint64_t value.

Please forget this idea of counter in the default callback.

[...]
> +void
> +rte_eth_count_unsent_packet_callback(struct rte_mbuf **pkts, uint16_t unsent,
> +		void *userdata);

What about rte_eth_tx_buffer_default_callback as name?