Re: [dpdk-dev] [PATCH 2/2] virtio: clean up space checks on xmit

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

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