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?
