First patch is fixing packed ring header len. Issue was reported by Marvin. Second patch is a (very) light optimization, to make used directly of the static hdr len value in packed-ring only functions. Maxime Coquelin (2): vhost: fix Virtio-net header len with packed ring vhost: use fixed Virtio-net header len packed ring lib/librte_vhost/vhost_user.c | 4 +++- lib/librte_vhost/virtio_net.c | 11 ++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) -- 2.26.2
In case packed ring layout has been negotiated, but neither Version 1 nor mergeable buffers, the Virtio-net header len is assigned to the legacy devices value, which is wrong. This patch fixes this with using the proper len as devices using packed ring are not legacy devices. Fixes: a922401f35cc ("vhost: add Rx support for packed ring") Fixes: ae999ce49dcb ("vhost: add Tx support for packed ring") Cc: stable@dpdk.org Reported-by: Marvin Liu <yong.liu@intel.com> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- lib/librte_vhost/vhost_user.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index 4deceb3e00..5d1fb9e863 100644 --- a/lib/librte_vhost/vhost_user.c +++ b/lib/librte_vhost/vhost_user.c @@ -341,7 +341,9 @@ vhost_user_set_features(struct virtio_net **pdev, struct VhostUserMsg *msg, dev->features = features; if (dev->features & - ((1 << VIRTIO_NET_F_MRG_RXBUF) | (1ULL << VIRTIO_F_VERSION_1))) { + ((1ULL << VIRTIO_NET_F_MRG_RXBUF) | + (1ULL << VIRTIO_F_VERSION_1) | + (1ULL << VIRTIO_F_RING_PACKED))) { dev->vhost_hlen = sizeof(struct virtio_net_hdr_mrg_rxbuf); } else { dev->vhost_hlen = sizeof(struct virtio_net_hdr); -- 2.26.2
This small optimization uses static the Virtio-net header len in packed datapath, since Virtio-net header cannot be the legacy one in case of packed ring. Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> --- lib/librte_vhost/virtio_net.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c index 0a0bea1a5a..5865fa5f65 100644 --- a/lib/librte_vhost/virtio_net.c +++ b/lib/librte_vhost/virtio_net.c @@ -1147,7 +1147,7 @@ vhost_enqueue_single_packed(struct virtio_net *dev, uint16_t buf_id = 0; uint32_t len = 0; uint16_t desc_count; - uint32_t size = pkt->pkt_len + dev->vhost_hlen; + uint32_t size = pkt->pkt_len + sizeof(struct virtio_net_hdr_mrg_rxbuf); uint16_t num_buffers = 0; uint32_t buffer_len[vq->size]; uint16_t buffer_buf_id[vq->size]; @@ -1262,7 +1262,7 @@ virtio_dev_rx_batch_packed(struct virtio_net *dev, uint16_t avail_idx = vq->last_avail_idx; uint64_t desc_addrs[PACKED_BATCH_SIZE]; struct virtio_net_hdr_mrg_rxbuf *hdrs[PACKED_BATCH_SIZE]; - uint32_t buf_offset = dev->vhost_hlen; + uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf); uint64_t lens[PACKED_BATCH_SIZE]; uint16_t ids[PACKED_BATCH_SIZE]; uint16_t i; @@ -1308,7 +1308,8 @@ virtio_dev_rx_batch_packed(struct virtio_net *dev, rte_prefetch0((void *)(uintptr_t)desc_addrs[i]); hdrs[i] = (struct virtio_net_hdr_mrg_rxbuf *) (uintptr_t)desc_addrs[i]; - lens[i] = pkts[i]->pkt_len + dev->vhost_hlen; + lens[i] = pkts[i]->pkt_len + + sizeof(struct virtio_net_hdr_mrg_rxbuf); } vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE) @@ -2277,7 +2278,7 @@ vhost_reserve_avail_batch_packed(struct virtio_net *dev, struct virtio_net_hdr *hdr; uint64_t lens[PACKED_BATCH_SIZE]; uint64_t buf_lens[PACKED_BATCH_SIZE]; - uint32_t buf_offset = dev->vhost_hlen; + uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf); uint16_t flags, i; if (unlikely(avail_idx & PACKED_BATCH_MASK)) @@ -2354,7 +2355,7 @@ virtio_dev_tx_batch_packed(struct virtio_net *dev, struct rte_mbuf **pkts) { uint16_t avail_idx = vq->last_avail_idx; - uint32_t buf_offset = dev->vhost_hlen; + uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf); uintptr_t desc_addrs[PACKED_BATCH_SIZE]; uint16_t ids[PACKED_BATCH_SIZE]; uint16_t i; -- 2.26.2
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, October 1, 2020 6:12 PM
> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Liu, Yong
> <yong.liu@intel.com>
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [PATCH 1/2] vhost: fix Virtio-net header len with packed ring
>
> In case packed ring layout has been negotiated, but neither
> Version 1 nor mergeable buffers, the Virtio-net header len
> is assigned to the legacy devices value, which is wrong.
>
> This patch fixes this with using the proper len as devices
> using packed ring are not legacy devices.
>
> Fixes: a922401f35cc ("vhost: add Rx support for packed ring")
> Fixes: ae999ce49dcb ("vhost: add Tx support for packed ring")
> Cc: stable@dpdk.org
>
> Reported-by: Marvin Liu <yong.liu@intel.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> lib/librte_vhost/vhost_user.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 4deceb3e00..5d1fb9e863 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -341,7 +341,9 @@ vhost_user_set_features(struct virtio_net **pdev,
> struct VhostUserMsg *msg,
>
> dev->features = features;
> if (dev->features &
> - ((1 << VIRTIO_NET_F_MRG_RXBUF) | (1ULL << VIRTIO_F_VERSION_1)))
> {
> + ((1ULL << VIRTIO_NET_F_MRG_RXBUF) |
> + (1ULL << VIRTIO_F_VERSION_1) |
> + (1ULL << VIRTIO_F_RING_PACKED))) {
> dev->vhost_hlen = sizeof(struct virtio_net_hdr_mrg_rxbuf);
> } else {
> dev->vhost_hlen = sizeof(struct virtio_net_hdr);
> --
> 2.26.2
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
Hi Maxime, > -----Original Message----- > From: Maxime Coquelin <maxime.coquelin@redhat.com> > Sent: Thursday, October 1, 2020 6:12 PM > To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Liu, Yong > <yong.liu@intel.com> > Cc: Maxime Coquelin <maxime.coquelin@redhat.com> > Subject: [PATCH 2/2] vhost: use fixed Virtio-net header len packed ring > > This small optimization uses static the Virtio-net header len Better use 'the static Virtio-net header length' here when you apply the patch 😊. With this fix: Reviewed-by: Chenbo Xia <chenbo.xia@intel.com> > in packed datapath, since Virtio-net header cannot be the > legacy one in case of packed ring. > > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> > --- > lib/librte_vhost/virtio_net.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c > index 0a0bea1a5a..5865fa5f65 100644 > --- a/lib/librte_vhost/virtio_net.c > +++ b/lib/librte_vhost/virtio_net.c > @@ -1147,7 +1147,7 @@ vhost_enqueue_single_packed(struct virtio_net *dev, > uint16_t buf_id = 0; > uint32_t len = 0; > uint16_t desc_count; > - uint32_t size = pkt->pkt_len + dev->vhost_hlen; > + uint32_t size = pkt->pkt_len + sizeof(struct > virtio_net_hdr_mrg_rxbuf); > uint16_t num_buffers = 0; > uint32_t buffer_len[vq->size]; > uint16_t buffer_buf_id[vq->size]; > @@ -1262,7 +1262,7 @@ virtio_dev_rx_batch_packed(struct virtio_net *dev, > uint16_t avail_idx = vq->last_avail_idx; > uint64_t desc_addrs[PACKED_BATCH_SIZE]; > struct virtio_net_hdr_mrg_rxbuf *hdrs[PACKED_BATCH_SIZE]; > - uint32_t buf_offset = dev->vhost_hlen; > + uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf); > uint64_t lens[PACKED_BATCH_SIZE]; > uint16_t ids[PACKED_BATCH_SIZE]; > uint16_t i; > @@ -1308,7 +1308,8 @@ virtio_dev_rx_batch_packed(struct virtio_net *dev, > rte_prefetch0((void *)(uintptr_t)desc_addrs[i]); > hdrs[i] = (struct virtio_net_hdr_mrg_rxbuf *) > (uintptr_t)desc_addrs[i]; > - lens[i] = pkts[i]->pkt_len + dev->vhost_hlen; > + lens[i] = pkts[i]->pkt_len + > + sizeof(struct virtio_net_hdr_mrg_rxbuf); > } > > vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE) > @@ -2277,7 +2278,7 @@ vhost_reserve_avail_batch_packed(struct virtio_net > *dev, > struct virtio_net_hdr *hdr; > uint64_t lens[PACKED_BATCH_SIZE]; > uint64_t buf_lens[PACKED_BATCH_SIZE]; > - uint32_t buf_offset = dev->vhost_hlen; > + uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf); > uint16_t flags, i; > > if (unlikely(avail_idx & PACKED_BATCH_MASK)) > @@ -2354,7 +2355,7 @@ virtio_dev_tx_batch_packed(struct virtio_net *dev, > struct rte_mbuf **pkts) > { > uint16_t avail_idx = vq->last_avail_idx; > - uint32_t buf_offset = dev->vhost_hlen; > + uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf); > uintptr_t desc_addrs[PACKED_BATCH_SIZE]; > uint16_t ids[PACKED_BATCH_SIZE]; > uint16_t i; > -- > 2.26.2
Welcome back Chenbo, On 10/9/20 8:36 AM, Xia, Chenbo wrote: > Hi Maxime, > >> -----Original Message----- >> From: Maxime Coquelin <maxime.coquelin@redhat.com> >> Sent: Thursday, October 1, 2020 6:12 PM >> To: dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Liu, Yong >> <yong.liu@intel.com> >> Cc: Maxime Coquelin <maxime.coquelin@redhat.com> >> Subject: [PATCH 2/2] vhost: use fixed Virtio-net header len packed ring >> >> This small optimization uses static the Virtio-net header len > > Better use 'the static Virtio-net header length' here when you apply the > patch 😊. With this fix: I did the change when applying, thanks! Maxime > Reviewed-by: Chenbo Xia <chenbo.xia@intel.com> > >> in packed datapath, since Virtio-net header cannot be the >> legacy one in case of packed ring. >> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> >> --- >> lib/librte_vhost/virtio_net.c | 11 ++++++----- >> 1 file changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c >> index 0a0bea1a5a..5865fa5f65 100644 >> --- a/lib/librte_vhost/virtio_net.c >> +++ b/lib/librte_vhost/virtio_net.c >> @@ -1147,7 +1147,7 @@ vhost_enqueue_single_packed(struct virtio_net *dev, >> uint16_t buf_id = 0; >> uint32_t len = 0; >> uint16_t desc_count; >> - uint32_t size = pkt->pkt_len + dev->vhost_hlen; >> + uint32_t size = pkt->pkt_len + sizeof(struct >> virtio_net_hdr_mrg_rxbuf); >> uint16_t num_buffers = 0; >> uint32_t buffer_len[vq->size]; >> uint16_t buffer_buf_id[vq->size]; >> @@ -1262,7 +1262,7 @@ virtio_dev_rx_batch_packed(struct virtio_net *dev, >> uint16_t avail_idx = vq->last_avail_idx; >> uint64_t desc_addrs[PACKED_BATCH_SIZE]; >> struct virtio_net_hdr_mrg_rxbuf *hdrs[PACKED_BATCH_SIZE]; >> - uint32_t buf_offset = dev->vhost_hlen; >> + uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf); >> uint64_t lens[PACKED_BATCH_SIZE]; >> uint16_t ids[PACKED_BATCH_SIZE]; >> uint16_t i; >> @@ -1308,7 +1308,8 @@ virtio_dev_rx_batch_packed(struct virtio_net *dev, >> rte_prefetch0((void *)(uintptr_t)desc_addrs[i]); >> hdrs[i] = (struct virtio_net_hdr_mrg_rxbuf *) >> (uintptr_t)desc_addrs[i]; >> - lens[i] = pkts[i]->pkt_len + dev->vhost_hlen; >> + lens[i] = pkts[i]->pkt_len + >> + sizeof(struct virtio_net_hdr_mrg_rxbuf); >> } >> >> vhost_for_each_try_unroll(i, 0, PACKED_BATCH_SIZE) >> @@ -2277,7 +2278,7 @@ vhost_reserve_avail_batch_packed(struct virtio_net >> *dev, >> struct virtio_net_hdr *hdr; >> uint64_t lens[PACKED_BATCH_SIZE]; >> uint64_t buf_lens[PACKED_BATCH_SIZE]; >> - uint32_t buf_offset = dev->vhost_hlen; >> + uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf); >> uint16_t flags, i; >> >> if (unlikely(avail_idx & PACKED_BATCH_MASK)) >> @@ -2354,7 +2355,7 @@ virtio_dev_tx_batch_packed(struct virtio_net *dev, >> struct rte_mbuf **pkts) >> { >> uint16_t avail_idx = vq->last_avail_idx; >> - uint32_t buf_offset = dev->vhost_hlen; >> + uint32_t buf_offset = sizeof(struct virtio_net_hdr_mrg_rxbuf); >> uintptr_t desc_addrs[PACKED_BATCH_SIZE]; >> uint16_t ids[PACKED_BATCH_SIZE]; >> uint16_t i; >> -- >> 2.26.2 >
On 10/1/20 12:11 PM, Maxime Coquelin wrote:
> First patch is fixing packed ring header len. Issue was
> reported by Marvin.
>
> Second patch is a (very) light optimization, to make used
> directly of the static hdr len value in packed-ring only
> functions.
>
> Maxime Coquelin (2):
> vhost: fix Virtio-net header len with packed ring
> vhost: use fixed Virtio-net header len packed ring
>
> lib/librte_vhost/vhost_user.c | 4 +++-
> lib/librte_vhost/virtio_net.c | 11 ++++++-----
> 2 files changed, 9 insertions(+), 6 deletions(-)
>
Applied to dpdk-next-virtio/main.
Thanks,
Maxime