[PATCH 0/2] virtio: bugfix and cleanup

[PATCH 0/2] virtio: bugfix and cleanup

[Stephen Hemminger]

Fix for stale offload flags, and simplify transmit checks.

Stephen Hemminger (2):
  virtio: make sure rcv mbuf initialized correctly
  virtio: clean up space checks on xmit

 drivers/net/virtio/virtio_rxtx.c | 72 ++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 39 deletions(-)

-- 
2.1.4

[PATCH 1/2] virtio: make sure rcv mbuf initialized correctly

[Stephen Hemminger]

The virtio driver was not initializing all the fields in
the receive mbuf. This would cause bugs where previous usage
of mbuf would leave stale TCI and offload flags.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/virtio/virtio_rxtx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 5770fa2..466fee6 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -611,6 +611,8 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
 
 		rxm->port = rxvq->port_id;
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
+		rxm->ol_flags = 0;
+		rxm->vlan_tci = 0;
 
 		rxm->nb_segs = 1;
 		rxm->next = NULL;
@@ -731,6 +733,8 @@ virtio_recv_mergeable_pkts(void *rx_queue,
 		rxm->data_off = RTE_PKTMBUF_HEADROOM;
 		rxm->nb_segs = seg_num;
 		rxm->next = NULL;
+		rxm->ol_flags = 0;
+		rxm->vlan_tci = 0;
 		rxm->pkt_len = (uint32_t)(len[0] - hdr_size);
 		rxm->data_len = (uint16_t)(len[0] - hdr_size);
 
-- 
2.1.4

Re: [PATCH 1/2] virtio: make sure rcv mbuf initialized correctly

[Yuanhan Liu]

On Thu, Dec 03, 2015 at 05:12:53PM -0800, Stephen Hemminger wrote:
> The virtio driver was not initializing all the fields in
> the receive mbuf. This would cause bugs where previous usage
> of mbuf would leave stale TCI and offload flags.

Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Thanks.

	--yliu

[PATCH 2/2] virtio: clean up space checks on xmit

[Stephen Hemminger]

The space check for transmit ring only needs a single conditional.
I.e only need to recheck for space if there was no space in first check.

This can help performance and simplifies loop.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/virtio/virtio_rxtx.c | 68 +++++++++++++++++-----------------------
 1 file changed, 29 insertions(+), 39 deletions(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 466fee6..7cb932a 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -832,7 +832,6 @@ uint16_t
 virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
 {
 	struct virtqueue *txvq = tx_queue;
-	struct rte_mbuf *txm;
 	uint16_t nb_used, nb_tx;
 	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;
 
-		/*Positive value indicates it need free vring descriptors */
+		/* Positive value indicates it need free vring descriptors */
 		if (unlikely(need > 0)) {
 			nb_used = VIRTQUEUE_NUSED(txvq);
 			virtio_rmb();
 			need = RTE_MIN(need, (int)nb_used);
 
 			virtio_xmit_cleanup(txvq, need);
-			need = (int)tx_pkts[nb_tx]->nb_segs -
-				txvq->vq_free_cnt + 1;
+			need = txm->nb_segs - txvq->vq_free_cnt + 1;
+			if (unlikely(need > 0)) {
+				PMD_TX_LOG(ERR,
+					   "No free tx descriptors to transmit");
+				break;
+ 			}
 		}
 
-		/*
-		 * Zero or negative value indicates it has enough free
-		 * descriptors to use for transmitting.
-		 */
-		if (likely(need <= 0)) {
-			txm = tx_pkts[nb_tx];
-
-			/* Do VLAN tag insertion */
-			if (unlikely(txm->ol_flags & PKT_TX_VLAN_PKT)) {
-				error = rte_vlan_insert(&txm);
-				if (unlikely(error)) {
-					rte_pktmbuf_free(txm);
-					++nb_tx;
-					continue;
-				}
-			}
-
-			/* Enqueue Packet buffers */
-			error = virtqueue_enqueue_xmit(txvq, txm);
+		/* Do VLAN tag insertion */
+		if (unlikely(txm->ol_flags & PKT_TX_VLAN_PKT)) {
+			error = rte_vlan_insert(&txm);
 			if (unlikely(error)) {
-				if (error == ENOSPC)
-					PMD_TX_LOG(ERR, "virtqueue_enqueue Free count = 0");
-				else if (error == EMSGSIZE)
-					PMD_TX_LOG(ERR, "virtqueue_enqueue Free count < 1");
-				else
-					PMD_TX_LOG(ERR, "virtqueue_enqueue error: %d", error);
-				break;
+				rte_pktmbuf_free(txm);
+				continue;
 			}
-			nb_tx++;
-			txvq->bytes += txm->pkt_len;
-			virtio_update_packet_stats(txvq, txm);
-		} else {
-			PMD_TX_LOG(ERR, "No free tx descriptors to transmit");
+		}
+
+		/* Enqueue Packet buffers */
+		error = virtqueue_enqueue_xmit(txvq, txm);
+		if (unlikely(error)) {
+			if (error == ENOSPC)
+				PMD_TX_LOG(ERR, "virtqueue_enqueue Free count = 0");
+			else if (error == EMSGSIZE)
+				PMD_TX_LOG(ERR, "virtqueue_enqueue Free count < 1");
+			else
+				PMD_TX_LOG(ERR, "virtqueue_enqueue error: %d", error);
 			break;
 		}
+
+		txvq->bytes += txm->pkt_len;
+		virtio_update_packet_stats(txvq, txm);
 	}
 
 	txvq->packets += nb_tx;
-- 
2.1.4

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

[Yuanhan Liu]

On Thu, Dec 03, 2015 at 05:12:54PM -0800, Stephen Hemminger wrote:
> The space check for transmit ring only needs a single conditional.
> I.e only need to recheck for space if there was no space in first check.
> 
> This can help performance and simplifies loop.

This is a good cleanup. Besides that, I have few minor comments below.

> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  drivers/net/virtio/virtio_rxtx.c | 68 +++++++++++++++++-----------------------
>  1 file changed, 29 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 466fee6..7cb932a 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -832,7 +832,6 @@ uint16_t
>  virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts)
>  {
>  	struct virtqueue *txvq = tx_queue;
> -	struct rte_mbuf *txm;
>  	uint16_t nb_used, nb_tx;
>  	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?

(And yes, it has nothing to do with your patch, I just found it we
can rename it to a better name to improve the code readability a bit.
If you agree, would you submit a patch, or should I do it?)

>  
> -		/*Positive value indicates it need free vring descriptors */
> +		/* Positive value indicates it need free vring descriptors */
>  		if (unlikely(need > 0)) {
>  			nb_used = VIRTQUEUE_NUSED(txvq);
>  			virtio_rmb();
>  			need = RTE_MIN(need, (int)nb_used);
>  
>  			virtio_xmit_cleanup(txvq, need);
> -			need = (int)tx_pkts[nb_tx]->nb_segs -
> -				txvq->vq_free_cnt + 1;
> +			need = txm->nb_segs - txvq->vq_free_cnt + 1;
> +			if (unlikely(need > 0)) {
> +				PMD_TX_LOG(ERR,
> +					   "No free tx descriptors to transmit");
> +				break;
> + 			}
   ^

Minor nit: I found a leading white space there.

Otherwise, it looks good:

Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

	--yliu

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

[Stephen Hemminger]

	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.

> 
> (And yes, it has nothing to do with your patch, I just found it we
> can rename it to a better name to improve the code readability a bit.
> If you agree, would you submit a patch, or should I do it?)
> 
> >  
> > -		/*Positive value indicates it need free vring descriptors */
> > +		/* Positive value indicates it need free vring descriptors */
> >  		if (unlikely(need > 0)) {
> >  			nb_used = VIRTQUEUE_NUSED(txvq);
> >  			virtio_rmb();
> >  			need = RTE_MIN(need, (int)nb_used);
> >  
> >  			virtio_xmit_cleanup(txvq, need);
> > -			need = (int)tx_pkts[nb_tx]->nb_segs -
> > -				txvq->vq_free_cnt + 1;
> > +			need = txm->nb_segs - txvq->vq_free_cnt + 1;
> > +			if (unlikely(need > 0)) {
> > +				PMD_TX_LOG(ERR,
> > +					   "No free tx descriptors to transmit");
> > +				break;
> > + 			}
>    ^
> 
> Minor nit: I found a leading white space there.

Hmm. I didn't see it in checkpatch.

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

[Yuanhan Liu]

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

Re: [PATCH 0/2] virtio: bugfix and cleanup

[Thomas Monjalon]

> Fix for stale offload flags, and simplify transmit checks.
> 
> Stephen Hemminger (2):
>   virtio: make sure rcv mbuf initialized correctly
>   virtio: clean up space checks on xmit

Applied, thanks