[PATCH v1 1/3] examples/l3fwd: relax RSS requirement with option

[PATCH v1 1/3] examples/l3fwd: relax RSS requirement with option

[Trevor Tao]

Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS
by default, but some hw and/or virtual interface does not
support the RSS and offload mode presupposed, e.g., some
virtio interfaces in the cloud don't support
RSS and the error msg may like:

virtio_dev_configure(): RSS support requested but not supported by
the device
Port0 dev_configure = -95

So to enable the l3fwd running in that environment, the Rx mode requirement
can be relaxed to reflect the hardware feature reality here, and the l3fwd
can run smoothly then.

An option named "relax-rx-mode" is added to enable the relax action
here, and it's disabled by default.

Signed-off-by: Trevor Tao <taozj888@163.com>
---
 examples/l3fwd/main.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
index 6063eb1399..2c8f528d00 100644
--- a/examples/l3fwd/main.c
+++ b/examples/l3fwd/main.c
@@ -73,6 +73,7 @@ static enum L3FWD_LOOKUP_MODE lookup_mode;
 static int numa_on = 1; /**< NUMA is enabled by default. */
 static int parse_ptype; /**< Parse packet type using rx callback, and */
 			/**< disabled by default */
+static int relax_rx_mode; /**< Relax RX mode is disabled by default */
 static int per_port_pool; /**< Use separate buffer pools per port; disabled */
 			  /**< by default */
 
@@ -678,6 +679,7 @@ static const char short_options[] =
 #define CMD_LINE_OPT_MAX_PKT_LEN "max-pkt-len"
 #define CMD_LINE_OPT_HASH_ENTRY_NUM "hash-entry-num"
 #define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
+#define CMD_LINE_OPT_RELAX_RX_MODE "relax-rx-mode"
 #define CMD_LINE_OPT_PER_PORT_POOL "per-port-pool"
 #define CMD_LINE_OPT_MODE "mode"
 #define CMD_LINE_OPT_EVENTQ_SYNC "eventq-sched"
@@ -705,6 +707,7 @@ enum {
 	CMD_LINE_OPT_MAX_PKT_LEN_NUM,
 	CMD_LINE_OPT_HASH_ENTRY_NUM_NUM,
 	CMD_LINE_OPT_PARSE_PTYPE_NUM,
+	CMD_LINE_OPT_RELAX_RX_MODE_NUM,
 	CMD_LINE_OPT_RULE_IPV4_NUM,
 	CMD_LINE_OPT_RULE_IPV6_NUM,
 	CMD_LINE_OPT_ALG_NUM,
@@ -728,6 +731,7 @@ static const struct option lgopts[] = {
 	{CMD_LINE_OPT_MAX_PKT_LEN, 1, 0, CMD_LINE_OPT_MAX_PKT_LEN_NUM},
 	{CMD_LINE_OPT_HASH_ENTRY_NUM, 1, 0, CMD_LINE_OPT_HASH_ENTRY_NUM_NUM},
 	{CMD_LINE_OPT_PARSE_PTYPE, 0, 0, CMD_LINE_OPT_PARSE_PTYPE_NUM},
+	{CMD_LINE_OPT_RELAX_RX_MODE, 0, 0, CMD_LINE_OPT_RELAX_RX_MODE_NUM},
 	{CMD_LINE_OPT_PER_PORT_POOL, 0, 0, CMD_LINE_OPT_PARSE_PER_PORT_POOL},
 	{CMD_LINE_OPT_MODE, 1, 0, CMD_LINE_OPT_MODE_NUM},
 	{CMD_LINE_OPT_EVENTQ_SYNC, 1, 0, CMD_LINE_OPT_EVENTQ_SYNC_NUM},
@@ -853,6 +857,11 @@ parse_args(int argc, char **argv)
 			parse_ptype = 1;
 			break;
 
+		case CMD_LINE_OPT_RELAX_RX_MODE_NUM:
+			printf("Relax rx mode is enabled\n");
+			relax_rx_mode = 1;
+			break;
+
 		case CMD_LINE_OPT_PARSE_PER_PORT_POOL:
 			printf("per port buffer pool is enabled\n");
 			per_port_pool = 1;
@@ -1257,8 +1266,14 @@ l3fwd_poll_resource_setup(void)
 		local_port_conf.rx_adv_conf.rss_conf.rss_hf &=
 			dev_info.flow_type_rss_offloads;
 
-		if (dev_info.max_rx_queues == 1)
-			local_port_conf.rxmode.mq_mode = RTE_ETH_MQ_RX_NONE;
+		/* relax the rx rss requirement */
+		if (dev_info.max_rx_queues == 1 || !local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
+			if (relax_rx_mode) {
+				printf("warning: modified the rx mq_mode to RTE_ETH_MQ_RX_NONE base on"
+					" device capability\n");
+				local_port_conf.rxmode.mq_mode = RTE_ETH_MQ_RX_NONE;
+			}
+		}
 
 		if (local_port_conf.rx_adv_conf.rss_conf.rss_hf !=
 				port_conf.rx_adv_conf.rss_conf.rss_hf) {
-- 
2.34.1

[PATCH v1 2/3] examples/l3fwd: relax the Offload requirement

[Trevor Tao]

Now the port Rx offload mode is set to RTE_ETH_RX_OFFLOAD_CHECKSUM
by default, but some hw and/or virtual interface does not support
the offload mode presupposed, e.g., some virtio interfaces in
the cloud may only partly support RTE_ETH_RX_OFFLOAD_UDP_CKSUM/
RTE_ETH_RX_OFFLOAD_TCP_CKSUM,
but not RTE_ETH_RX_OFFLOAD_IPV4_CKSUM, and the error msg here:

Ethdev port_id=0 requested Rx offloads 0xe does not match Rx offloads
capabilities 0x201d in rte_eth_dev_configure()

So to enable the l3fwd running in that environment, the Rx mode requirement
can be relaxed to reflect the hardware feature reality here, and the l3fwd
can run smoothly then.
A warning msg would be provided to user in case it happens here.

On the other side, enabling the software cksum check in case missing the
hw support.

The relax action for rx cksum offload is just enabled when relax_rx_mode is
true which is false by default.

Signed-off-by: Trevor Tao <taozj888@163.com>
---
 examples/l3fwd/l3fwd.h     | 12 ++++++++++--
 examples/l3fwd/l3fwd_em.h  |  2 +-
 examples/l3fwd/l3fwd_lpm.h |  2 +-
 examples/l3fwd/main.c      | 14 ++++++++++++++
 4 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
index b55855c932..fd98ad3373 100644
--- a/examples/l3fwd/l3fwd.h
+++ b/examples/l3fwd/l3fwd.h
@@ -159,7 +159,7 @@ send_single_packet(struct lcore_conf *qconf,
 
 #ifdef DO_RFC_1812_CHECKS
 static inline int
-is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len)
+is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len, uint64_t ol_flags)
 {
 	/* From http://www.rfc-editor.org/rfc/rfc1812.txt section 5.2.2 */
 	/*
@@ -170,7 +170,15 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len)
 		return -1;
 
 	/* 2. The IP checksum must be correct. */
-	/* this is checked in H/W */
+	/* if this is not checked in H/W, check it. */
+	if ((ol_flags & RTE_ETH_RX_OFFLOAD_IPV4_CKSUM) == 0) {
+		uint16_t actual_cksum, expected_cksum;
+		actual_cksum = pkt->hdr_checksum;
+		pkt->hdr_checksum = 0;
+		expected_cksum = rte_ipv4_cksum(pkt);
+		if (actual_cksum != expected_cksum)
+			return -2;
+	}
 
 	/*
 	 * 3. The IP version number must be 4. If the version number is not 4
diff --git a/examples/l3fwd/l3fwd_em.h b/examples/l3fwd/l3fwd_em.h
index 7d051fc076..1fee2e2e6c 100644
--- a/examples/l3fwd/l3fwd_em.h
+++ b/examples/l3fwd/l3fwd_em.h
@@ -20,7 +20,7 @@ l3fwd_em_handle_ipv4(struct rte_mbuf *m, uint16_t portid,
 
 #ifdef DO_RFC_1812_CHECKS
 	/* Check to make sure the packet is valid (RFC1812) */
-	if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len) < 0) {
+	if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len, m->ol_flags) < 0) {
 		rte_pktmbuf_free(m);
 		return BAD_PORT;
 	}
diff --git a/examples/l3fwd/l3fwd_lpm.h b/examples/l3fwd/l3fwd_lpm.h
index c61b969584..5ddae7da0f 100644
--- a/examples/l3fwd/l3fwd_lpm.h
+++ b/examples/l3fwd/l3fwd_lpm.h
@@ -22,7 +22,7 @@ l3fwd_lpm_simple_forward(struct rte_mbuf *m, uint16_t portid,
 
 #ifdef DO_RFC_1812_CHECKS
 		/* Check to make sure the packet is valid (RFC1812) */
-		if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len) < 0) {
+		if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len) < 0, m->ol_flags) {
 			rte_pktmbuf_free(m);
 			return;
 		}
diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
index 2c8f528d00..a48ae7f62b 100644
--- a/examples/l3fwd/main.c
+++ b/examples/l3fwd/main.c
@@ -1284,6 +1284,20 @@ l3fwd_poll_resource_setup(void)
 				local_port_conf.rx_adv_conf.rss_conf.rss_hf);
 		}
 
+		/* relax the rx offload requirement */
+		if ((local_port_conf.rxmode.offloads & dev_info.rx_offload_capa) !=
+			local_port_conf.rxmode.offloads) {
+			printf("Port %u requested Rx offloads 0x%"PRIx64" does not"
+				" match Rx offloads capabilities 0x%"PRIx64"\n",
+				portid, local_port_conf.rxmode.offloads,
+				dev_info.rx_offload_capa);
+			if (relax_rx_mode) {
+				local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
+				printf("warning: modified the rx offload to 0x%"PRIx64" based on device"
+				" capability\n", local_port_conf.rxmode.offloads);
+			}
+		}
+
 		ret = rte_eth_dev_configure(portid, nb_rx_queue,
 					(uint16_t)n_tx_queue, &local_port_conf);
 		if (ret < 0)
-- 
2.34.1

[PATCH v1 3/3] doc: add a relax rx mode requirement option

[Trevor Tao]

Add an option to enable the RX mode requirement relax
in release notes and l3fwd sample guide.

Signed-off-by: Trevor Tao <taozj888@163.com>
---
 doc/guides/rel_notes/release_23_11.rst  | 2 ++
 doc/guides/sample_app_ug/l3_forward.rst | 4 +++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/doc/guides/rel_notes/release_23_11.rst b/doc/guides/rel_notes/release_23_11.rst
index f09ecd50fe..2f9e4a54c8 100644
--- a/doc/guides/rel_notes/release_23_11.rst
+++ b/doc/guides/rel_notes/release_23_11.rst
@@ -84,6 +84,8 @@ New Features
   default. The implementation using C11 standard atomic operations is enabled
   via the ``enable_stdatomic`` build option.
 
+* sample: Added a command option ``--relax-rx-mode`` in l3fwd example
+  to relax the rx RSS/Offload mode requirement if needed.
 
 Removed Items
 -------------
diff --git a/doc/guides/sample_app_ug/l3_forward.rst b/doc/guides/sample_app_ug/l3_forward.rst
index 1cc2c1dd1d..00283f070c 100644
--- a/doc/guides/sample_app_ug/l3_forward.rst
+++ b/doc/guides/sample_app_ug/l3_forward.rst
@@ -126,6 +126,8 @@ Where,
 
 * ``--parse-ptype:`` Optional, set to use software to analyze packet type. Without this option, hardware will check the packet type.
 
+* ``--relax-rx-mode:`` Optional, set to enable rx mode relax when RSS/offload is not fully supported by the hardware. When the IPv4 cksum offload is relaxed, it is calculated by the software instead. Without this option, the RSS and cksum offload will be forced.
+
 * ``--per-port-pool:`` Optional, set to use independent buffer pools per port. Without this option, single buffer pool is used for all ports.
 
 * ``--mode:`` Optional, Packet transfer mode for I/O, poll or eventdev.
@@ -140,7 +142,7 @@ Where,
 
 * ``--event-vector-tmo:`` Optional, Max timeout to form vector in nanoseconds if event vectorization is enabled.
 
-* ``--alg=<val>:`` optional, ACL classify method to use, one of:
+* ``--alg=<val>:`` Optional, ACL classify method to use, one of:
   ``scalar|sse|avx2|neon|altivec|avx512x16|avx512x32``
 
 * ``-E:`` Optional, enable exact match,
-- 
2.34.1

Re: [PATCH v1 1/3] examples/l3fwd: relax RSS requirement with option

[Konstantin Ananyev]

Hi Trevor,

> Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS
> by default, but some hw and/or virtual interface does not
> support the RSS and offload mode presupposed, e.g., some
> virtio interfaces in the cloud don't support
> RSS and the error msg may like:
> 
> virtio_dev_configure(): RSS support requested but not supported by
> the device
> Port0 dev_configure = -95
> 
> So to enable the l3fwd running in that environment, the Rx mode requirement
> can be relaxed to reflect the hardware feature reality here, and the l3fwd
> can run smoothly then.
> 
> An option named "relax-rx-mode" is added to enable the relax action
> here, and it's disabled by default.
> 
> Signed-off-by: Trevor Tao <taozj888@163.com>
> ---
>   examples/l3fwd/main.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> index 6063eb1399..2c8f528d00 100644
> --- a/examples/l3fwd/main.c
> +++ b/examples/l3fwd/main.c
> @@ -73,6 +73,7 @@ static enum L3FWD_LOOKUP_MODE lookup_mode;
>   static int numa_on = 1; /**< NUMA is enabled by default. */
>   static int parse_ptype; /**< Parse packet type using rx callback, and */
>   			/**< disabled by default */
> +static int relax_rx_mode; /**< Relax RX mode is disabled by default */
>   static int per_port_pool; /**< Use separate buffer pools per port; disabled */
>   			  /**< by default */
>   
> @@ -678,6 +679,7 @@ static const char short_options[] =
>   #define CMD_LINE_OPT_MAX_PKT_LEN "max-pkt-len"
>   #define CMD_LINE_OPT_HASH_ENTRY_NUM "hash-entry-num"
>   #define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
> +#define CMD_LINE_OPT_RELAX_RX_MODE "relax-rx-mode"
>   #define CMD_LINE_OPT_PER_PORT_POOL "per-port-pool"
>   #define CMD_LINE_OPT_MODE "mode"
>   #define CMD_LINE_OPT_EVENTQ_SYNC "eventq-sched"
> @@ -705,6 +707,7 @@ enum {
>   	CMD_LINE_OPT_MAX_PKT_LEN_NUM,
>   	CMD_LINE_OPT_HASH_ENTRY_NUM_NUM,
>   	CMD_LINE_OPT_PARSE_PTYPE_NUM,
> +	CMD_LINE_OPT_RELAX_RX_MODE_NUM,
>   	CMD_LINE_OPT_RULE_IPV4_NUM,
>   	CMD_LINE_OPT_RULE_IPV6_NUM,
>   	CMD_LINE_OPT_ALG_NUM,
> @@ -728,6 +731,7 @@ static const struct option lgopts[] = {
>   	{CMD_LINE_OPT_MAX_PKT_LEN, 1, 0, CMD_LINE_OPT_MAX_PKT_LEN_NUM},
>   	{CMD_LINE_OPT_HASH_ENTRY_NUM, 1, 0, CMD_LINE_OPT_HASH_ENTRY_NUM_NUM},
>   	{CMD_LINE_OPT_PARSE_PTYPE, 0, 0, CMD_LINE_OPT_PARSE_PTYPE_NUM},
> +	{CMD_LINE_OPT_RELAX_RX_MODE, 0, 0, CMD_LINE_OPT_RELAX_RX_MODE_NUM},
>   	{CMD_LINE_OPT_PER_PORT_POOL, 0, 0, CMD_LINE_OPT_PARSE_PER_PORT_POOL},
>   	{CMD_LINE_OPT_MODE, 1, 0, CMD_LINE_OPT_MODE_NUM},
>   	{CMD_LINE_OPT_EVENTQ_SYNC, 1, 0, CMD_LINE_OPT_EVENTQ_SYNC_NUM},
> @@ -853,6 +857,11 @@ parse_args(int argc, char **argv)
>   			parse_ptype = 1;
>   			break;
>   
> +		case CMD_LINE_OPT_RELAX_RX_MODE_NUM:
> +			printf("Relax rx mode is enabled\n");
> +			relax_rx_mode = 1;
> +			break;
> +
>   		case CMD_LINE_OPT_PARSE_PER_PORT_POOL:
>   			printf("per port buffer pool is enabled\n");
>   			per_port_pool = 1;
> @@ -1257,8 +1266,14 @@ l3fwd_poll_resource_setup(void)
>   		local_port_conf.rx_adv_conf.rss_conf.rss_hf &=
>   			dev_info.flow_type_rss_offloads;
>   
> -		if (dev_info.max_rx_queues == 1)
> -			local_port_conf.rxmode.mq_mode = RTE_ETH_MQ_RX_NONE;
> +		/* relax the rx rss requirement */
> +		if (dev_info.max_rx_queues == 1 || !local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
> +			if (relax_rx_mode) {
> +				printf("warning: modified the rx mq_mode to RTE_ETH_MQ_RX_NONE base on"
> +					" device capability\n");
> +				local_port_conf.rxmode.mq_mode = RTE_ETH_MQ_RX_NONE;
> +			}
> +		}

But that way we change current behavior - always use MQ_RX_NONE
for devices with just one RX queue.
Was it intended?
Might be it should be:
	if (dev_info.max_rx_queues == 1)
		local_port_conf.rxmode.mq_mode = RTE_ETH_MQ_RX_NONE;

+	/* relax the rx rss requirement */
+	if (relax_rx_mode &&
+ 			!local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
+		printf("...");
+		local_port_conf.rxmode.mq_mode = RTE_ETH_MQ_RX_NONE;
+	}

?


>   
>   		if (local_port_conf.rx_adv_conf.rss_conf.rss_hf !=
>   				port_conf.rx_adv_conf.rss_conf.rss_hf) {

Re: [PATCH v1 2/3] examples/l3fwd: relax the Offload requirement

[Konstantin Ananyev]

02.10.2023 09:53, Trevor Tao пишет:
> Now the port Rx offload mode is set to RTE_ETH_RX_OFFLOAD_CHECKSUM
> by default, but some hw and/or virtual interface does not support
> the offload mode presupposed, e.g., some virtio interfaces in
> the cloud may only partly support RTE_ETH_RX_OFFLOAD_UDP_CKSUM/
> RTE_ETH_RX_OFFLOAD_TCP_CKSUM,
> but not RTE_ETH_RX_OFFLOAD_IPV4_CKSUM, and the error msg here:
> 
> Ethdev port_id=0 requested Rx offloads 0xe does not match Rx offloads
> capabilities 0x201d in rte_eth_dev_configure()
> 
> So to enable the l3fwd running in that environment, the Rx mode requirement
> can be relaxed to reflect the hardware feature reality here, and the l3fwd
> can run smoothly then.
> A warning msg would be provided to user in case it happens here.
> 
> On the other side, enabling the software cksum check in case missing the
> hw support.
> 
> The relax action for rx cksum offload is just enabled when relax_rx_mode is
> true which is false by default.
> 
> Signed-off-by: Trevor Tao <taozj888@163.com>
> ---
>   examples/l3fwd/l3fwd.h     | 12 ++++++++++--
>   examples/l3fwd/l3fwd_em.h  |  2 +-
>   examples/l3fwd/l3fwd_lpm.h |  2 +-
>   examples/l3fwd/main.c      | 14 ++++++++++++++
>   4 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
> index b55855c932..fd98ad3373 100644
> --- a/examples/l3fwd/l3fwd.h
> +++ b/examples/l3fwd/l3fwd.h
> @@ -159,7 +159,7 @@ send_single_packet(struct lcore_conf *qconf,
>   
>   #ifdef DO_RFC_1812_CHECKS
>   static inline int
> -is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len)
> +is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len, uint64_t ol_flags)
>   {
>   	/* From http://www.rfc-editor.org/rfc/rfc1812.txt section 5.2.2 */
>   	/*
> @@ -170,7 +170,15 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len)
>   		return -1;
>   
>   	/* 2. The IP checksum must be correct. */
> -	/* this is checked in H/W */
> +	/* if this is not checked in H/W, check it. */
> +	if ((ol_flags & RTE_ETH_RX_OFFLOAD_IPV4_CKSUM) == 0) {
> +		uint16_t actual_cksum, expected_cksum;
> +		actual_cksum = pkt->hdr_checksum;
> +		pkt->hdr_checksum = 0;
> +		expected_cksum = rte_ipv4_cksum(pkt);
> +		if (actual_cksum != expected_cksum)
> +			return -2;
> +	}
>   
>   	/*
>   	 * 3. The IP version number must be 4. If the version number is not 4
> diff --git a/examples/l3fwd/l3fwd_em.h b/examples/l3fwd/l3fwd_em.h
> index 7d051fc076..1fee2e2e6c 100644
> --- a/examples/l3fwd/l3fwd_em.h
> +++ b/examples/l3fwd/l3fwd_em.h
> @@ -20,7 +20,7 @@ l3fwd_em_handle_ipv4(struct rte_mbuf *m, uint16_t portid,
>   
>   #ifdef DO_RFC_1812_CHECKS
>   	/* Check to make sure the packet is valid (RFC1812) */
> -	if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len) < 0) {
> +	if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len, m->ol_flags) < 0) {
>   		rte_pktmbuf_free(m);
>   		return BAD_PORT;
>   	}
> diff --git a/examples/l3fwd/l3fwd_lpm.h b/examples/l3fwd/l3fwd_lpm.h
> index c61b969584..5ddae7da0f 100644
> --- a/examples/l3fwd/l3fwd_lpm.h
> +++ b/examples/l3fwd/l3fwd_lpm.h
> @@ -22,7 +22,7 @@ l3fwd_lpm_simple_forward(struct rte_mbuf *m, uint16_t portid,
>   
>   #ifdef DO_RFC_1812_CHECKS
>   		/* Check to make sure the packet is valid (RFC1812) */
> -		if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len) < 0) {
> +		if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len) < 0, m->ol_flags) {

Typo, pls fix.

>   			rte_pktmbuf_free(m);
>   			return;
>   		}
> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> index 2c8f528d00..a48ae7f62b 100644
> --- a/examples/l3fwd/main.c
> +++ b/examples/l3fwd/main.c
> @@ -1284,6 +1284,20 @@ l3fwd_poll_resource_setup(void)
>   				local_port_conf.rx_adv_conf.rss_conf.rss_hf);
>   		}
>   
> +		/* relax the rx offload requirement */
> +		if ((local_port_conf.rxmode.offloads & dev_info.rx_offload_capa) !=
> +			local_port_conf.rxmode.offloads) {


Ok, but we relax only IP cksum.
Though l3fwd tries to enable IP/TCP/UDP cksum.
What if TCP/UDP is not supported, should we allow it or fail?


> +			printf("Port %u requested Rx offloads 0x%"PRIx64" does not"
> +				" match Rx offloads capabilities 0x%"PRIx64"\n",
> +				portid, local_port_conf.rxmode.offloads,
> +				dev_info.rx_offload_capa);
> +			if (relax_rx_mode) {
> +				local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
> +				printf("warning: modified the rx offload to 0x%"PRIx64" based on device"
> +				" capability\n", local_port_conf.rxmode.offloads);
> +			}
> +		}
> +
>   		ret = rte_eth_dev_configure(portid, nb_rx_queue,
>   					(uint16_t)n_tx_queue, &local_port_conf);
>   		if (ret < 0)

if you going to submit new version, pls don't forget to bump the version 
number of the patch.

Re:Re: [PATCH v1 2/3] examples/l3fwd: relax the Offload requirement

[taozj888]

[-- Attachment #1: Type: text/plain, Size: 5315 bytes --]

Hi Konstantin:
For your comments, please see my answer inline.
Thanks.




Trevor Tao

At 2023-10-09 17:03:11, "Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru> wrote:
>02.10.2023 09:53, Trevor Tao пишет:
>> Now the port Rx offload mode is set to RTE_ETH_RX_OFFLOAD_CHECKSUM
>> by default, but some hw and/or virtual interface does not support
>> the offload mode presupposed, e.g., some virtio interfaces in
>> the cloud may only partly support RTE_ETH_RX_OFFLOAD_UDP_CKSUM/
>> RTE_ETH_RX_OFFLOAD_TCP_CKSUM,
>> but not RTE_ETH_RX_OFFLOAD_IPV4_CKSUM, and the error msg here:
>> 
>> Ethdev port_id=0 requested Rx offloads 0xe does not match Rx offloads
>> capabilities 0x201d in rte_eth_dev_configure()
>> 
>> So to enable the l3fwd running in that environment, the Rx mode requirement
>> can be relaxed to reflect the hardware feature reality here, and the l3fwd
>> can run smoothly then.
>> A warning msg would be provided to user in case it happens here.
>> 
>> On the other side, enabling the software cksum check in case missing the
>> hw support.
>> 
>> The relax action for rx cksum offload is just enabled when relax_rx_mode is
>> true which is false by default.
>> 
>> Signed-off-by: Trevor Tao <taozj888@163.com>
>> ---
>>   examples/l3fwd/l3fwd.h     | 12 ++++++++++--
>>   examples/l3fwd/l3fwd_em.h  |  2 +-
>>   examples/l3fwd/l3fwd_lpm.h |  2 +-
>>   examples/l3fwd/main.c      | 14 ++++++++++++++
>>   4 files changed, 26 insertions(+), 4 deletions(-)
>> 
>> diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
>> index b55855c932..fd98ad3373 100644
>> --- a/examples/l3fwd/l3fwd.h
>> +++ b/examples/l3fwd/l3fwd.h
>> @@ -159,7 +159,7 @@ send_single_packet(struct lcore_conf *qconf,
>>   
>>   #ifdef DO_RFC_1812_CHECKS
>>   static inline int
>> -is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len)
>> +is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len, uint64_t ol_flags)
>>   {
>>   	/* From http://www.rfc-editor.org/rfc/rfc1812.txt section 5.2.2 */
>>   	/*
>> @@ -170,7 +170,15 @@ is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len)
>>   		return -1;
>>   
>>   	/* 2. The IP checksum must be correct. */
>> -	/* this is checked in H/W */
>> +	/* if this is not checked in H/W, check it. */
>> +	if ((ol_flags & RTE_ETH_RX_OFFLOAD_IPV4_CKSUM) == 0) {
>> +		uint16_t actual_cksum, expected_cksum;
>> +		actual_cksum = pkt->hdr_checksum;
>> +		pkt->hdr_checksum = 0;
>> +		expected_cksum = rte_ipv4_cksum(pkt);
>> +		if (actual_cksum != expected_cksum)
>> +			return -2;
>> +	}
>>   
>>   	/*
>>   	 * 3. The IP version number must be 4. If the version number is not 4
>> diff --git a/examples/l3fwd/l3fwd_em.h b/examples/l3fwd/l3fwd_em.h
>> index 7d051fc076..1fee2e2e6c 100644
>> --- a/examples/l3fwd/l3fwd_em.h
>> +++ b/examples/l3fwd/l3fwd_em.h
>> @@ -20,7 +20,7 @@ l3fwd_em_handle_ipv4(struct rte_mbuf *m, uint16_t portid,
>>   
>>   #ifdef DO_RFC_1812_CHECKS
>>   	/* Check to make sure the packet is valid (RFC1812) */
>> -	if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len) < 0) {
>> +	if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len, m->ol_flags) < 0) {
>>   		rte_pktmbuf_free(m);
>>   		return BAD_PORT;
>>   	}
>> diff --git a/examples/l3fwd/l3fwd_lpm.h b/examples/l3fwd/l3fwd_lpm.h
>> index c61b969584..5ddae7da0f 100644
>> --- a/examples/l3fwd/l3fwd_lpm.h
>> +++ b/examples/l3fwd/l3fwd_lpm.h
>> @@ -22,7 +22,7 @@ l3fwd_lpm_simple_forward(struct rte_mbuf *m, uint16_t portid,
>>   
>>   #ifdef DO_RFC_1812_CHECKS
>>   		/* Check to make sure the packet is valid (RFC1812) */
>> -		if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len) < 0) {
>> +		if (is_valid_ipv4_pkt(ipv4_hdr, m->pkt_len) < 0, m->ol_flags) {
>

>Typo, pls fix.


>OK, I will fix it. Thanks.
>
>>   			rte_pktmbuf_free(m);
>>   			return;
>>   		}
>> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
>> index 2c8f528d00..a48ae7f62b 100644
>> --- a/examples/l3fwd/main.c
>> +++ b/examples/l3fwd/main.c
>> @@ -1284,6 +1284,20 @@ l3fwd_poll_resource_setup(void)
>>   				local_port_conf.rx_adv_conf.rss_conf.rss_hf);
>>   		}
>>   
>> +		/* relax the rx offload requirement */
>> +		if ((local_port_conf.rxmode.offloads & dev_info.rx_offload_capa) !=
>> +			local_port_conf.rxmode.offloads) {
>
>
>Ok, but we relax only IP cksum.
>Though l3fwd tries to enable IP/TCP/UDP cksum.

>What if TCP/UDP is not supported, should we allow it or fail?


>I think the l3fwd just check the packet at the IP layer, and the TCP/UDP should not bother us here, so I think it can be allowed to relax them if they are not supported. 
What do think about it?
Thanks.




>
>
>> +			printf("Port %u requested Rx offloads 0x%"PRIx64" does not"
>> +				" match Rx offloads capabilities 0x%"PRIx64"\n",
>> +				portid, local_port_conf.rxmode.offloads,
>> +				dev_info.rx_offload_capa);
>> +			if (relax_rx_mode) {
>> +				local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
>> +				printf("warning: modified the rx offload to 0x%"PRIx64" based on device"
>> +				" capability\n", local_port_conf.rxmode.offloads);
>> +			}
>> +		}
>> +
>>   		ret = rte_eth_dev_configure(portid, nb_rx_queue,
>>   					(uint16_t)n_tx_queue, &local_port_conf);
>>   		if (ret < 0)
>
>if you going to submit new version, pls don't forget to bump the version 

>number of the patch.


>OK, I will check it.
Thanks.

[-- Attachment #2: Type: text/html, Size: 6977 bytes --]

Re:Re: [PATCH v1 1/3] examples/l3fwd: relax RSS requirement with option

[taozj888]

[-- Attachment #1: Type: text/plain, Size: 4604 bytes --]

Hi Konstantin:


For your comments, please see my answers inline.
Thanks.


Trevor Tao

At 2023-10-09 16:43:01, "Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru> wrote:
>Hi Trevor,
>
>> Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS
>> by default, but some hw and/or virtual interface does not
>> support the RSS and offload mode presupposed, e.g., some
>> virtio interfaces in the cloud don't support
>> RSS and the error msg may like:
>> 
>> virtio_dev_configure(): RSS support requested but not supported by
>> the device
>> Port0 dev_configure = -95
>> 
>> So to enable the l3fwd running in that environment, the Rx mode requirement
>> can be relaxed to reflect the hardware feature reality here, and the l3fwd
>> can run smoothly then.
>> 
>> An option named "relax-rx-mode" is added to enable the relax action
>> here, and it's disabled by default.
>> 
>> Signed-off-by: Trevor Tao <taozj888@163.com>
>> ---
>>   examples/l3fwd/main.c | 19 +++++++++++++++++--
>>   1 file changed, 17 insertions(+), 2 deletions(-)
>> 
>> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
>> index 6063eb1399..2c8f528d00 100644
>> --- a/examples/l3fwd/main.c
>> +++ b/examples/l3fwd/main.c
>> @@ -73,6 +73,7 @@ static enum L3FWD_LOOKUP_MODE lookup_mode;
>>   static int numa_on = 1; /**< NUMA is enabled by default. */
>>   static int parse_ptype; /**< Parse packet type using rx callback, and */
>>   			/**< disabled by default */
>> +static int relax_rx_mode; /**< Relax RX mode is disabled by default */
>>   static int per_port_pool; /**< Use separate buffer pools per port; disabled */
>>   			  /**< by default */
>>   
>> @@ -678,6 +679,7 @@ static const char short_options[] =
>>   #define CMD_LINE_OPT_MAX_PKT_LEN "max-pkt-len"
>>   #define CMD_LINE_OPT_HASH_ENTRY_NUM "hash-entry-num"
>>   #define CMD_LINE_OPT_PARSE_PTYPE "parse-ptype"
>> +#define CMD_LINE_OPT_RELAX_RX_MODE "relax-rx-mode"
>>   #define CMD_LINE_OPT_PER_PORT_POOL "per-port-pool"
>>   #define CMD_LINE_OPT_MODE "mode"
>>   #define CMD_LINE_OPT_EVENTQ_SYNC "eventq-sched"
>> @@ -705,6 +707,7 @@ enum {
>>   	CMD_LINE_OPT_MAX_PKT_LEN_NUM,
>>   	CMD_LINE_OPT_HASH_ENTRY_NUM_NUM,
>>   	CMD_LINE_OPT_PARSE_PTYPE_NUM,
>> +	CMD_LINE_OPT_RELAX_RX_MODE_NUM,
>>   	CMD_LINE_OPT_RULE_IPV4_NUM,
>>   	CMD_LINE_OPT_RULE_IPV6_NUM,
>>   	CMD_LINE_OPT_ALG_NUM,
>> @@ -728,6 +731,7 @@ static const struct option lgopts[] = {
>>   	{CMD_LINE_OPT_MAX_PKT_LEN, 1, 0, CMD_LINE_OPT_MAX_PKT_LEN_NUM},
>>   	{CMD_LINE_OPT_HASH_ENTRY_NUM, 1, 0, CMD_LINE_OPT_HASH_ENTRY_NUM_NUM},
>>   	{CMD_LINE_OPT_PARSE_PTYPE, 0, 0, CMD_LINE_OPT_PARSE_PTYPE_NUM},
>> +	{CMD_LINE_OPT_RELAX_RX_MODE, 0, 0, CMD_LINE_OPT_RELAX_RX_MODE_NUM},
>>   	{CMD_LINE_OPT_PER_PORT_POOL, 0, 0, CMD_LINE_OPT_PARSE_PER_PORT_POOL},
>>   	{CMD_LINE_OPT_MODE, 1, 0, CMD_LINE_OPT_MODE_NUM},
>>   	{CMD_LINE_OPT_EVENTQ_SYNC, 1, 0, CMD_LINE_OPT_EVENTQ_SYNC_NUM},
>> @@ -853,6 +857,11 @@ parse_args(int argc, char **argv)
>>   			parse_ptype = 1;
>>   			break;
>>   
>> +		case CMD_LINE_OPT_RELAX_RX_MODE_NUM:
>> +			printf("Relax rx mode is enabled\n");
>> +			relax_rx_mode = 1;
>> +			break;
>> +
>>   		case CMD_LINE_OPT_PARSE_PER_PORT_POOL:
>>   			printf("per port buffer pool is enabled\n");
>>   			per_port_pool = 1;
>> @@ -1257,8 +1266,14 @@ l3fwd_poll_resource_setup(void)
>>   		local_port_conf.rx_adv_conf.rss_conf.rss_hf &=
>>   			dev_info.flow_type_rss_offloads;
>>   
>> -		if (dev_info.max_rx_queues == 1)
>> -			local_port_conf.rxmode.mq_mode = RTE_ETH_MQ_RX_NONE;
>> +		/* relax the rx rss requirement */
>> +		if (dev_info.max_rx_queues == 1 || !local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
>> +			if (relax_rx_mode) {
>> +				printf("warning: modified the rx mq_mode to RTE_ETH_MQ_RX_NONE base on"
>> +					" device capability\n");
>> +				local_port_conf.rxmode.mq_mode = RTE_ETH_MQ_RX_NONE;
>> +			}
>> +		}
>
>But that way we change current behavior - always use MQ_RX_NONE
>for devices with just one RX queue.
>Was it intended?
>Might be it should be:
>	if (dev_info.max_rx_queues == 1)
>		local_port_conf.rxmode.mq_mode = RTE_ETH_MQ_RX_NONE;
>
>+	/* relax the rx rss requirement */
>+	if (relax_rx_mode &&
>+ 			!local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
>+		printf("...");
>+		local_port_conf.rxmode.mq_mode = RTE_ETH_MQ_RX_NONE;
>+	}
>

>?
>OK, I will change it as what you gave here though I think there is no difference between the max_rx_queue is 1 and rss_hf is 0, both of them mean that RSS is not supported.
Thanks. 
>
>
>>   
>>   		if (local_port_conf.rx_adv_conf.rss_conf.rss_hf !=
>>   				port_conf.rx_adv_conf.rss_conf.rss_hf) {

[-- Attachment #2: Type: text/html, Size: 5731 bytes --]