From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yuanhan Liu Subject: Re: [PATCH v2] net/virtio: fix wrong TX pkt length stats Date: Mon, 23 Oct 2017 21:21:44 +0800 Message-ID: <20171023132143.GI1545@yliu-home> References: <20171011043935.16813-1-zhiyong.yang@intel.com> <20171023064036.56821-1-zhiyong.yang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org, stable@dpdk.org, maxime.coquelin@redhat.com To: Zhiyong Yang Return-path: Content-Disposition: inline In-Reply-To: <20171023064036.56821-1-zhiyong.yang@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, Oct 23, 2017 at 02:40:36PM +0800, Zhiyong Yang wrote: > In the function virtqueue_enqueue_xmit(), when can_push is true, > vtnet_hdr_size is added to pkt_len by calling rte_pktmbuf_prepend. > So, virtio header length should be subtracted before calling stats > function. > > Fixes: 58169a9c8153 ("net/virtio: support Tx checksum offload") > > Cc: stable@dpdk.org > Cc: yliu@fridaylinux.org > Cc: maxime.coquelin@redhat.com > Signed-off-by: Zhiyong Yang > Reviewed-by: Maxime Coquelin > --- > > Changes in V2: > Put code in the function virtqueue_enqueue_xmit() according to > yuanhan's comments. > > drivers/net/virtio/virtio_rxtx.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c > index 2cf82fef4..f28751e07 100644 > --- a/drivers/net/virtio/virtio_rxtx.c > +++ b/drivers/net/virtio/virtio_rxtx.c > @@ -396,6 +396,13 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie, > vq->vq_desc_tail_idx = idx; > vq->vq_free_cnt = (uint16_t)(vq->vq_free_cnt - needed); > vq_update_avail_ring(vq, head_idx); > + > + /* when can_push is true, vtnet_hdr_size is added to pkt_len > + * of mbuf. It should be subtracted in order to make stats function > + * work in the right way. > + */ > + if (can_push) > + cookie->pkt_len -= head_size; I actually meant to put it inside the "if (can_push)" clause, so that you don't have to add yet another if. The another reason I wanted you to do the move is, rte_pktmbuf_prepend (which did the magic) is exactly invoked inside this if. Such move puts logical related code tight together. At least, you could make the comment simpler: you don't have to say "when can_push is true ...". Instead, it could be: /* * rte_pktmbuf_prepend() counts the hdr size to the pkt length, * which is wrong. Below subtract restores the correct pkt size. */ --yliu