Re: [dpdk-dev] [PATCH] net/virtio: fix wrong TX pkt length stats

[Maxime Coquelin]

On 10/13/2017 02:44 PM, Yang, Zhiyong wrote:

Hi Maxime,

-----Original Message-----
From: Maxime Coquelin [mailto:maxime.coque...@redhat.com]
Sent: Friday, October 13, 2017 6:06 PM
To: Yang, Zhiyong <zhiyong.y...@intel.com>; dev@dpdk.org
Cc: y...@fridaylinux.org; Tan, Jianfeng <jianfeng....@intel.com>
Subject: Re: [PATCH] net/virtio: fix wrong TX pkt length stats

Hi Zhiyong,

On 10/11/2017 06:39 AM, Zhiyong Yang wrote:

In the function virtqueue_enqueue_xmit(), when can_push == 1 is true,
vtnet_hdr_size is added to pkt_len by calling rte_pktmbuf_prepend.
So, virtio header len should be subtracted before calling stats
function.

Fixes: 58169a9c8153 ("net/virtio: support Tx checksum offload")

Signed-off-by: Zhiyong Yang <zhiyong.y...@intel.com>
---
   drivers/net/virtio/virtio_rxtx.c | 6 ++++++
   1 file changed, 6 insertions(+)

diff --git a/drivers/net/virtio/virtio_rxtx.c
b/drivers/net/virtio/virtio_rxtx.c
index 609b4138a..bf14f9a99 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -1079,6 +1079,12 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf

**tx_pkts, uint16_t nb_pkts)

                /* Enqueue Packet buffers */
                virtqueue_enqueue_xmit(txvq, txm, slots, use_indirect,

can_push);

+               /* In function virtqueue_enqueue_xmit(), when can_push == 1
+                * is true, vtnet_hdr_size is added to pkt_len of mbuf. So, it
+                * should be subtracted before calling stats function.
+                */
+               if (can_push == 1)
+                       txm->pkt_len -= txvq->vq->hw->vtnet_hdr_size;
                txvq->stats.bytes += txm->pkt_len;

I acknowledge the problem, but I don't like modifying pkt_len.
This is not the case currently, but in case we want to do something with the
mbuf later in virtio_xmit_cleanup(), it could be good to have the real length
there.

What about:

if (can_push == 1)
      txvq->stats.bytes += txm->pkt_len - txvq->vq->hw->vtnet_hdr_size; else
      txvq->stats.bytes += txm->pkt_len;

I don't like modifying pkt_len, too.
But We need to consider that some stats are updated in 
virtio_update_packet_stats()
In this function,  pkt_len will be used further.

Ha, good point!
I think we have no better way then:
Reviewed-by: Maxime Coquelin <maxime.coque...@redhat.com>

Thanks,
Maxime

Thanks
Zhiyong

Thanks,
Maxime

                virtio_update_packet_stats(&txvq->stats, txm);
        }