diff mbox

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

Message ID 20171011043935.16813-1-zhiyong.yang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Yuanhan Liu
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Yang, Zhiyong Oct. 11, 2017, 4:39 a.m. UTC 
  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(+)

Comments

Maxime Coquelin Oct. 13, 2017, 10:06 a.m. UTC | #1 
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;

Thanks,
Maxime

>   		virtio_update_packet_stats(&txvq->stats, txm);
>   	}
>
Yang, Zhiyong Oct. 13, 2017, 12:44 p.m. UTC | #2 
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.  

Thanks
Zhiyong 

> 

> Thanks,

> Maxime

> 

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

> >   	}

> >
Maxime Coquelin Oct. 13, 2017, 12:47 p.m. UTC | #3 
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);
>>>    	}
>>>
Yuanhan Liu Oct. 17, 2017, 1:21 p.m. UTC | #4 
On Wed, Oct 11, 2017 at 12:39:35PM +0800, 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.

Nice catch!

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

It should also Cc stable.

> 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

Hmm..., why not modifying it directly in virtqueue_enqueue_xmit()?

	--yliu

> +		 * 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;
>  		virtio_update_packet_stats(&txvq->stats, txm);
>  	}
> -- 
> 2.13.3
diff mbox

Patch

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;
 		virtio_update_packet_stats(&txvq->stats, txm);
 	}