From: Yuanhan Liu <yuanhan.liu@linux.intel.com>
To: Stephen Hemminger <stephen@networkplumber.org>
Cc: dev@dpdk.org
Subject: Re: [PATCH 2/2] virtio: clean up space checks on xmit
Date: Tue, 8 Dec 2015 09:54:10 +0800 [thread overview]
Message-ID: <20151208015410.GG29571@yliu-dev.sh.intel.com> (raw)
In-Reply-To: <20151205115007.2230a16a@xeon-e3>
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
next prev parent reply other threads:[~2015-12-08 1:53 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-04 1:12 [PATCH 0/2] virtio: bugfix and cleanup Stephen Hemminger
2015-12-04 1:12 ` [PATCH 1/2] virtio: make sure rcv mbuf initialized correctly Stephen Hemminger
2015-12-04 3:18 ` Yuanhan Liu
2015-12-04 1:12 ` [PATCH 2/2] virtio: clean up space checks on xmit Stephen Hemminger
2015-12-04 3:28 ` Yuanhan Liu
2015-12-05 19:50 ` Stephen Hemminger
2015-12-08 1:54 ` Yuanhan Liu [this message]
2015-12-06 23:02 ` [PATCH 0/2] virtio: bugfix and cleanup Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20151208015410.GG29571@yliu-dev.sh.intel.com \
--to=yuanhan.liu@linux.intel.com \
--cc=dev@dpdk.org \
--cc=stephen@networkplumber.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.