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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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 <zhiyong.yang@intel.com>; dev@dpdk.org
>> Cc: yliu@fridaylinux.org; Tan, Jianfeng <jianfeng.tan@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.yang@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.coquelin@redhat.com>

Thanks,
Maxime

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