From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Coquelin Subject: Re: [PATCH] net/virtio: fix wrong TX pkt length stats Date: Fri, 13 Oct 2017 14:47:36 +0200 Message-ID: <34d70b50-c35f-5eb2-7d30-19d35a1a43cb@redhat.com> References: <20171011043935.16813-1-zhiyong.yang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: "yliu@fridaylinux.org" , "Tan, Jianfeng" To: "Yang, Zhiyong" , "dev@dpdk.org" Return-path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id F326E1B626 for ; Fri, 13 Oct 2017 14:47:40 +0200 (CEST) In-Reply-To: Content-Language: en-US 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 10/13/2017 02:44 PM, Yang, Zhiyong wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com] >> Sent: Friday, October 13, 2017 6:06 PM >> To: Yang, Zhiyong ; dev@dpdk.org >> Cc: yliu@fridaylinux.org; Tan, Jianfeng >> 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 >>> --- >>> 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 Thanks, Maxime > Thanks > Zhiyong > >> >> Thanks, >> Maxime >> >>> virtio_update_packet_stats(&txvq->stats, txm); >>> } >>>