From mboxrd@z Thu Jan  1 00:00:00 1970
From: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Subject: Re: [PATCH 2/2] virtio: clean up space checks on xmit
Date: Tue, 8 Dec 2015 09:54:10 +0800
Message-ID: <20151208015410.GG29571@yliu-dev.sh.intel.com>
References: <1449191574-14629-1-git-send-email-stephen@networkplumber.org>
 <1449191574-14629-3-git-send-email-stephen@networkplumber.org>
 <20151204032858.GB29571@yliu-dev.sh.intel.com>
 <20151205115007.2230a16a@xeon-e3>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: dev@dpdk.org
To: Stephen Hemminger <stephen@networkplumber.org>
Return-path: <dev-bounces@dpdk.org>
Received: from mga03.intel.com (mga03.intel.com [134.134.136.65])
 by dpdk.org (Postfix) with ESMTP id 2540F11F5
 for <dev@dpdk.org>; Tue,  8 Dec 2015 02:53:14 +0100 (CET)
Content-Disposition: inline
In-Reply-To: <20151205115007.2230a16a@xeon-e3>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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