On Sat, Dec 05, 2015 at 11:50:07AM -0800, Stephen Hemminger wrote:
> int error;
> > >
> > > @@ -846,58 +845,49 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf
> > > **tx_pkts, uint16_t nb_pkts)
> > > if (likely(nb_used > txvq->vq_nentries - txvq->vq_free_thresh))
> > > virtio_xmit_cleanup(txvq, nb_used);
> > >
> > > - nb_tx = 0;
> > > -
> > > - while (nb_tx < nb_pkts) {
> > > + for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++) {
> > > + struct rte_mbuf *txm = tx_pkts[nb_tx];
> > > /* Need one more descriptor for virtio header. */
> > > - int need = tx_pkts[nb_tx]->nb_segs - txvq->vq_free_cnt + 1;
> > > + int need = txm->nb_segs - txvq->vq_free_cnt + 1;
> >
> > While reviewing the code, I found the var name `need' is not properly
> > taken. Maybe `need_cleanup' is better, and it's better to be defined
> > as bool type. What do you think of that?
>
> The variable need indicates how many more buffers are needed to
> complete the transmit. In later patches, there is a variable slots
> so:
> needed = slots - free
>
> So if needed is positive, then more buffers are needed than available
> and transmit is blocked. If needed is negative then there is free
> space available.
Yeah, I knew that. And there is a comment for that (thanks for the
explanation anyway!):
/* Positive value indicates it need free vring descriptors */
I mean, if a var name well taken, we could avoid comments like above.
However, for this case, I simply overlooked that virtio_xmit_cleanup()
takes `need' as the input, so that I thought `need' is just a boolean
var to check if we need to get few more free spaces. And that's how
my above suggestion comes. So, sorry for the noisy.
--yliu
