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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi Thomas,

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

Good point,  yes need to make it consistent.

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

I described the benefits, I think it provides here:
http://dpdk.org/ml/archives/dev/2016-February/033058.html

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

Yes, that could be done outside, but if user has to do it anyway,
why not to put it inside?
I don't expect any performance gain/loss because of that -
just seems a bit more convenient to the user.

Konstantin

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