[dpdk-dev] net/virtio: fix wrong TX pkt length stats
Checks
Commit Message
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
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);
> }
>
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);
> > }
> >
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);
>>> }
>>>
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
@@ -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);
}