[PATCH 0/6] Vhost checksum offload improvements

[PATCH 0/6] Vhost checksum offload improvements

[Maxime Coquelin]

This series aims at improving Vhost checksum offloading
support.

The first patch reverts overwritting MAC address in
testpmd CSUM forward mode. This is required to be able to
test checksum offloading with real traffic. MAC forwarding
mode should be used if the MAC addresses need to be
changed.

Second patch is a Vhost library fix to be compliant with
the Virtio specification, which requires that the
pseudo-header checksum is being set by the device when
offloading the checksum to the guest.

Third patch enables the compliant offloading mode of Vhost
library in Vhost PMD by default, since the legacy mode
violates the mbuf API by setting Tx flags in the receive
path. A new devarg is introduced for application willing
to use the legacy mode.

Fourth patch is just a small cleanup to represent a boolean
value as a boolean.

The two last patches introduces compatibility layers
that performs checksum in SW when the ethdev and Virtio
features are not aligned.

Note that the two last patches are not tagged as fixes
because they rely on the new compliant offload mode of
Vhost library, and so would casue an ABI breakage if
backported.

With this series, it is now possible to perform IO
forwarding between a vhost-user port and a Vitio-user
with kernel backend port even if the guest has negotiated
VIRTIO_NET_F_CSUM.

With csum forward mode, it now works whathever the
offloading configuration set either on Virtio or Ethdev
sides.

Maxime Coquelin (6):
  Revert "app/testpmd: modify mac in csum forwarding"
  vhost: fix missing enqueue pseudo-header calculation
  net/vhost: enable compliant offloading mode
  net/vhost: make VLAN stripping flag a boolean
  net/vhost: perform SW checksum in Rx path
  net/vhost: perform SW checksum in Tx path

 app/test-pmd/csumonly.c            |   4 -
 doc/guides/nics/features/vhost.ini |   1 +
 doc/guides/nics/vhost.rst          |   6 ++
 drivers/net/vhost/rte_eth_vhost.c  | 166 ++++++++++++++++++++++++++++-
 lib/vhost/virtio_net.c             |  10 ++
 5 files changed, 179 insertions(+), 8 deletions(-)

-- 
2.35.1

[PATCH 1/6] Revert "app/testpmd: modify mac in csum forwarding"

[Maxime Coquelin]

This patch reverts commit 10f4620f02e1 ("app/testpmd: modify mac in csum forwarding"),
as the checksum forwarding is expected to only perform
checksum and not also overwritte the source and destination
MAC addresses.

Doing so, we can test checksum offloading with real traffic
without breaking broadcast packets.

Fixes: 10f4620f02e1 ("app/testpmd: modify mac in csum forwarding")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 app/test-pmd/csumonly.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index cdb1920763..8b6665d6f3 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -894,10 +894,6 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
 		 * and inner headers */
 
 		eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
-		rte_ether_addr_copy(&peer_eth_addrs[fs->peer_addr],
-				&eth_hdr->dst_addr);
-		rte_ether_addr_copy(&ports[fs->tx_port].eth_addr,
-				&eth_hdr->src_addr);
 		parse_ethernet(eth_hdr, &info);
 		l3_hdr = (char *)eth_hdr + info.l2_len;
 
-- 
2.35.1

RE: [PATCH 1/6] Revert "app/testpmd: modify mac in csum forwarding"

[Xia, Chenbo]

+ testpmd maintainers

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, May 5, 2022 6:27 PM
> To: dev@dpdk.org; jasowang@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> david.marchand@redhat.com; olivier.matz@6wind.com
> Cc: stable@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH 1/6] Revert "app/testpmd: modify mac in csum forwarding"
> 
> This patch reverts commit 10f4620f02e1 ("app/testpmd: modify mac in csum
> forwarding"),
> as the checksum forwarding is expected to only perform
> checksum and not also overwritte the source and destination

overwrites

> MAC addresses.
> 
> Doing so, we can test checksum offloading with real traffic
> without breaking broadcast packets.
> 
> Fixes: 10f4620f02e1 ("app/testpmd: modify mac in csum forwarding")
> Cc: stable@dpdk.org

With above fixed:

Acked-by: Chenbo Xia <chenbo.xia@intel.com>

> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  app/test-pmd/csumonly.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
> index cdb1920763..8b6665d6f3 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -894,10 +894,6 @@ pkt_burst_checksum_forward(struct fwd_stream *fs)
>  		 * and inner headers */
> 
>  		eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
> -		rte_ether_addr_copy(&peer_eth_addrs[fs->peer_addr],
> -				&eth_hdr->dst_addr);
> -		rte_ether_addr_copy(&ports[fs->tx_port].eth_addr,
> -				&eth_hdr->src_addr);
>  		parse_ethernet(eth_hdr, &info);
>  		l3_hdr = (char *)eth_hdr + info.l2_len;
> 
> --
> 2.35.1

RE: [PATCH 1/6] Revert "app/testpmd: modify mac in csum forwarding"

[Zhang, Yuying]

Hi Coquelin,

Please fix the code style issue of commit log.

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: Monday, May 16, 2022 9:04 PM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; dev@dpdk.org;
> jasowang@redhat.com; david.marchand@redhat.com;
> olivier.matz@6wind.com
> Cc: stable@dpdk.org; Li, Xiaoyun <xiaoyun.li@intel.com>; Zhang, Yuying
> <yuying.zhang@intel.com>; Singh, Aman Deep <aman.deep.singh@intel.com>
> Subject: RE: [PATCH 1/6] Revert "app/testpmd: modify mac in csum
> forwarding"
> 
> + testpmd maintainers
> 
> > -----Original Message-----
> > From: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Sent: Thursday, May 5, 2022 6:27 PM
> > To: dev@dpdk.org; jasowang@redhat.com; Xia, Chenbo
> > <chenbo.xia@intel.com>; david.marchand@redhat.com;
> > olivier.matz@6wind.com
> > Cc: stable@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>
> > Subject: [PATCH 1/6] Revert "app/testpmd: modify mac in csum
> forwarding"
> >
> > This patch reverts commit 10f4620f02e1 ("app/testpmd: modify mac in
> > csum forwarding"), as the checksum forwarding is expected to only
> > perform checksum and not also overwritte the source and destination
> 
> overwrites
> 
> > MAC addresses.
> >
> > Doing so, we can test checksum offloading with real traffic without
> > breaking broadcast packets.
> >
> > Fixes: 10f4620f02e1 ("app/testpmd: modify mac in csum forwarding")
> > Cc: stable@dpdk.org
> 
> With above fixed:
> 
> Acked-by: Chenbo Xia <chenbo.xia@intel.com>
> 
> >
> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > ---
> >  app/test-pmd/csumonly.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c index
> > cdb1920763..8b6665d6f3 100644
> > --- a/app/test-pmd/csumonly.c
> > +++ b/app/test-pmd/csumonly.c
> > @@ -894,10 +894,6 @@ pkt_burst_checksum_forward(struct fwd_stream
> *fs)
> >  		 * and inner headers */
> >
> >  		eth_hdr = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
> > -		rte_ether_addr_copy(&peer_eth_addrs[fs->peer_addr],
> > -				&eth_hdr->dst_addr);
> > -		rte_ether_addr_copy(&ports[fs->tx_port].eth_addr,
> > -				&eth_hdr->src_addr);
> >  		parse_ethernet(eth_hdr, &info);
> >  		l3_hdr = (char *)eth_hdr + info.l2_len;
> >
> > --
> > 2.35.1

Re: [PATCH 1/6] Revert "app/testpmd: modify mac in csum forwarding"

[David Marchand]

On Thu, May 5, 2022 at 12:27 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This patch reverts commit 10f4620f02e1 ("app/testpmd: modify mac in csum forwarding"),
> as the checksum forwarding is expected to only perform
> checksum and not also overwritte the source and destination
> MAC addresses.
>
> Doing so, we can test checksum offloading with real traffic
> without breaking broadcast packets.
>
> Fixes: 10f4620f02e1 ("app/testpmd: modify mac in csum forwarding")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks Maxime, this lgtm.

Cc: dts ml since dts was dropping this code for some tests.

origin/main:test_plans/dpdk_gro_lib_cbdma_test_plan.rst:
     ether_addr_copy(&peer_eth_addrs[fs->peer_addr],
origin/main:test_plans/dpdk_gro_lib_cbdma_test_plan.rst:
     ether_addr_copy(&ports[fs->tx_port].eth_addr,
origin/main:test_plans/dpdk_gro_lib_test_plan.rst:
ether_addr_copy(&peer_eth_addrs[fs->peer_addr],
origin/main:test_plans/dpdk_gro_lib_test_plan.rst:
ether_addr_copy(&ports[fs->tx_port].eth_addr,
origin/main:test_plans/dpdk_gso_lib_test_plan.rst:
ether_addr_copy(&peer_eth_addrs[fs->peer_addr],
origin/main:test_plans/dpdk_gso_lib_test_plan.rst:
ether_addr_copy(&ports[fs->tx_port].eth_addr,
origin/main:test_plans/virtio_user_as_exceptional_path_test_plan.rst:
                  ether_addr_copy(&peer_eth_addrs[fs->peer_addr],
origin/main:test_plans/virtio_user_as_exceptional_path_test_plan.rst:
                  ether_addr_copy(&ports[fs->tx_port].eth_addr,
origin/main:tests/TestSuite_dpdk_gro_lib.py:            "sed -i
'/ether_addr_copy(&peer_eth/i\#if 0' ./app/test-pmd/csumonly.c", "#"
origin/main:tests/TestSuite_dpdk_gro_lib_cbdma.py:            "sed -i
'/ether_addr_copy(&peer_eth/i\#if 0' ./app/test-pmd/csumonly.c", "#"
origin/main:tests/TestSuite_dpdk_gso_lib.py:            "sed -i
'/ether_addr_copy(&peer_eth/i\#if 0' ./app/test-pmd/csumonly.c", "#"
origin/main:tests/TestSuite_virtio_user_as_exceptional_path.py:
    "sed -i '/ether_addr_copy(&peer_eth/i\#if 0'
./app/test-pmd/csumonly.c", "#"



-- 
David Marchand

[PATCH 2/6] vhost: fix missing enqueue pseudo-header calculation

[Maxime Coquelin]

The Virtio specification requires that in case of checksum
offloading, the pseudo-header checksum must be set in the
L4 header.

When received from another Vhost-user port, the packet
checksum might already contain the pseudo-header checksum
but we have no way to know it. So we have no other choice
than doing the pseudo-header checksum systematically.

This patch handles this using the rte_net_intel_cksum_prepare()
helper.

Fixes: 859b480d5afd ("vhost: add guest offload setting")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/virtio_net.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index 5f432b0d77..c0ff3357a8 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -548,6 +548,16 @@ virtio_enqueue_offload(struct rte_mbuf *m_buf, struct virtio_net_hdr *net_hdr)
 		csum_l4 |= RTE_MBUF_F_TX_TCP_CKSUM;
 
 	if (csum_l4) {
+		/*
+		 * Pseudo-header checksum must be set as per Virtio spec.
+		 *
+		 * Note: We don't propagate rte_net_intel_cksum_prepare()
+		 * errors, as it would have an impact on performance, and an
+		 * error would mean the packet is dropped by the guest instead
+		 * of being dropped here.
+		 */
+		rte_net_intel_cksum_prepare(m_buf);
+
 		net_hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
 		net_hdr->csum_start = m_buf->l2_len + m_buf->l3_len;
 
-- 
2.35.1

RE: [PATCH 2/6] vhost: fix missing enqueue pseudo-header calculation

[Xia, Chenbo]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, May 5, 2022 6:27 PM
> To: dev@dpdk.org; jasowang@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> david.marchand@redhat.com; olivier.matz@6wind.com
> Cc: stable@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH 2/6] vhost: fix missing enqueue pseudo-header calculation
> 
> The Virtio specification requires that in case of checksum
> offloading, the pseudo-header checksum must be set in the
> L4 header.
> 
> When received from another Vhost-user port, the packet
> checksum might already contain the pseudo-header checksum
> but we have no way to know it. So we have no other choice
> than doing the pseudo-header checksum systematically.
> 
> This patch handles this using the rte_net_intel_cksum_prepare()
> helper.
> 
> Fixes: 859b480d5afd ("vhost: add guest offload setting")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/vhost/virtio_net.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index 5f432b0d77..c0ff3357a8 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -548,6 +548,16 @@ virtio_enqueue_offload(struct rte_mbuf *m_buf, struct
> virtio_net_hdr *net_hdr)
>  		csum_l4 |= RTE_MBUF_F_TX_TCP_CKSUM;
> 
>  	if (csum_l4) {
> +		/*
> +		 * Pseudo-header checksum must be set as per Virtio spec.
> +		 *
> +		 * Note: We don't propagate rte_net_intel_cksum_prepare()
> +		 * errors, as it would have an impact on performance, and an
> +		 * error would mean the packet is dropped by the guest instead
> +		 * of being dropped here.
> +		 */
> +		rte_net_intel_cksum_prepare(m_buf);
> +
>  		net_hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
>  		net_hdr->csum_start = m_buf->l2_len + m_buf->l3_len;
> 
> --
> 2.35.1

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

[PATCH 3/6] net/vhost: enable compliant offloading mode

[Maxime Coquelin]

This patch enables the compliant offloading flags mode by
default, which prevents the Rx path to set Tx offload flags,
which is illegal. A new legacy-ol-flags devarg is introduced
to enable the legacy behaviour.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 doc/guides/nics/vhost.rst         |  6 ++++++
 drivers/net/vhost/rte_eth_vhost.c | 19 ++++++++++++++++---
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/doc/guides/nics/vhost.rst b/doc/guides/nics/vhost.rst
index ee802ec4a8..d7c0e2ade8 100644
--- a/doc/guides/nics/vhost.rst
+++ b/doc/guides/nics/vhost.rst
@@ -64,6 +64,12 @@ The user can specify below arguments in `--vdev` option.
     It is used to enable external buffer support in vhost library.
     (Default: 0 (disabled))
 
+#.  ``legacy-ol-flags``:
+
+    It is used to restore legacy behavior for offloading that was not
+    compliant with offloading API.
+    (Default: 0 (disabled))
+
 Vhost PMD event handling
 ------------------------
 
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 070f0e6dfd..0a2e8d9b29 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -31,9 +31,10 @@ enum {VIRTIO_RXQ, VIRTIO_TXQ, VIRTIO_QNUM};
 #define ETH_VHOST_CLIENT_ARG		"client"
 #define ETH_VHOST_IOMMU_SUPPORT		"iommu-support"
 #define ETH_VHOST_POSTCOPY_SUPPORT	"postcopy-support"
-#define ETH_VHOST_VIRTIO_NET_F_HOST_TSO "tso"
-#define ETH_VHOST_LINEAR_BUF  "linear-buffer"
-#define ETH_VHOST_EXT_BUF  "ext-buffer"
+#define ETH_VHOST_VIRTIO_NET_F_HOST_TSO	"tso"
+#define ETH_VHOST_LINEAR_BUF		"linear-buffer"
+#define ETH_VHOST_EXT_BUF		"ext-buffer"
+#define ETH_VHOST_LEGACY_OL_FLAGS	"legacy-ol-flags"
 #define VHOST_MAX_PKT_BURST 32
 
 static const char *valid_arguments[] = {
@@ -1563,6 +1564,7 @@ rte_pmd_vhost_probe(struct rte_vdev_device *dev)
 	int tso = 0;
 	int linear_buf = 0;
 	int ext_buf = 0;
+	int legacy_ol_flags = 0;
 	struct rte_eth_dev *eth_dev;
 	const char *name = rte_vdev_device_name(dev);
 
@@ -1672,6 +1674,17 @@ rte_pmd_vhost_probe(struct rte_vdev_device *dev)
 			flags |= RTE_VHOST_USER_EXTBUF_SUPPORT;
 	}
 
+	if (rte_kvargs_count(kvlist, ETH_VHOST_LEGACY_OL_FLAGS) == 1) {
+		ret = rte_kvargs_process(kvlist,
+				ETH_VHOST_LEGACY_OL_FLAGS,
+				&open_int, &legacy_ol_flags);
+		if (ret < 0)
+			goto out_free;
+	}
+
+	if (legacy_ol_flags == 0)
+		flags |= RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS;
+
 	if (dev->device.numa_node == SOCKET_ID_ANY)
 		dev->device.numa_node = rte_socket_id();
 
-- 
2.35.1

RE: [PATCH 3/6] net/vhost: enable compliant offloading mode

[Xia, Chenbo]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, May 5, 2022 6:27 PM
> To: dev@dpdk.org; jasowang@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> david.marchand@redhat.com; olivier.matz@6wind.com
> Cc: stable@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH 3/6] net/vhost: enable compliant offloading mode
> 
> This patch enables the compliant offloading flags mode by
> default, which prevents the Rx path to set Tx offload flags,
> which is illegal. A new legacy-ol-flags devarg is introduced
> to enable the legacy behaviour.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  doc/guides/nics/vhost.rst         |  6 ++++++
>  drivers/net/vhost/rte_eth_vhost.c | 19 ++++++++++++++++---
>  2 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/doc/guides/nics/vhost.rst b/doc/guides/nics/vhost.rst
> index ee802ec4a8..d7c0e2ade8 100644
> --- a/doc/guides/nics/vhost.rst
> +++ b/doc/guides/nics/vhost.rst
> @@ -64,6 +64,12 @@ The user can specify below arguments in `--vdev` option.
>      It is used to enable external buffer support in vhost library.
>      (Default: 0 (disabled))
> 
> +#.  ``legacy-ol-flags``:
> +
> +    It is used to restore legacy behavior for offloading that was not
> +    compliant with offloading API.
> +    (Default: 0 (disabled))
> +
>  Vhost PMD event handling
>  ------------------------
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c
> b/drivers/net/vhost/rte_eth_vhost.c
> index 070f0e6dfd..0a2e8d9b29 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -31,9 +31,10 @@ enum {VIRTIO_RXQ, VIRTIO_TXQ, VIRTIO_QNUM};
>  #define ETH_VHOST_CLIENT_ARG		"client"
>  #define ETH_VHOST_IOMMU_SUPPORT		"iommu-support"
>  #define ETH_VHOST_POSTCOPY_SUPPORT	"postcopy-support"
> -#define ETH_VHOST_VIRTIO_NET_F_HOST_TSO "tso"
> -#define ETH_VHOST_LINEAR_BUF  "linear-buffer"
> -#define ETH_VHOST_EXT_BUF  "ext-buffer"
> +#define ETH_VHOST_VIRTIO_NET_F_HOST_TSO	"tso"
> +#define ETH_VHOST_LINEAR_BUF		"linear-buffer"
> +#define ETH_VHOST_EXT_BUF		"ext-buffer"
> +#define ETH_VHOST_LEGACY_OL_FLAGS	"legacy-ol-flags"
>  #define VHOST_MAX_PKT_BURST 32
> 
>  static const char *valid_arguments[] = {
> @@ -1563,6 +1564,7 @@ rte_pmd_vhost_probe(struct rte_vdev_device *dev)
>  	int tso = 0;
>  	int linear_buf = 0;
>  	int ext_buf = 0;
> +	int legacy_ol_flags = 0;
>  	struct rte_eth_dev *eth_dev;
>  	const char *name = rte_vdev_device_name(dev);
> 
> @@ -1672,6 +1674,17 @@ rte_pmd_vhost_probe(struct rte_vdev_device *dev)
>  			flags |= RTE_VHOST_USER_EXTBUF_SUPPORT;
>  	}
> 
> +	if (rte_kvargs_count(kvlist, ETH_VHOST_LEGACY_OL_FLAGS) == 1) {
> +		ret = rte_kvargs_process(kvlist,
> +				ETH_VHOST_LEGACY_OL_FLAGS,
> +				&open_int, &legacy_ol_flags);
> +		if (ret < 0)
> +			goto out_free;
> +	}
> +
> +	if (legacy_ol_flags == 0)
> +		flags |= RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS;

Putting this check into above '{}' like other option does, will look better.

Thanks,
Chenbo

> +
>  	if (dev->device.numa_node == SOCKET_ID_ANY)
>  		dev->device.numa_node = rte_socket_id();
> 
> --
> 2.35.1

Re: [PATCH 3/6] net/vhost: enable compliant offloading mode

[Maxime Coquelin]

Hi Chenbo,

On 5/16/22 15:26, Xia, Chenbo wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Thursday, May 5, 2022 6:27 PM
>> To: dev@dpdk.org; jasowang@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
>> david.marchand@redhat.com; olivier.matz@6wind.com
>> Cc: stable@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [PATCH 3/6] net/vhost: enable compliant offloading mode
>>
>> This patch enables the compliant offloading flags mode by
>> default, which prevents the Rx path to set Tx offload flags,
>> which is illegal. A new legacy-ol-flags devarg is introduced
>> to enable the legacy behaviour.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   doc/guides/nics/vhost.rst         |  6 ++++++
>>   drivers/net/vhost/rte_eth_vhost.c | 19 ++++++++++++++++---
>>   2 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/doc/guides/nics/vhost.rst b/doc/guides/nics/vhost.rst
>> index ee802ec4a8..d7c0e2ade8 100644
>> --- a/doc/guides/nics/vhost.rst
>> +++ b/doc/guides/nics/vhost.rst
>> @@ -64,6 +64,12 @@ The user can specify below arguments in `--vdev` option.
>>       It is used to enable external buffer support in vhost library.
>>       (Default: 0 (disabled))
>>
>> +#.  ``legacy-ol-flags``:
>> +
>> +    It is used to restore legacy behavior for offloading that was not
>> +    compliant with offloading API.
>> +    (Default: 0 (disabled))
>> +
>>   Vhost PMD event handling
>>   ------------------------
>>
>> diff --git a/drivers/net/vhost/rte_eth_vhost.c
>> b/drivers/net/vhost/rte_eth_vhost.c
>> index 070f0e6dfd..0a2e8d9b29 100644
>> --- a/drivers/net/vhost/rte_eth_vhost.c
>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>> @@ -31,9 +31,10 @@ enum {VIRTIO_RXQ, VIRTIO_TXQ, VIRTIO_QNUM};
>>   #define ETH_VHOST_CLIENT_ARG		"client"
>>   #define ETH_VHOST_IOMMU_SUPPORT		"iommu-support"
>>   #define ETH_VHOST_POSTCOPY_SUPPORT	"postcopy-support"
>> -#define ETH_VHOST_VIRTIO_NET_F_HOST_TSO "tso"
>> -#define ETH_VHOST_LINEAR_BUF  "linear-buffer"
>> -#define ETH_VHOST_EXT_BUF  "ext-buffer"
>> +#define ETH_VHOST_VIRTIO_NET_F_HOST_TSO	"tso"
>> +#define ETH_VHOST_LINEAR_BUF		"linear-buffer"
>> +#define ETH_VHOST_EXT_BUF		"ext-buffer"
>> +#define ETH_VHOST_LEGACY_OL_FLAGS	"legacy-ol-flags"
>>   #define VHOST_MAX_PKT_BURST 32
>>
>>   static const char *valid_arguments[] = {
>> @@ -1563,6 +1564,7 @@ rte_pmd_vhost_probe(struct rte_vdev_device *dev)
>>   	int tso = 0;
>>   	int linear_buf = 0;
>>   	int ext_buf = 0;
>> +	int legacy_ol_flags = 0;
>>   	struct rte_eth_dev *eth_dev;
>>   	const char *name = rte_vdev_device_name(dev);
>>
>> @@ -1672,6 +1674,17 @@ rte_pmd_vhost_probe(struct rte_vdev_device *dev)
>>   			flags |= RTE_VHOST_USER_EXTBUF_SUPPORT;
>>   	}
>>
>> +	if (rte_kvargs_count(kvlist, ETH_VHOST_LEGACY_OL_FLAGS) == 1) {
>> +		ret = rte_kvargs_process(kvlist,
>> +				ETH_VHOST_LEGACY_OL_FLAGS,
>> +				&open_int, &legacy_ol_flags);
>> +		if (ret < 0)
>> +			goto out_free;
>> +	}
>> +
>> +	if (legacy_ol_flags == 0)
>> +		flags |= RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS;
> 
> Putting this check into above '{}' like other option does, will look better.

Actually, it would change the behavior because we want to set the flag
even if legacy devrarg is not passed.

Regards,
Maxime

> Thanks,
> Chenbo
> 
>> +
>>   	if (dev->device.numa_node == SOCKET_ID_ANY)
>>   		dev->device.numa_node = rte_socket_id();
>>
>> --
>> 2.35.1
> 

RE: [PATCH 3/6] net/vhost: enable compliant offloading mode

[Xia, Chenbo]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Monday, May 16, 2022 9:29 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org; jasowang@redhat.com;
> david.marchand@redhat.com; olivier.matz@6wind.com
> Cc: stable@dpdk.org
> Subject: Re: [PATCH 3/6] net/vhost: enable compliant offloading mode
> 
> Hi Chenbo,
> 
> On 5/16/22 15:26, Xia, Chenbo wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Thursday, May 5, 2022 6:27 PM
> >> To: dev@dpdk.org; jasowang@redhat.com; Xia, Chenbo
> <chenbo.xia@intel.com>;
> >> david.marchand@redhat.com; olivier.matz@6wind.com
> >> Cc: stable@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Subject: [PATCH 3/6] net/vhost: enable compliant offloading mode
> >>
> >> This patch enables the compliant offloading flags mode by
> >> default, which prevents the Rx path to set Tx offload flags,
> >> which is illegal. A new legacy-ol-flags devarg is introduced
> >> to enable the legacy behaviour.
> >>
> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> ---
> >>   doc/guides/nics/vhost.rst         |  6 ++++++
> >>   drivers/net/vhost/rte_eth_vhost.c | 19 ++++++++++++++++---
> >>   2 files changed, 22 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/doc/guides/nics/vhost.rst b/doc/guides/nics/vhost.rst
> >> index ee802ec4a8..d7c0e2ade8 100644
> >> --- a/doc/guides/nics/vhost.rst
> >> +++ b/doc/guides/nics/vhost.rst
> >> @@ -64,6 +64,12 @@ The user can specify below arguments in `--vdev`
> option.
> >>       It is used to enable external buffer support in vhost library.
> >>       (Default: 0 (disabled))
> >>
> >> +#.  ``legacy-ol-flags``:
> >> +
> >> +    It is used to restore legacy behavior for offloading that was not
> >> +    compliant with offloading API.
> >> +    (Default: 0 (disabled))
> >> +
> >>   Vhost PMD event handling
> >>   ------------------------
> >>
> >> diff --git a/drivers/net/vhost/rte_eth_vhost.c
> >> b/drivers/net/vhost/rte_eth_vhost.c
> >> index 070f0e6dfd..0a2e8d9b29 100644
> >> --- a/drivers/net/vhost/rte_eth_vhost.c
> >> +++ b/drivers/net/vhost/rte_eth_vhost.c
> >> @@ -31,9 +31,10 @@ enum {VIRTIO_RXQ, VIRTIO_TXQ, VIRTIO_QNUM};
> >>   #define ETH_VHOST_CLIENT_ARG		"client"
> >>   #define ETH_VHOST_IOMMU_SUPPORT		"iommu-support"
> >>   #define ETH_VHOST_POSTCOPY_SUPPORT	"postcopy-support"
> >> -#define ETH_VHOST_VIRTIO_NET_F_HOST_TSO "tso"
> >> -#define ETH_VHOST_LINEAR_BUF  "linear-buffer"
> >> -#define ETH_VHOST_EXT_BUF  "ext-buffer"
> >> +#define ETH_VHOST_VIRTIO_NET_F_HOST_TSO	"tso"
> >> +#define ETH_VHOST_LINEAR_BUF		"linear-buffer"
> >> +#define ETH_VHOST_EXT_BUF		"ext-buffer"
> >> +#define ETH_VHOST_LEGACY_OL_FLAGS	"legacy-ol-flags"
> >>   #define VHOST_MAX_PKT_BURST 32
> >>
> >>   static const char *valid_arguments[] = {
> >> @@ -1563,6 +1564,7 @@ rte_pmd_vhost_probe(struct rte_vdev_device *dev)
> >>   	int tso = 0;
> >>   	int linear_buf = 0;
> >>   	int ext_buf = 0;
> >> +	int legacy_ol_flags = 0;
> >>   	struct rte_eth_dev *eth_dev;
> >>   	const char *name = rte_vdev_device_name(dev);
> >>
> >> @@ -1672,6 +1674,17 @@ rte_pmd_vhost_probe(struct rte_vdev_device *dev)
> >>   			flags |= RTE_VHOST_USER_EXTBUF_SUPPORT;
> >>   	}
> >>
> >> +	if (rte_kvargs_count(kvlist, ETH_VHOST_LEGACY_OL_FLAGS) == 1) {
> >> +		ret = rte_kvargs_process(kvlist,
> >> +				ETH_VHOST_LEGACY_OL_FLAGS,
> >> +				&open_int, &legacy_ol_flags);
> >> +		if (ret < 0)
> >> +			goto out_free;
> >> +	}
> >> +
> >> +	if (legacy_ol_flags == 0)
> >> +		flags |= RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS;
> >
> > Putting this check into above '{}' like other option does, will look
> better.
> 
> Actually, it would change the behavior because we want to set the flag
> even if legacy devrarg is not passed.

You're right...

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

> 
> Regards,
> Maxime
> 
> > Thanks,
> > Chenbo
> >
> >> +
> >>   	if (dev->device.numa_node == SOCKET_ID_ANY)
> >>   		dev->device.numa_node = rte_socket_id();
> >>
> >> --
> >> 2.35.1
> >

RE: [PATCH 3/6] net/vhost: enable compliant offloading mode

[Ma, WenwuX]

Hi Maxime,

> -----Original Message-----
> From: Xia, Chenbo <chenbo.xia@intel.com>
> Sent: 2022年5月16日 21:40
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; dev@dpdk.org;
> jasowang@redhat.com; david.marchand@redhat.com;
> olivier.matz@6wind.com
> Cc: stable@dpdk.org
> Subject: RE: [PATCH 3/6] net/vhost: enable compliant offloading mode
> 
> > -----Original Message-----
> > From: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Sent: Monday, May 16, 2022 9:29 PM
> > To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org;
> > jasowang@redhat.com; david.marchand@redhat.com;
> olivier.matz@6wind.com
> > Cc: stable@dpdk.org
> > Subject: Re: [PATCH 3/6] net/vhost: enable compliant offloading mode
> >
> > Hi Chenbo,
> >
> > On 5/16/22 15:26, Xia, Chenbo wrote:
> > > Hi Maxime,
> > >
> > >> -----Original Message-----
> > >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> > >> Sent: Thursday, May 5, 2022 6:27 PM
> > >> To: dev@dpdk.org; jasowang@redhat.com; Xia, Chenbo
> > <chenbo.xia@intel.com>;
> > >> david.marchand@redhat.com; olivier.matz@6wind.com
> > >> Cc: stable@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>
> > >> Subject: [PATCH 3/6] net/vhost: enable compliant offloading mode
> > >>
> > >> This patch enables the compliant offloading flags mode by default,
> > >> which prevents the Rx path to set Tx offload flags, which is
> > >> illegal. A new legacy-ol-flags devarg is introduced to enable the
> > >> legacy behaviour.
> > >>
> > >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > >> ---
> > >>   doc/guides/nics/vhost.rst         |  6 ++++++
> > >>   drivers/net/vhost/rte_eth_vhost.c | 19 ++++++++++++++++---
> > >>   2 files changed, 22 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/doc/guides/nics/vhost.rst b/doc/guides/nics/vhost.rst
> > >> index ee802ec4a8..d7c0e2ade8 100644
> > >> --- a/doc/guides/nics/vhost.rst
> > >> +++ b/doc/guides/nics/vhost.rst
> > >> @@ -64,6 +64,12 @@ The user can specify below arguments in `--vdev`
> > option.
> > >>       It is used to enable external buffer support in vhost library.
> > >>       (Default: 0 (disabled))
> > >>
> > >> +#.  ``legacy-ol-flags``:
> > >> +
> > >> +    It is used to restore legacy behavior for offloading that was not
> > >> +    compliant with offloading API.
> > >> +    (Default: 0 (disabled))
> > >> +
> > >>   Vhost PMD event handling
> > >>   ------------------------
> > >>
> > >> diff --git a/drivers/net/vhost/rte_eth_vhost.c
> > >> b/drivers/net/vhost/rte_eth_vhost.c
> > >> index 070f0e6dfd..0a2e8d9b29 100644
> > >> --- a/drivers/net/vhost/rte_eth_vhost.c
> > >> +++ b/drivers/net/vhost/rte_eth_vhost.c
> > >> @@ -31,9 +31,10 @@ enum {VIRTIO_RXQ, VIRTIO_TXQ,
> VIRTIO_QNUM};
> > >>   #define ETH_VHOST_CLIENT_ARG		"client"
> > >>   #define ETH_VHOST_IOMMU_SUPPORT		"iommu-support"
> > >>   #define ETH_VHOST_POSTCOPY_SUPPORT	"postcopy-support"
> > >> -#define ETH_VHOST_VIRTIO_NET_F_HOST_TSO "tso"
> > >> -#define ETH_VHOST_LINEAR_BUF  "linear-buffer"
> > >> -#define ETH_VHOST_EXT_BUF  "ext-buffer"
> > >> +#define ETH_VHOST_VIRTIO_NET_F_HOST_TSO	"tso"
> > >> +#define ETH_VHOST_LINEAR_BUF		"linear-buffer"
> > >> +#define ETH_VHOST_EXT_BUF		"ext-buffer"
> > >> +#define ETH_VHOST_LEGACY_OL_FLAGS	"legacy-ol-flags"
> > >>   #define VHOST_MAX_PKT_BURST 32
> > >>

ETH_VHOST_LEGACY_OL_FLAGS should be added into valid_arguments array.

static const char *valid_arguments[] = {
        ETH_VHOST_IFACE_ARG,
        ETH_VHOST_QUEUES_ARG,
        ETH_VHOST_CLIENT_ARG,
        ETH_VHOST_IOMMU_SUPPORT,
        ETH_VHOST_POSTCOPY_SUPPORT,
        ETH_VHOST_VIRTIO_NET_F_HOST_TSO,
        ETH_VHOST_LINEAR_BUF,
        ETH_VHOST_EXT_BUF,
        NULL
};

> > >>   static const char *valid_arguments[] = { @@ -1563,6 +1564,7 @@
> > >> rte_pmd_vhost_probe(struct rte_vdev_device *dev)
> > >>   	int tso = 0;
> > >>   	int linear_buf = 0;
> > >>   	int ext_buf = 0;
> > >> +	int legacy_ol_flags = 0;
> > >>   	struct rte_eth_dev *eth_dev;
> > >>   	const char *name = rte_vdev_device_name(dev);
> > >>
> > >> @@ -1672,6 +1674,17 @@ rte_pmd_vhost_probe(struct
> rte_vdev_device *dev)
> > >>   			flags |= RTE_VHOST_USER_EXTBUF_SUPPORT;
> > >>   	}
> > >>
> > >> +	if (rte_kvargs_count(kvlist, ETH_VHOST_LEGACY_OL_FLAGS) == 1) {
> > >> +		ret = rte_kvargs_process(kvlist,
> > >> +				ETH_VHOST_LEGACY_OL_FLAGS,
> > >> +				&open_int, &legacy_ol_flags);
> > >> +		if (ret < 0)
> > >> +			goto out_free;
> > >> +	}
> > >> +
> > >> +	if (legacy_ol_flags == 0)
> > >> +		flags |= RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS;
> > >
> > > Putting this check into above '{}' like other option does, will look
> > better.
> >
> > Actually, it would change the behavior because we want to set the flag
> > even if legacy devrarg is not passed.
> 
> You're right...
> 
> Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
> 
> >
> > Regards,
> > Maxime
> >
> > > Thanks,
> > > Chenbo
> > >
> > >> +
> > >>   	if (dev->device.numa_node == SOCKET_ID_ANY)
> > >>   		dev->device.numa_node = rte_socket_id();
> > >>
> > >> --
> > >> 2.35.1
> > >

Re: [PATCH 3/6] net/vhost: enable compliant offloading mode

[Maxime Coquelin]

Hi Wenwu,

On 6/7/22 03:19, Ma, WenwuX wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Xia, Chenbo <chenbo.xia@intel.com>
>> Sent: 2022年5月16日 21:40
>> To: Maxime Coquelin <maxime.coquelin@redhat.com>; dev@dpdk.org;
>> jasowang@redhat.com; david.marchand@redhat.com;
>> olivier.matz@6wind.com
>> Cc: stable@dpdk.org
>> Subject: RE: [PATCH 3/6] net/vhost: enable compliant offloading mode
>>
>>> -----Original Message-----
>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Sent: Monday, May 16, 2022 9:29 PM
>>> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org;
>>> jasowang@redhat.com; david.marchand@redhat.com;
>> olivier.matz@6wind.com
>>> Cc: stable@dpdk.org
>>> Subject: Re: [PATCH 3/6] net/vhost: enable compliant offloading mode
>>>
>>> Hi Chenbo,
>>>
>>> On 5/16/22 15:26, Xia, Chenbo wrote:
>>>> Hi Maxime,
>>>>
>>>>> -----Original Message-----
>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>> Sent: Thursday, May 5, 2022 6:27 PM
>>>>> To: dev@dpdk.org; jasowang@redhat.com; Xia, Chenbo
>>> <chenbo.xia@intel.com>;
>>>>> david.marchand@redhat.com; olivier.matz@6wind.com
>>>>> Cc: stable@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>> Subject: [PATCH 3/6] net/vhost: enable compliant offloading mode
>>>>>
>>>>> This patch enables the compliant offloading flags mode by default,
>>>>> which prevents the Rx path to set Tx offload flags, which is
>>>>> illegal. A new legacy-ol-flags devarg is introduced to enable the
>>>>> legacy behaviour.
>>>>>
>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>> ---
>>>>>    doc/guides/nics/vhost.rst         |  6 ++++++
>>>>>    drivers/net/vhost/rte_eth_vhost.c | 19 ++++++++++++++++---
>>>>>    2 files changed, 22 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/doc/guides/nics/vhost.rst b/doc/guides/nics/vhost.rst
>>>>> index ee802ec4a8..d7c0e2ade8 100644
>>>>> --- a/doc/guides/nics/vhost.rst
>>>>> +++ b/doc/guides/nics/vhost.rst
>>>>> @@ -64,6 +64,12 @@ The user can specify below arguments in `--vdev`
>>> option.
>>>>>        It is used to enable external buffer support in vhost library.
>>>>>        (Default: 0 (disabled))
>>>>>
>>>>> +#.  ``legacy-ol-flags``:
>>>>> +
>>>>> +    It is used to restore legacy behavior for offloading that was not
>>>>> +    compliant with offloading API.
>>>>> +    (Default: 0 (disabled))
>>>>> +
>>>>>    Vhost PMD event handling
>>>>>    ------------------------
>>>>>
>>>>> diff --git a/drivers/net/vhost/rte_eth_vhost.c
>>>>> b/drivers/net/vhost/rte_eth_vhost.c
>>>>> index 070f0e6dfd..0a2e8d9b29 100644
>>>>> --- a/drivers/net/vhost/rte_eth_vhost.c
>>>>> +++ b/drivers/net/vhost/rte_eth_vhost.c
>>>>> @@ -31,9 +31,10 @@ enum {VIRTIO_RXQ, VIRTIO_TXQ,
>> VIRTIO_QNUM};
>>>>>    #define ETH_VHOST_CLIENT_ARG		"client"
>>>>>    #define ETH_VHOST_IOMMU_SUPPORT		"iommu-support"
>>>>>    #define ETH_VHOST_POSTCOPY_SUPPORT	"postcopy-support"
>>>>> -#define ETH_VHOST_VIRTIO_NET_F_HOST_TSO "tso"
>>>>> -#define ETH_VHOST_LINEAR_BUF  "linear-buffer"
>>>>> -#define ETH_VHOST_EXT_BUF  "ext-buffer"
>>>>> +#define ETH_VHOST_VIRTIO_NET_F_HOST_TSO	"tso"
>>>>> +#define ETH_VHOST_LINEAR_BUF		"linear-buffer"
>>>>> +#define ETH_VHOST_EXT_BUF		"ext-buffer"
>>>>> +#define ETH_VHOST_LEGACY_OL_FLAGS	"legacy-ol-flags"
>>>>>    #define VHOST_MAX_PKT_BURST 32
>>>>>
> 
> ETH_VHOST_LEGACY_OL_FLAGS should be added into valid_arguments array.
> 
> static const char *valid_arguments[] = {
>          ETH_VHOST_IFACE_ARG,
>          ETH_VHOST_QUEUES_ARG,
>          ETH_VHOST_CLIENT_ARG,
>          ETH_VHOST_IOMMU_SUPPORT,
>          ETH_VHOST_POSTCOPY_SUPPORT,
>          ETH_VHOST_VIRTIO_NET_F_HOST_TSO,
>          ETH_VHOST_LINEAR_BUF,
>          ETH_VHOST_EXT_BUF,
>          NULL
> };

Thanks, good catch!
I missed to test this new devarg addition, fixing it for v2.

Maxime

[PATCH 4/6] net/vhost: make VLAN stripping flag a boolean

[Maxime Coquelin]

This trivial patch makes the vlan_strip field of the
pmd_internal struct a boolean, since it is handled as
such.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/vhost/rte_eth_vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index 0a2e8d9b29..baa973ad6d 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -110,7 +110,7 @@ struct pmd_internal {
 	uint16_t max_queues;
 	int vid;
 	rte_atomic32_t started;
-	uint8_t vlan_strip;
+	bool vlan_strip;
 };
 
 struct internal_list {
-- 
2.35.1

RE: [PATCH 4/6] net/vhost: make VLAN stripping flag a boolean

[Xia, Chenbo]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, May 5, 2022 6:27 PM
> To: dev@dpdk.org; jasowang@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> david.marchand@redhat.com; olivier.matz@6wind.com
> Cc: stable@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH 4/6] net/vhost: make VLAN stripping flag a boolean
> 
> This trivial patch makes the vlan_strip field of the
> pmd_internal struct a boolean, since it is handled as
> such.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/vhost/rte_eth_vhost.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c
> b/drivers/net/vhost/rte_eth_vhost.c
> index 0a2e8d9b29..baa973ad6d 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -110,7 +110,7 @@ struct pmd_internal {
>  	uint16_t max_queues;
>  	int vid;
>  	rte_atomic32_t started;
> -	uint8_t vlan_strip;
> +	bool vlan_strip;
>  };
> 
>  struct internal_list {
> --
> 2.35.1

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

[PATCH 5/6] net/vhost: perform SW checksum in Rx path

[Maxime Coquelin]

Virtio specification supports host checksum offloading
for L4, which is enabled with VIRTIO_NET_F_CSUM feature
negotiation. However, the Vhost PMD does not advertise
Rx checksum offload capabilities, so we can end-up with
the VIRTIO_NET_F_CSUM feature being negociated, implying
the Vhost library returns packets with checksum being
offloaded while the application did not request for it.

Advertising these offload capabilities at the ethdev level
is not enough, because we could still end-up with the
application not enabling these offloads while the guest
still negotiate them.

This patch advertizes the Rx checksum offload capabilities,
and introduces a compatibility layer to cover the case
VIRTIO_NET_F_CSUM has been negotiated but the application
does not configure the Rx checksum offloads. This function
performis the L4 Rx checksum in SW for UDP and TCP. Note
that it is not needed to calculate the pseudo-header
checksum, because the Virtio specification requires that
the driver do it.

This patch does not advertize SCTP checksum offloading
capability for now, but it could be handled later if the
need arises.

Reported-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 doc/guides/nics/features/vhost.ini |  1 +
 drivers/net/vhost/rte_eth_vhost.c  | 83 ++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/doc/guides/nics/features/vhost.ini b/doc/guides/nics/features/vhost.ini
index ef81abb439..15f4dfe5e8 100644
--- a/doc/guides/nics/features/vhost.ini
+++ b/doc/guides/nics/features/vhost.ini
@@ -7,6 +7,7 @@
 Link status          = Y
 Free Tx mbuf on demand = Y
 Queue status event   = Y
+L4 checksum offload  = P
 Basic stats          = Y
 Extended stats       = Y
 x86-32               = Y
diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index baa973ad6d..d5303f7368 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -12,6 +12,7 @@
 #include <ethdev_vdev.h>
 #include <rte_malloc.h>
 #include <rte_memcpy.h>
+#include <rte_net.h>
 #include <rte_bus_vdev.h>
 #include <rte_kvargs.h>
 #include <rte_vhost.h>
@@ -107,10 +108,12 @@ struct pmd_internal {
 	char *iface_name;
 	uint64_t flags;
 	uint64_t disable_flags;
+	uint64_t features;
 	uint16_t max_queues;
 	int vid;
 	rte_atomic32_t started;
 	bool vlan_strip;
+	bool rx_sw_csum;
 };
 
 struct internal_list {
@@ -362,6 +365,70 @@ vhost_update_single_packet_xstats(struct vhost_queue *vq, struct rte_mbuf *buf)
 	vhost_count_xcast_packets(vq, buf);
 }
 
+static void
+vhost_dev_csum_configure(struct rte_eth_dev *eth_dev)
+{
+	struct pmd_internal *internal = eth_dev->data->dev_private;
+	const struct rte_eth_rxmode *rxmode = &eth_dev->data->dev_conf.rxmode;
+
+	internal->rx_sw_csum = false;
+
+	/* SW checksum is not compatible with legacy mode */
+	if (!(internal->flags & RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS))
+		return;
+
+	if (internal->features & (1ULL << VIRTIO_NET_F_CSUM)) {
+		if (!(rxmode->offloads &
+				(RTE_ETH_RX_OFFLOAD_UDP_CKSUM | RTE_ETH_RX_OFFLOAD_TCP_CKSUM))) {
+			VHOST_LOG(NOTICE, "Rx csum will be done in SW, may impact performance.");
+			internal->rx_sw_csum = true;
+		}
+	}
+}
+
+static void
+vhost_dev_rx_sw_csum(struct rte_mbuf *mbuf)
+{
+	struct rte_net_hdr_lens hdr_lens;
+	uint32_t ptype, hdr_len;
+	uint16_t csum = 0, csum_offset;
+
+	/* Return early if the L4 checksum was not offloaded */
+	if ((mbuf->ol_flags & RTE_MBUF_F_RX_L4_CKSUM_MASK) != RTE_MBUF_F_RX_L4_CKSUM_NONE)
+		return;
+
+	ptype = rte_net_get_ptype(mbuf, &hdr_lens, RTE_PTYPE_ALL_MASK);
+
+	hdr_len = hdr_lens.l2_len + hdr_lens.l3_len;
+
+	switch (ptype & RTE_PTYPE_L4_MASK) {
+	case RTE_PTYPE_L4_TCP:
+		csum_offset = offsetof(struct rte_tcp_hdr, cksum) + hdr_len;
+		break;
+	case RTE_PTYPE_L4_UDP:
+		csum_offset = offsetof(struct rte_udp_hdr, dgram_cksum) + hdr_len;
+		break;
+	default:
+		/* Unsupported packet type */
+		return;
+	}
+
+	/* The pseudo-header checksum is already performed, as per Virtio spec */
+	if (rte_raw_cksum_mbuf(mbuf, hdr_len, rte_pktmbuf_pkt_len(mbuf) - hdr_len, &csum) < 0)
+		return;
+
+	csum = ~csum;
+	/* See RFC768 */
+	if (unlikely((ptype & RTE_PTYPE_L4_UDP) && csum == 0))
+		csum = 0xffff;
+
+	if (rte_pktmbuf_data_len(mbuf) >= csum_offset + 1)
+		*rte_pktmbuf_mtod_offset(mbuf, uint16_t *, csum_offset) = csum;
+
+	mbuf->ol_flags &= ~RTE_MBUF_F_RX_L4_CKSUM_MASK;
+	mbuf->ol_flags |= RTE_MBUF_F_RX_L4_CKSUM_GOOD;
+}
+
 static uint16_t
 eth_vhost_rx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
 {
@@ -402,6 +469,9 @@ eth_vhost_rx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
 		if (r->internal->vlan_strip)
 			rte_vlan_strip(bufs[i]);
 
+		if (r->internal->rx_sw_csum)
+			vhost_dev_rx_sw_csum(bufs[i]);
+
 		r->stats.bytes += bufs[i]->pkt_len;
 		r->stats.xstats[VHOST_BYTE] += bufs[i]->pkt_len;
 
@@ -805,6 +875,11 @@ new_device(int vid)
 		eth_dev->data->numa_node = newnode;
 #endif
 
+	if (rte_vhost_get_negotiated_features(vid, &internal->features)) {
+		VHOST_LOG(ERR, "Failed to get device features\n");
+		return -1;
+	}
+
 	internal->vid = vid;
 	if (rte_atomic32_read(&internal->started) == 1) {
 		queue_setup(eth_dev, internal);
@@ -827,6 +902,8 @@ new_device(int vid)
 
 	eth_dev->data->dev_link.link_status = RTE_ETH_LINK_UP;
 
+	vhost_dev_csum_configure(eth_dev);
+
 	rte_atomic32_set(&internal->dev_attached, 1);
 	update_queuing_status(eth_dev);
 
@@ -1131,6 +1208,8 @@ eth_dev_configure(struct rte_eth_dev *dev)
 
 	internal->vlan_strip = !!(rxmode->offloads & RTE_ETH_RX_OFFLOAD_VLAN_STRIP);
 
+	vhost_dev_csum_configure(dev);
+
 	return 0;
 }
 
@@ -1281,6 +1360,10 @@ eth_dev_info(struct rte_eth_dev *dev,
 	dev_info->tx_offload_capa = RTE_ETH_TX_OFFLOAD_MULTI_SEGS |
 				RTE_ETH_TX_OFFLOAD_VLAN_INSERT;
 	dev_info->rx_offload_capa = RTE_ETH_RX_OFFLOAD_VLAN_STRIP;
+	if (internal->flags & RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS) {
+		dev_info->rx_offload_capa |= RTE_ETH_RX_OFFLOAD_UDP_CKSUM |
+			RTE_ETH_RX_OFFLOAD_TCP_CKSUM;
+	}
 
 	return 0;
 }
-- 
2.35.1

[PATCH 6/6] net/vhost: perform SW checksum in Tx path

[Maxime Coquelin]

Virtio specification supports guest checksum offloading
for L4, which is enabled with VIRTIO_NET_F_GUEST_CSUM
feature negotiation. However, the Vhost PMD does not
advertise Tx checksum offload capabilities.

Advertising these offload capabilities at the ethdev level
is not enough, because we could still end-up with the
application enabling these offloads while the guest not
negotiating it.

This patch advertizes the Tx checksum offload capabilities,
and introduces a compatibility layer to cover the case
VIRTIO_NET_F_GUEST_CSUM has not been negotiated but the
application does configure the Tx checksum offloads. This
function performs the L4 Tx checksum in SW for UDP and TCP.
Compared to Rx SW checksum, the Tx SW checksum function
needs to compute the pseudo-header checksum, as we cannot
knwo whether it was done before.

This patch does not advertize SCTP checksum offloading
capability for now, but it could be handled later if the
need arises.

Reported-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 drivers/net/vhost/rte_eth_vhost.c | 62 +++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
index d5303f7368..52a802de05 100644
--- a/drivers/net/vhost/rte_eth_vhost.c
+++ b/drivers/net/vhost/rte_eth_vhost.c
@@ -114,6 +114,7 @@ struct pmd_internal {
 	rte_atomic32_t started;
 	bool vlan_strip;
 	bool rx_sw_csum;
+	bool tx_sw_csum;
 };
 
 struct internal_list {
@@ -370,8 +371,10 @@ vhost_dev_csum_configure(struct rte_eth_dev *eth_dev)
 {
 	struct pmd_internal *internal = eth_dev->data->dev_private;
 	const struct rte_eth_rxmode *rxmode = &eth_dev->data->dev_conf.rxmode;
+	const struct rte_eth_txmode *txmode = &eth_dev->data->dev_conf.txmode;
 
 	internal->rx_sw_csum = false;
+	internal->tx_sw_csum = false;
 
 	/* SW checksum is not compatible with legacy mode */
 	if (!(internal->flags & RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS))
@@ -384,6 +387,56 @@ vhost_dev_csum_configure(struct rte_eth_dev *eth_dev)
 			internal->rx_sw_csum = true;
 		}
 	}
+
+	if (!(internal->features & (1ULL << VIRTIO_NET_F_GUEST_CSUM))) {
+		if (txmode->offloads &
+				(RTE_ETH_TX_OFFLOAD_UDP_CKSUM | RTE_ETH_TX_OFFLOAD_TCP_CKSUM)) {
+			VHOST_LOG(NOTICE, "Tx csum will be done in SW, may impact performance.");
+			internal->tx_sw_csum = true;
+		}
+	}
+}
+
+static void
+vhost_dev_tx_sw_csum(struct rte_mbuf *mbuf)
+{
+	uint32_t hdr_len;
+	uint16_t csum = 0, csum_offset;
+
+	switch (mbuf->ol_flags & RTE_MBUF_F_TX_L4_MASK) {
+	case RTE_MBUF_F_TX_L4_NO_CKSUM:
+		return;
+	case RTE_MBUF_F_TX_TCP_CKSUM:
+		csum_offset = offsetof(struct rte_tcp_hdr, cksum);
+		break;
+	case RTE_MBUF_F_TX_UDP_CKSUM:
+		csum_offset = offsetof(struct rte_udp_hdr, dgram_cksum);
+		break;
+	default:
+		/* Unsupported packet type. */
+		return;
+	}
+
+	hdr_len = mbuf->l2_len + mbuf->l3_len;
+	csum_offset += hdr_len;
+
+	/* Prepare the pseudo-header checksum */
+	if (rte_net_intel_cksum_prepare(mbuf) < 0)
+		return;
+
+	if (rte_raw_cksum_mbuf(mbuf, hdr_len, rte_pktmbuf_pkt_len(mbuf) - hdr_len, &csum) < 0)
+		return;
+
+	csum = ~csum;
+	/* See RFC768 */
+	if (unlikely((mbuf->packet_type & RTE_PTYPE_L4_UDP) && csum == 0))
+		csum = 0xffff;
+
+	if (rte_pktmbuf_data_len(mbuf) >= csum_offset + 1)
+		*rte_pktmbuf_mtod_offset(mbuf, uint16_t *, csum_offset) = csum;
+
+	mbuf->ol_flags &= ~RTE_MBUF_F_TX_L4_MASK;
+	mbuf->ol_flags |= RTE_MBUF_F_TX_L4_NO_CKSUM;
 }
 
 static void
@@ -513,6 +566,10 @@ eth_vhost_tx(void *q, struct rte_mbuf **bufs, uint16_t nb_bufs)
 			}
 		}
 
+		if (r->internal->tx_sw_csum)
+			vhost_dev_tx_sw_csum(m);
+
+
 		bufs[nb_send] = m;
 		++nb_send;
 	}
@@ -1359,6 +1416,11 @@ eth_dev_info(struct rte_eth_dev *dev,
 
 	dev_info->tx_offload_capa = RTE_ETH_TX_OFFLOAD_MULTI_SEGS |
 				RTE_ETH_TX_OFFLOAD_VLAN_INSERT;
+	if (internal->flags & RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS) {
+		dev_info->tx_offload_capa |= RTE_ETH_TX_OFFLOAD_UDP_CKSUM |
+			RTE_ETH_TX_OFFLOAD_TCP_CKSUM;
+	}
+
 	dev_info->rx_offload_capa = RTE_ETH_RX_OFFLOAD_VLAN_STRIP;
 	if (internal->flags & RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS) {
 		dev_info->rx_offload_capa |= RTE_ETH_RX_OFFLOAD_UDP_CKSUM |
-- 
2.35.1

RE: [PATCH 6/6] net/vhost: perform SW checksum in Tx path

[Ma, WenwuX]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: 2022年5月5日 18:27
> To: dev@dpdk.org; jasowang@redhat.com; Xia, Chenbo
> <chenbo.xia@intel.com>; david.marchand@redhat.com;
> olivier.matz@6wind.com
> Cc: stable@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [PATCH 6/6] net/vhost: perform SW checksum in Tx path
> 
> Virtio specification supports guest checksum offloading for L4, which is
> enabled with VIRTIO_NET_F_GUEST_CSUM feature negotiation. However,
> the Vhost PMD does not advertise Tx checksum offload capabilities.
> 
> Advertising these offload capabilities at the ethdev level is not enough,
> because we could still end-up with the application enabling these offloads
> while the guest not negotiating it.
> 
> This patch advertizes the Tx checksum offload capabilities, and introduces a
> compatibility layer to cover the case VIRTIO_NET_F_GUEST_CSUM has not
> been negotiated but the application does configure the Tx checksum
> offloads. This function performs the L4 Tx checksum in SW for UDP and TCP.
> Compared to Rx SW checksum, the Tx SW checksum function needs to
> compute the pseudo-header checksum, as we cannot knwo whether it was
> done before.
> 
> This patch does not advertize SCTP checksum offloading capability for now,
> but it could be handled later if the need arises.

In virtio_enqueue_offload(), if RTE_MBUF_F_TX_IP_CKSUM is set, we will
performs the L3 Tx checksum, why do not we advertise IPV4 checksum offloading capability?
Will we advertise it later?

> 
> Reported-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  drivers/net/vhost/rte_eth_vhost.c | 62
> +++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c
> b/drivers/net/vhost/rte_eth_vhost.c
> index d5303f7368..52a802de05 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -114,6 +114,7 @@ struct pmd_internal {
>  	rte_atomic32_t started;
>  	bool vlan_strip;
>  	bool rx_sw_csum;
> +	bool tx_sw_csum;
>  };
> 
>  struct internal_list {
> @@ -370,8 +371,10 @@ vhost_dev_csum_configure(struct rte_eth_dev
> *eth_dev)  {
>  	struct pmd_internal *internal = eth_dev->data->dev_private;
>  	const struct rte_eth_rxmode *rxmode = &eth_dev->data-
> >dev_conf.rxmode;
> +	const struct rte_eth_txmode *txmode = &eth_dev->data-
> >dev_conf.txmode;
> 
>  	internal->rx_sw_csum = false;
> +	internal->tx_sw_csum = false;
> 
>  	/* SW checksum is not compatible with legacy mode */
>  	if (!(internal->flags &
> RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS))
> @@ -384,6 +387,56 @@ vhost_dev_csum_configure(struct rte_eth_dev
> *eth_dev)
>  			internal->rx_sw_csum = true;
>  		}
>  	}
> +
> +	if (!(internal->features & (1ULL << VIRTIO_NET_F_GUEST_CSUM))) {
> +		if (txmode->offloads &
> +				(RTE_ETH_TX_OFFLOAD_UDP_CKSUM |
> RTE_ETH_TX_OFFLOAD_TCP_CKSUM)) {
> +			VHOST_LOG(NOTICE, "Tx csum will be done in SW,
> may impact performance.");
> +			internal->tx_sw_csum = true;
> +		}
> +	}
> +}
> +
> +static void
> +vhost_dev_tx_sw_csum(struct rte_mbuf *mbuf) {
> +	uint32_t hdr_len;
> +	uint16_t csum = 0, csum_offset;
> +
> +	switch (mbuf->ol_flags & RTE_MBUF_F_TX_L4_MASK) {
> +	case RTE_MBUF_F_TX_L4_NO_CKSUM:
> +		return;
> +	case RTE_MBUF_F_TX_TCP_CKSUM:
> +		csum_offset = offsetof(struct rte_tcp_hdr, cksum);
> +		break;
> +	case RTE_MBUF_F_TX_UDP_CKSUM:
> +		csum_offset = offsetof(struct rte_udp_hdr, dgram_cksum);
> +		break;
> +	default:
> +		/* Unsupported packet type. */
> +		return;
> +	}
> +
> +	hdr_len = mbuf->l2_len + mbuf->l3_len;
> +	csum_offset += hdr_len;
> +
> +	/* Prepare the pseudo-header checksum */
> +	if (rte_net_intel_cksum_prepare(mbuf) < 0)
> +		return;
> +
> +	if (rte_raw_cksum_mbuf(mbuf, hdr_len,
> rte_pktmbuf_pkt_len(mbuf) - hdr_len, &csum) < 0)
> +		return;
> +
> +	csum = ~csum;
> +	/* See RFC768 */
> +	if (unlikely((mbuf->packet_type & RTE_PTYPE_L4_UDP) && csum ==
> 0))
> +		csum = 0xffff;
> +
> +	if (rte_pktmbuf_data_len(mbuf) >= csum_offset + 1)
> +		*rte_pktmbuf_mtod_offset(mbuf, uint16_t *, csum_offset)
> = csum;
> +
> +	mbuf->ol_flags &= ~RTE_MBUF_F_TX_L4_MASK;
> +	mbuf->ol_flags |= RTE_MBUF_F_TX_L4_NO_CKSUM;
>  }
> 
>  static void
> @@ -513,6 +566,10 @@ eth_vhost_tx(void *q, struct rte_mbuf **bufs,
> uint16_t nb_bufs)
>  			}
>  		}
> 
> +		if (r->internal->tx_sw_csum)
> +			vhost_dev_tx_sw_csum(m);
> +
> +
>  		bufs[nb_send] = m;
>  		++nb_send;
>  	}
> @@ -1359,6 +1416,11 @@ eth_dev_info(struct rte_eth_dev *dev,
> 
>  	dev_info->tx_offload_capa = RTE_ETH_TX_OFFLOAD_MULTI_SEGS |
>  				RTE_ETH_TX_OFFLOAD_VLAN_INSERT;
> +	if (internal->flags & RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS)
> {
> +		dev_info->tx_offload_capa |=
> RTE_ETH_TX_OFFLOAD_UDP_CKSUM |
> +			RTE_ETH_TX_OFFLOAD_TCP_CKSUM;
> +	}
> +
>  	dev_info->rx_offload_capa = RTE_ETH_RX_OFFLOAD_VLAN_STRIP;
>  	if (internal->flags & RTE_VHOST_USER_NET_COMPLIANT_OL_FLAGS)
> {
>  		dev_info->rx_offload_capa |=
> RTE_ETH_RX_OFFLOAD_UDP_CKSUM |
> --
> 2.35.1

Re: [PATCH 6/6] net/vhost: perform SW checksum in Tx path

[Maxime Coquelin]

Hi Wenwu,

Sorry, I missed your review.

On 5/7/22 05:20, Ma, WenwuX wrote:
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: 2022年5月5日 18:27
>> To: dev@dpdk.org; jasowang@redhat.com; Xia, Chenbo
>> <chenbo.xia@intel.com>; david.marchand@redhat.com;
>> olivier.matz@6wind.com
>> Cc: stable@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [PATCH 6/6] net/vhost: perform SW checksum in Tx path
>>
>> Virtio specification supports guest checksum offloading for L4, which is
>> enabled with VIRTIO_NET_F_GUEST_CSUM feature negotiation. However,
>> the Vhost PMD does not advertise Tx checksum offload capabilities.
>>
>> Advertising these offload capabilities at the ethdev level is not enough,
>> because we could still end-up with the application enabling these offloads
>> while the guest not negotiating it.
>>
>> This patch advertizes the Tx checksum offload capabilities, and introduces a
>> compatibility layer to cover the case VIRTIO_NET_F_GUEST_CSUM has not
>> been negotiated but the application does configure the Tx checksum
>> offloads. This function performs the L4 Tx checksum in SW for UDP and TCP.
>> Compared to Rx SW checksum, the Tx SW checksum function needs to
>> compute the pseudo-header checksum, as we cannot knwo whether it was
>> done before.
>>
>> This patch does not advertize SCTP checksum offloading capability for now,
>> but it could be handled later if the need arises.
> 
> In virtio_enqueue_offload(), if RTE_MBUF_F_TX_IP_CKSUM is set, we will
> performs the L3 Tx checksum, why do not we advertise IPV4 checksum offloading capability?
> Will we advertise it later?
> 

Indeed, we have an IPv4 SW checksum fallback in Vhost library.
We could think about adding the capability, but that's not urgent I
think. Do you have a use-case where it is needed?

Regards,
Maxime

RE: [PATCH 6/6] net/vhost: perform SW checksum in Tx path

[Ma, WenwuX]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: 2022年6月2日 17:07
> To: Ma, WenwuX <wenwux.ma@intel.com>; dev@dpdk.org;
> jasowang@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> david.marchand@redhat.com; Matz, Olivier <olivier.matz@6wind.com>
> Cc: stable@dpdk.org
> Subject: Re: [PATCH 6/6] net/vhost: perform SW checksum in Tx path
> 
> Hi Wenwu,
> 
> Sorry, I missed your review.
> 
> On 5/7/22 05:20, Ma, WenwuX wrote:
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: 2022年5月5日 18:27
> >> To: dev@dpdk.org; jasowang@redhat.com; Xia, Chenbo
> >> <chenbo.xia@intel.com>; david.marchand@redhat.com;
> >> olivier.matz@6wind.com
> >> Cc: stable@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Subject: [PATCH 6/6] net/vhost: perform SW checksum in Tx path
> >>
> >> Virtio specification supports guest checksum offloading for L4, which
> >> is enabled with VIRTIO_NET_F_GUEST_CSUM feature negotiation.
> However,
> >> the Vhost PMD does not advertise Tx checksum offload capabilities.
> >>
> >> Advertising these offload capabilities at the ethdev level is not
> >> enough, because we could still end-up with the application enabling
> >> these offloads while the guest not negotiating it.
> >>
> >> This patch advertizes the Tx checksum offload capabilities, and
> >> introduces a compatibility layer to cover the case
> >> VIRTIO_NET_F_GUEST_CSUM has not been negotiated but the
> application
> >> does configure the Tx checksum offloads. This function performs the L4 Tx
> checksum in SW for UDP and TCP.
> >> Compared to Rx SW checksum, the Tx SW checksum function needs to
> >> compute the pseudo-header checksum, as we cannot knwo whether it
> was
> >> done before.
> >>
> >> This patch does not advertize SCTP checksum offloading capability for
> >> now, but it could be handled later if the need arises.
> >
> > In virtio_enqueue_offload(), if RTE_MBUF_F_TX_IP_CKSUM is set, we will
> > performs the L3 Tx checksum, why do not we advertise IPV4 checksum
> offloading capability?
> > Will we advertise it later?
> >
> 
> Indeed, we have an IPv4 SW checksum fallback in Vhost library.
> We could think about adding the capability, but that's not urgent I think. Do
> you have a use-case where it is needed?
> 
The GRO/GSO library doesn't re-calculate IPv4 checksums for merged/fragmented packets, it will cause iperf in the vm to fail.

> Regards,
> Maxime

Re: [PATCH 6/6] net/vhost: perform SW checksum in Tx path

[Maxime Coquelin]

Hi Wenwu,

On 6/6/22 11:44, Ma, WenwuX wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: 2022年6月2日 17:07
>> To: Ma, WenwuX <wenwux.ma@intel.com>; dev@dpdk.org;
>> jasowang@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
>> david.marchand@redhat.com; Matz, Olivier <olivier.matz@6wind.com>
>> Cc: stable@dpdk.org
>> Subject: Re: [PATCH 6/6] net/vhost: perform SW checksum in Tx path
>>
>> Hi Wenwu,
>>
>> Sorry, I missed your review.
>>
>> On 5/7/22 05:20, Ma, WenwuX wrote:
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Sent: 2022年5月5日 18:27
>>>> To: dev@dpdk.org; jasowang@redhat.com; Xia, Chenbo
>>>> <chenbo.xia@intel.com>; david.marchand@redhat.com;
>>>> olivier.matz@6wind.com
>>>> Cc: stable@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Subject: [PATCH 6/6] net/vhost: perform SW checksum in Tx path
>>>>
>>>> Virtio specification supports guest checksum offloading for L4, which
>>>> is enabled with VIRTIO_NET_F_GUEST_CSUM feature negotiation.
>> However,
>>>> the Vhost PMD does not advertise Tx checksum offload capabilities.
>>>>
>>>> Advertising these offload capabilities at the ethdev level is not
>>>> enough, because we could still end-up with the application enabling
>>>> these offloads while the guest not negotiating it.
>>>>
>>>> This patch advertizes the Tx checksum offload capabilities, and
>>>> introduces a compatibility layer to cover the case
>>>> VIRTIO_NET_F_GUEST_CSUM has not been negotiated but the
>> application
>>>> does configure the Tx checksum offloads. This function performs the L4 Tx
>> checksum in SW for UDP and TCP.
>>>> Compared to Rx SW checksum, the Tx SW checksum function needs to
>>>> compute the pseudo-header checksum, as we cannot knwo whether it
>> was
>>>> done before.
>>>>
>>>> This patch does not advertize SCTP checksum offloading capability for
>>>> now, but it could be handled later if the need arises.
>>>
>>> In virtio_enqueue_offload(), if RTE_MBUF_F_TX_IP_CKSUM is set, we will
>>> performs the L3 Tx checksum, why do not we advertise IPV4 checksum
>> offloading capability?
>>> Will we advertise it later?
>>>
>>
>> Indeed, we have an IPv4 SW checksum fallback in Vhost library.
>> We could think about adding the capability, but that's not urgent I think. Do
>> you have a use-case where it is needed?
>>
> The GRO/GSO library doesn't re-calculate IPv4 checksums for merged/fragmented packets, it will cause iperf in the vm to fail.

Can you please elaborate?

If we don't expose the IPv4 checksum availability, it would be done by
the application using the Vhost PMD, so the result will be the same from
the VM point of view. Am I missing something?

Thanks,
Maxime

>> Regards,
>> Maxime
> 

RE: [PATCH 6/6] net/vhost: perform SW checksum in Tx path

[Ma, WenwuX]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: 2022年6月8日 16:14
> To: Ma, WenwuX <wenwux.ma@intel.com>; dev@dpdk.org;
> jasowang@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> david.marchand@redhat.com; Matz, Olivier <olivier.matz@6wind.com>
> Cc: stable@dpdk.org
> Subject: Re: [PATCH 6/6] net/vhost: perform SW checksum in Tx path
> 
> Hi Wenwu,
> 
> On 6/6/22 11:44, Ma, WenwuX wrote:
> >
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: 2022年6月2日 17:07
> >> To: Ma, WenwuX <wenwux.ma@intel.com>; dev@dpdk.org;
> >> jasowang@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>;
> >> david.marchand@redhat.com; Matz, Olivier <olivier.matz@6wind.com>
> >> Cc: stable@dpdk.org
> >> Subject: Re: [PATCH 6/6] net/vhost: perform SW checksum in Tx path
> >>
> >> Hi Wenwu,
> >>
> >> Sorry, I missed your review.
> >>
> >> On 5/7/22 05:20, Ma, WenwuX wrote:
> >>>
> >>>> -----Original Message-----
> >>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>> Sent: 2022年5月5日 18:27
> >>>> To: dev@dpdk.org; jasowang@redhat.com; Xia, Chenbo
> >>>> <chenbo.xia@intel.com>; david.marchand@redhat.com;
> >>>> olivier.matz@6wind.com
> >>>> Cc: stable@dpdk.org; Maxime Coquelin
> <maxime.coquelin@redhat.com>
> >>>> Subject: [PATCH 6/6] net/vhost: perform SW checksum in Tx path
> >>>>
> >>>> Virtio specification supports guest checksum offloading for L4,
> >>>> which is enabled with VIRTIO_NET_F_GUEST_CSUM feature
> negotiation.
> >> However,
> >>>> the Vhost PMD does not advertise Tx checksum offload capabilities.
> >>>>
> >>>> Advertising these offload capabilities at the ethdev level is not
> >>>> enough, because we could still end-up with the application enabling
> >>>> these offloads while the guest not negotiating it.
> >>>>
> >>>> This patch advertizes the Tx checksum offload capabilities, and
> >>>> introduces a compatibility layer to cover the case
> >>>> VIRTIO_NET_F_GUEST_CSUM has not been negotiated but the
> >> application
> >>>> does configure the Tx checksum offloads. This function performs the
> >>>> L4 Tx
> >> checksum in SW for UDP and TCP.
> >>>> Compared to Rx SW checksum, the Tx SW checksum function needs to
> >>>> compute the pseudo-header checksum, as we cannot knwo whether it
> >> was
> >>>> done before.
> >>>>
> >>>> This patch does not advertize SCTP checksum offloading capability
> >>>> for now, but it could be handled later if the need arises.
> >>>
> >>> In virtio_enqueue_offload(), if RTE_MBUF_F_TX_IP_CKSUM is set, we
> >>> will performs the L3 Tx checksum, why do not we advertise IPV4
> >>> checksum
> >> offloading capability?
> >>> Will we advertise it later?
> >>>
> >>
> >> Indeed, we have an IPv4 SW checksum fallback in Vhost library.
> >> We could think about adding the capability, but that's not urgent I
> >> think. Do you have a use-case where it is needed?
> >>
> > The GRO/GSO library doesn't re-calculate IPv4 checksums for
> merged/fragmented packets, it will cause iperf in the vm to fail.
> 
> Can you please elaborate?
> 
> If we don't expose the IPv4 checksum availability, it would be done by the
> application using the Vhost PMD, so the result will be the same from the VM
> point of view. Am I missing something?
> 
Ok, it is not urgent.

> Thanks,
> Maxime
> 
> >> Regards,
> >> Maxime
> >