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

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

[Trevor Tao]

Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS, and offload
mode set to RTE_ETH_RX_OFFLOAD_CHECKSUM by default, but some hardware
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 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:

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

and:
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 the
hw support missing.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Trevor Tao <taozj888@163.com>
---
 examples/l3fwd/l3fwd.h | 12 +++++++++++-
 examples/l3fwd/main.c  | 21 +++++++++++++++++++--
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
index b55855c932..cc10643c4b 100644
--- a/examples/l3fwd/l3fwd.h
+++ b/examples/l3fwd/l3fwd.h
@@ -115,6 +115,8 @@ extern struct acl_algorithms acl_alg[];
 
 extern uint32_t max_pkt_len;
 
+extern struct rte_eth_conf port_conf;
+
 /* Send burst of packets on an output interface */
 static inline int
 send_burst(struct lcore_conf *qconf, uint16_t n, uint16_t port)
@@ -170,7 +172,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 ((port_conf.rxmode.offloads & 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/main.c b/examples/l3fwd/main.c
index 6063eb1399..37aec64718 100644
--- a/examples/l3fwd/main.c
+++ b/examples/l3fwd/main.c
@@ -117,7 +117,7 @@ static struct lcore_params * lcore_params = lcore_params_array_default;
 static uint16_t nb_lcore_params = sizeof(lcore_params_array_default) /
 				sizeof(lcore_params_array_default[0]);
 
-static struct rte_eth_conf port_conf = {
+struct rte_eth_conf port_conf = {
 	.rxmode = {
 		.mq_mode = RTE_ETH_MQ_RX_RSS,
 		.offloads = RTE_ETH_RX_OFFLOAD_CHECKSUM,
@@ -1257,8 +1257,12 @@ 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)
+		/* relax the rx rss requirement */
+		if (dev_info.max_rx_queues == 1 || !local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
+			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) {
@@ -1269,6 +1273,19 @@ 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);
+			local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
+			port_conf.rxmode.offloads = local_port_conf.rxmode.offloads;
+			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

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

[Stephen Hemminger]

On Sun,  3 Sep 2023 04:01:11 +0000
Trevor Tao <taozj888@163.com> wrote:

> Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS, and offload
> mode set to RTE_ETH_RX_OFFLOAD_CHECKSUM by default, but some hardware
> 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 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:
> 
> virtio_dev_configure(): RSS support requested but not supported by
> the device
> Port0 dev_configure = -95
> 
> and:
> 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 the
> hw support missing.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Trevor Tao <taozj888@163.com>

Multiple queue without RSS and rte_flow support is kind of useless.
All packets will arrive on one queue.

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

[Konstantin Ananyev]

03/09/2023 05:01, Trevor Tao пишет:
> Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS, and offload
> mode set to RTE_ETH_RX_OFFLOAD_CHECKSUM by default, but some hardware
> 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 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:
> 
> virtio_dev_configure(): RSS support requested but not supported by
> the device
> Port0 dev_configure = -95
> 
> and:
> 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 the
> hw support missing.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org

I don't think there was abug here.
We are talking about changing current requirements for the app.
So not sure it is a real fix and that such change can be
propagated to stable releases.

> 
> Signed-off-by: Trevor Tao <taozj888@163.com>
> ---
>   examples/l3fwd/l3fwd.h | 12 +++++++++++-
>   examples/l3fwd/main.c  | 21 +++++++++++++++++++--
>   2 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
> index b55855c932..cc10643c4b 100644
> --- a/examples/l3fwd/l3fwd.h
> +++ b/examples/l3fwd/l3fwd.h
> @@ -115,6 +115,8 @@ extern struct acl_algorithms acl_alg[];
>   
>   extern uint32_t max_pkt_len;
>   
> +extern struct rte_eth_conf port_conf;
> +
>   /* Send burst of packets on an output interface */
>   static inline int
>   send_burst(struct lcore_conf *qconf, uint16_t n, uint16_t port)
> @@ -170,7 +172,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 ((port_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_IPV4_CKSUM) == 0) {

Might be better to check particular mbuf flag:
if ((mbuf->ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) == 
TE_MBUF_F_RX_IP_CKSUM_UNKNOWN) {...}

> +		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/main.c b/examples/l3fwd/main.c
> index 6063eb1399..37aec64718 100644
> --- a/examples/l3fwd/main.c
> +++ b/examples/l3fwd/main.c
> @@ -117,7 +117,7 @@ static struct lcore_params * lcore_params = lcore_params_array_default;
>   static uint16_t nb_lcore_params = sizeof(lcore_params_array_default) /
>   				sizeof(lcore_params_array_default[0]);
>   
> -static struct rte_eth_conf port_conf = {
> +struct rte_eth_conf port_conf = {
>   	.rxmode = {
>   		.mq_mode = RTE_ETH_MQ_RX_RSS,
>   		.offloads = RTE_ETH_RX_OFFLOAD_CHECKSUM,
> @@ -1257,8 +1257,12 @@ 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)
> +		/* relax the rx rss requirement */
> +		if (dev_info.max_rx_queues == 1 || !local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
> +			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;

Should we probably instead have a new commnad-line option to explicitly 
disable RSS?
Something like: '--no-rss' or so?

> +		}
>   
>   		if (local_port_conf.rx_adv_conf.rss_conf.rss_hf !=
>   				port_conf.rx_adv_conf.rss_conf.rss_hf) {
> @@ -1269,6 +1273,19 @@ 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);
> +			local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
> +			port_conf.rxmode.offloads = local_port_conf.rxmode.offloads;

Why to remove offloads in port_conf?
There could be multiple ports, and on others desired HW offloads might 
be supported.

> +			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)

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

[taozj888]

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

Hi Konstantin:




Please see my comments inline.
Thanks.


Trevor 

At 2023-09-18 02:04:19, "Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru> wrote:
>03/09/2023 05:01, Trevor Tao пишет:
>> Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS, and offload
>> mode set to RTE_ETH_RX_OFFLOAD_CHECKSUM by default, but some hardware
>> 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 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:
>> 
>> virtio_dev_configure(): RSS support requested but not supported by
>> the device
>> Port0 dev_configure = -95
>> 
>> and:
>> 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 the
>> hw support missing.
>> 
>> Fixes: af75078fece3 ("first public release")
>> Cc: stable@dpdk.org
>
>I don't think there was abug here.
>We are talking about changing current requirements for the app.
>So not sure it is a real fix and that such change can be

>propagated to stable releases.
Trevor: I think it's not a bug fix but a feature enhancement, it would enable l3fwd to work smoothly on the HW/virtual interfaces which don't support RSS and/or cksum offloading.


>
>> 
>> Signed-off-by: Trevor Tao <taozj888@163.com>
>> ---
>>   examples/l3fwd/l3fwd.h | 12 +++++++++++-
>>   examples/l3fwd/main.c  | 21 +++++++++++++++++++--
>>   2 files changed, 30 insertions(+), 3 deletions(-)
>> 
>> diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
>> index b55855c932..cc10643c4b 100644
>> --- a/examples/l3fwd/l3fwd.h
>> +++ b/examples/l3fwd/l3fwd.h
>> @@ -115,6 +115,8 @@ extern struct acl_algorithms acl_alg[];
>>   
>>   extern uint32_t max_pkt_len;
>>   
>> +extern struct rte_eth_conf port_conf;
>> +
>>   /* Send burst of packets on an output interface */
>>   static inline int
>>   send_burst(struct lcore_conf *qconf, uint16_t n, uint16_t port)
>> @@ -170,7 +172,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 ((port_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_IPV4_CKSUM) == 0) {
>
>Might be better to check particular mbuf flag:
>if ((mbuf->ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) == 

>TE_MBUF_F_RX_IP_CKSUM_UNKNOWN) {...}
Trevor: the utility function is_valid_ipv4_pkt is just against an IPv4 pkt, and there's no mbuf information, and if needed, there would be an extra ol_flags added here to check if it was already done by the ethernet device, but look for a sample in:
https://github.com/DPDK/dpdk/blob/main/examples/l3fwd-power/main.c#L487
so I think it's ok to just use the port_conf here. If you still think it's better to use m->ol_flags, please tell me.
>
>> +		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/main.c b/examples/l3fwd/main.c
>> index 6063eb1399..37aec64718 100644
>> --- a/examples/l3fwd/main.c
>> +++ b/examples/l3fwd/main.c
>> @@ -117,7 +117,7 @@ static struct lcore_params * lcore_params = lcore_params_array_default;
>>   static uint16_t nb_lcore_params = sizeof(lcore_params_array_default) /
>>   				sizeof(lcore_params_array_default[0]);
>>   
>> -static struct rte_eth_conf port_conf = {
>> +struct rte_eth_conf port_conf = {
>>   	.rxmode = {
>>   		.mq_mode = RTE_ETH_MQ_RX_RSS,
>>   		.offloads = RTE_ETH_RX_OFFLOAD_CHECKSUM,
>> @@ -1257,8 +1257,12 @@ 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)
>> +		/* relax the rx rss requirement */
>> +		if (dev_info.max_rx_queues == 1 || !local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
>> +			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;
>
>Should we probably instead have a new commnad-line option to explicitly 
>disable RSS?

>Something like: '--no-rss' or so?
Trevor: the RSS capability for a certain port was got by the rte_eth_dev_info_get() automatically, and we think the user should not care about its status beforehand, but if it's missing, a warning notification for the degrade here would be proposed to make it run smoothly.
>
>> +		}
>>   
>>   		if (local_port_conf.rx_adv_conf.rss_conf.rss_hf !=
>>   				port_conf.rx_adv_conf.rss_conf.rss_hf) {
>> @@ -1269,6 +1273,19 @@ 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);
>> +			local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
>> +			port_conf.rxmode.offloads = local_port_conf.rxmode.offloads;
>
>Why to remove offloads in port_conf?
>There could be multiple ports, and on others desired HW offloads might 

>be supported.
Trevor: Yes, there would be multiple ports, so if one of the ports lack HW offload, it would be ok to just use the relaxed requirement here, like we previously talked in https://github.com/DPDK/dpdk/blob/main/examples/l3fwd-power/main.c#L487, if you still think it's needed to use the per-port case, it would be ok to use the ol_flags as talked previously.
>
>> +			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)

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

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

[Konstantin Ananyev]

Hi Trevor,


> 
> At 2023-09-18 02:04:19, "Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru> wrote:
>>03/09/2023 05:01, Trevor Tao пишет:
>>> Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS, and offload
>>> mode set to RTE_ETH_RX_OFFLOAD_CHECKSUM by default, but some hardware
>>> 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 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:
>>> 
>>> virtio_dev_configure(): RSS support requested but not supported by
>>> the device
>>> Port0 dev_configure = -95
>>> 
>>> and:
>>> 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 the
>>> hw support missing.
>>> 
>>> Fixes: af75078fece3 ("first public release")
>>> Cc: stable@dpdk.org
>>
>>I don't think there was abug here.
>>We are talking about changing current requirements for the app.
>>So not sure it is a real fix and that such change can be
> 
>>propagated to stable releases.
> Trevor: I think it's not a bug fix but a feature enhancement, it would enable l3fwd to work smoothly on the HW/virtual interfaces which don't support RSS and/or cksum offloading.


Yes. it seems like sort of an enhancement.
While 'Fixes: ...' are for bugs only.
AFAIK, only bug-fixes are take for backporting by stable releases.
That's why there seems no point to add CC: stable@dpdk.org

Another generic things:
- l3fwd doc and release notes probably need to be updated
- as you areintroducing 2 distinct features: no-rss and no-ipv4-cksum
   it is probably better to split it into 2 different patches (in the 
same series).

> 
> 
>>
>>> 
>>> Signed-off-by: Trevor Tao <taozj888@163.com>
>>> ---
>>>   examples/l3fwd/l3fwd.h | 12 +++++++++++-
>>>   examples/l3fwd/main.c  | 21 +++++++++++++++++++--
>>>   2 files changed, 30 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
>>> index b55855c932..cc10643c4b 100644
>>> --- a/examples/l3fwd/l3fwd.h
>>> +++ b/examples/l3fwd/l3fwd.h
>>> @@ -115,6 +115,8 @@ extern struct acl_algorithms acl_alg[];
>>>   
>>>   extern uint32_t max_pkt_len;
>>>   
>>> +extern struct rte_eth_conf port_conf;
>>> +
>>>   /* Send burst of packets on an output interface */
>>>   static inline int
>>>   send_burst(struct lcore_conf *qconf, uint16_t n, uint16_t port)
>>> @@ -170,7 +172,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 ((port_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_IPV4_CKSUM) == 0) {
>>
>>Might be better to check particular mbuf flag:
>>if ((mbuf->ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) == 
> 
>>TE_MBUF_F_RX_IP_CKSUM_UNKNOWN) {...}
> Trevor: the utility function is_valid_ipv4_pkt is just against an IPv4 pkt, and there's no mbuf information, and if needed, there would be an extra ol_flags added here to check if it was already done by the ethernet device, but look for a sample in:
> https://github.com/DPDK/dpdk/blob/main/examples/l3fwd-power/main.c#L487
> so I think it's ok to just use the port_conf here. If you still think it's better to use m->ol_flags, please tell me.


Yep, passing ol_flags, or mbuf itself seems like a proper way to do it.
Aproach taken in l3fwd-power doesn't look right to me, see below.

>>
>>> +		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/main.c b/examples/l3fwd/main.c
>>> index 6063eb1399..37aec64718 100644
>>> --- a/examples/l3fwd/main.c
>>> +++ b/examples/l3fwd/main.c
>>> @@ -117,7 +117,7 @@ static struct lcore_params * lcore_params = lcore_params_array_default;
>>>   static uint16_t nb_lcore_params = sizeof(lcore_params_array_default) /
>>>   				sizeof(lcore_params_array_default[0]);
>>>   
>>> -static struct rte_eth_conf port_conf = {
>>> +struct rte_eth_conf port_conf = {
>>>   	.rxmode = {
>>>   		.mq_mode = RTE_ETH_MQ_RX_RSS,
>>>   		.offloads = RTE_ETH_RX_OFFLOAD_CHECKSUM,
>>> @@ -1257,8 +1257,12 @@ 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)
>>> +		/* relax the rx rss requirement */
>>> +		if (dev_info.max_rx_queues == 1 || !local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
>>> +			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;
>>
>>Should we probably instead have a new commnad-line option to explicitly 
>>disable RSS?
> 
>>Something like: '--no-rss' or so?
> Trevor: the RSS capability for a certain port was got by the rte_eth_dev_info_get() automatically, and we think the user should not care about its status beforehand, but if it's missing, a warning notification for the degrade here would be proposed to make it run smoothly.

Personally, I still think it would be better the user will
have an ability to disable it explicitly.
Same as l3fwd does now with 'parse-ptype'.

>>
>>> +		}
>>>   
>>>   		if (local_port_conf.rx_adv_conf.rss_conf.rss_hf !=
>>>   				port_conf.rx_adv_conf.rss_conf.rss_hf) {
>>> @@ -1269,6 +1273,19 @@ 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);
>>> +			local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
>>> +			port_conf.rxmode.offloads = local_port_conf.rxmode.offloads;
>>
>>Why to remove offloads in port_conf?
>>There could be multiple ports, and on others desired HW offloads might 
> 
>>be supported.
> Trevor: Yes, there would be multiple ports, so if one of the ports lack HW offload, it would be ok to just use the relaxed requirement here, like we previously talked in https://github.com/DPDK/dpdk/blob/main/examples/l3fwd-power/main.c#L487, if you still think it's needed to use the per-port case, it would be ok to use the ol_flags as talked previously.


But then, depending on the ports order you can end-up with IP_CKSUM 
offload enabled on some ports (but not used), while completely disable 
on other ports - even if these ports do support IP_CKSUM.
I think the better way would be not to touch port_conf here, and above
use ol_flags to decide should we compute cksum in SW or not.


>>
>>> +			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)

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

[taozj888]

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




Hi Konstantin,


Please see my comments inline
Thanks.




Trevor
At 2023-09-20 16:04:41, "Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru> wrote:
>
>Hi Trevor,
>
>
>> 
>> At 2023-09-18 02:04:19, "Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru> wrote:
>>>03/09/2023 05:01, Trevor Tao пишет:
>>>> Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS, and offload
>>>> mode set to RTE_ETH_RX_OFFLOAD_CHECKSUM by default, but some hardware
>>>> 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 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:
>>>> 
>>>> virtio_dev_configure(): RSS support requested but not supported by
>>>> the device
>>>> Port0 dev_configure = -95
>>>> 
>>>> and:
>>>> 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 the
>>>> hw support missing.
>>>> 
>>>> Fixes: af75078fece3 ("first public release")
>>>> Cc: stable@dpdk.org
>>>
>>>I don't think there was abug here.
>>>We are talking about changing current requirements for the app.
>>>So not sure it is a real fix and that such change can be
>> 
>>>propagated to stable releases.
>> Trevor: I think it's not a bug fix but a feature enhancement, it would enable l3fwd to work smoothly on the HW/virtual interfaces which don't support RSS and/or cksum offloading.
>
>
>Yes. it seems like sort of an enhancement.
>While 'Fixes: ...' are for bugs only.
>AFAIK, only bug-fixes are take for backporting by stable releases.
>That's why there seems no point to add CC: stable@dpdk.org
>
>Another generic things:

>- l3fwd doc and release notes probably need to be updated
Trevor>>I think it's ok to update the l3fwd doc and release notes, but I would like to know which part of the doc/notes is approriate to add the enhancement declaration. 
>- as you areintroducing 2 distinct features: no-rss and no-ipv4-cksum
>   it is probably better to split it into 2 different patches (in the 

>same series).
Trevor>>I think it's ok to split it into 2 patches here in the same series, if you would like to.
Thanks.
>
>> 
>> 
>>>
>>>> 
>>>> Signed-off-by: Trevor Tao <taozj888@163.com>
>>>> ---
>>>>   examples/l3fwd/l3fwd.h | 12 +++++++++++-
>>>>   examples/l3fwd/main.c  | 21 +++++++++++++++++++--
>>>>   2 files changed, 30 insertions(+), 3 deletions(-)
>>>> 
>>>> diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
>>>> index b55855c932..cc10643c4b 100644
>>>> --- a/examples/l3fwd/l3fwd.h
>>>> +++ b/examples/l3fwd/l3fwd.h
>>>> @@ -115,6 +115,8 @@ extern struct acl_algorithms acl_alg[];
>>>>   
>>>>   extern uint32_t max_pkt_len;
>>>>   
>>>> +extern struct rte_eth_conf port_conf;
>>>> +
>>>>   /* Send burst of packets on an output interface */
>>>>   static inline int
>>>>   send_burst(struct lcore_conf *qconf, uint16_t n, uint16_t port)
>>>> @@ -170,7 +172,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 ((port_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_IPV4_CKSUM) == 0) {
>>>
>>>Might be better to check particular mbuf flag:
>>>if ((mbuf->ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) == 
>> 
>>>TE_MBUF_F_RX_IP_CKSUM_UNKNOWN) {...}
>> Trevor: the utility function is_valid_ipv4_pkt is just against an IPv4 pkt, and there's no mbuf information, and if needed, there would be an extra ol_flags added here to check if it was already done by the ethernet device, but look for a sample in:
>> https://github.com/DPDK/dpdk/blob/main/examples/l3fwd-power/main.c#L487
>> so I think it's ok to just use the port_conf here. If you still think it's better to use m->ol_flags, please tell me.
>
>
>Yep, passing ol_flags, or mbuf itself seems like a proper way to do it.
>Aproach taken in l3fwd-power doesn't look right to me, see below.
>
>>>
>>>> +		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/main.c b/examples/l3fwd/main.c
>>>> index 6063eb1399..37aec64718 100644
>>>> --- a/examples/l3fwd/main.c
>>>> +++ b/examples/l3fwd/main.c
>>>> @@ -117,7 +117,7 @@ static struct lcore_params * lcore_params = lcore_params_array_default;
>>>>   static uint16_t nb_lcore_params = sizeof(lcore_params_array_default) /
>>>>   				sizeof(lcore_params_array_default[0]);
>>>>   
>>>> -static struct rte_eth_conf port_conf = {
>>>> +struct rte_eth_conf port_conf = {
>>>>   	.rxmode = {
>>>>   		.mq_mode = RTE_ETH_MQ_RX_RSS,
>>>>   		.offloads = RTE_ETH_RX_OFFLOAD_CHECKSUM,
>>>> @@ -1257,8 +1257,12 @@ 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)
>>>> +		/* relax the rx rss requirement */
>>>> +		if (dev_info.max_rx_queues == 1 || !local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
>>>> +			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;
>>>
>>>Should we probably instead have a new commnad-line option to explicitly 
>>>disable RSS?
>> 
>>>Something like: '--no-rss' or so?
>> Trevor: the RSS capability for a certain port was got by the rte_eth_dev_info_get() automatically, and we think the user should not care about its status beforehand, but if it's missing, a warning notification for the degrade here would be proposed to make it run smoothly.
>
>Personally, I still think it would be better the user will
>have an ability to disable it explicitly.
>Same as l3fwd does now with 'parse-ptype'.
>
>>>
>>>> +		}
>>>>   
>>>>   		if (local_port_conf.rx_adv_conf.rss_conf.rss_hf !=
>>>>   				port_conf.rx_adv_conf.rss_conf.rss_hf) {
>>>> @@ -1269,6 +1273,19 @@ 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);
>>>> +			local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
>>>> +			port_conf.rxmode.offloads = local_port_conf.rxmode.offloads;
>>>
>>>Why to remove offloads in port_conf?
>>>There could be multiple ports, and on others desired HW offloads might 
>> 
>>>be supported.
>> Trevor: Yes, there would be multiple ports, so if one of the ports lack HW offload, it would be ok to just use the relaxed requirement here, like we previously talked in https://github.com/DPDK/dpdk/blob/main/examples/l3fwd-power/main.c#L487, if you still think it's needed to use the per-port case, it would be ok to use the ol_flags as talked previously.
>
>
>But then, depending on the ports order you can end-up with IP_CKSUM 
>offload enabled on some ports (but not used), while completely disable 
>on other ports - even if these ports do support IP_CKSUM.
>I think the better way would be not to touch port_conf here, and above
>use ol_flags to decide should we compute cksum in SW or not.
>
>
>>>
>>>> +			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)

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

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

[Konstantin Ananyev]

Hi Trevor,
>>
>>
>>> 
>>> At 2023-09-18 02:04:19, "Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru> wrote:
>>>>03/09/2023 05:01, Trevor Tao пишет:
>>>>> Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS, and offload
>>>>> mode set to RTE_ETH_RX_OFFLOAD_CHECKSUM by default, but some hardware
>>>>> 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 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:
>>>>> 
>>>>> virtio_dev_configure(): RSS support requested but not supported by
>>>>> the device
>>>>> Port0 dev_configure = -95
>>>>> 
>>>>> and:
>>>>> 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 the
>>>>> hw support missing.
>>>>> 
>>>>> Fixes: af75078fece3 ("first public release")
>>>>> Cc: stable@dpdk.org
>>>>
>>>>I don't think there was abug here.
>>>>We are talking about changing current requirements for the app.
>>>>So not sure it is a real fix and that such change can be
>>> 
>>>>propagated to stable releases.
>>> Trevor: I think it's not a bug fix but a feature enhancement, it would enable l3fwd to work smoothly on the HW/virtual interfaces which don't support RSS and/or cksum offloading.
>>
>>
>>Yes. it seems like sort of an enhancement.
>>While 'Fixes: ...' are for bugs only.
>>AFAIK, only bug-fixes are take for backporting by stable releases.
>>That's why there seems no point to add CC: stable@dpdk.org
>>
>>Another generic things:
>  >- l3fwd doc and release notes probably need to be updated
> *Trevor>>I think it's ok to update the l3fwd doc and release notes, but 
> I would like to know which part of the doc/notes is approriate to add 
> the enhancement declaration. *

  think both:
http://doc.dpdk.org/guides/sample_app_ug/l3_forward.html
and elease notes in doc/guides/rel_notes/ need to be updated.

>>- as you areintroducing 2 distinct features: no-rss and no-ipv4-cksum
>>   it is probably better to split it into 2 different patches (in the 
>  >same series).
> *Trevor>>I think it's ok to split it into 2 patches here in the same 
> series, if you would like to.*
> *Thanks.*

That is not my own desire, but usual contrution practise we all try
to comply with.
You can find more details at:
https://doc.dpdk.org/guides/contributing/patches.html

Thanks
Konstantin


>>
>>> 
>>> 
>>>>
>>>>> 
>>>>> Signed-off-by: Trevor Tao <taozj888@163.com>
>>>>> ---
>>>>>   examples/l3fwd/l3fwd.h | 12 +++++++++++-
>>>>>   examples/l3fwd/main.c  | 21 +++++++++++++++++++--
>>>>>   2 files changed, 30 insertions(+), 3 deletions(-)
>>>>> 
>>>>> diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
>>>>> index b55855c932..cc10643c4b 100644
>>>>> --- a/examples/l3fwd/l3fwd.h
>>>>> +++ b/examples/l3fwd/l3fwd.h
>>>>> @@ -115,6 +115,8 @@ extern struct acl_algorithms acl_alg[];
>>>>>   
>>>>>   extern uint32_t max_pkt_len;
>>>>>   
>>>>> +extern struct rte_eth_conf port_conf;
>>>>> +
>>>>>   /* Send burst of packets on an output interface */
>>>>>   static inline int
>>>>>   send_burst(struct lcore_conf *qconf, uint16_t n, uint16_t port)
>>>>> @@ -170,7 +172,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 ((port_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_IPV4_CKSUM) == 0) {
>>>>
>>>>Might be better to check particular mbuf flag:
>>>>if ((mbuf->ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) == 
>>> 
>>>>TE_MBUF_F_RX_IP_CKSUM_UNKNOWN) {...}
>>> Trevor: the utility function is_valid_ipv4_pkt is just against an IPv4 pkt, and there's no mbuf information, and if needed, there would be an extra ol_flags added here to check if it was already done by the ethernet device, but look for a sample in:
>>> https://github.com/DPDK/dpdk/blob/main/examples/l3fwd-power/main.c#L487
>>> so I think it's ok to just use the port_conf here. If you still think it's better to use m->ol_flags, please tell me.
>>
>>
>>Yep, passing ol_flags, or mbuf itself seems like a proper way to do it.
>>Aproach taken in l3fwd-power doesn't look right to me, see below.
>>
>>>>
>>>>> +		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/main.c b/examples/l3fwd/main.c
>>>>> index 6063eb1399..37aec64718 100644
>>>>> --- a/examples/l3fwd/main.c
>>>>> +++ b/examples/l3fwd/main.c
>>>>> @@ -117,7 +117,7 @@ static struct lcore_params * lcore_params = lcore_params_array_default;
>>>>>   static uint16_t nb_lcore_params = sizeof(lcore_params_array_default) /
>>>>>   				sizeof(lcore_params_array_default[0]);
>>>>>   
>>>>> -static struct rte_eth_conf port_conf = {
>>>>> +struct rte_eth_conf port_conf = {
>>>>>   	.rxmode = {
>>>>>   		.mq_mode = RTE_ETH_MQ_RX_RSS,
>>>>>   		.offloads = RTE_ETH_RX_OFFLOAD_CHECKSUM,
>>>>> @@ -1257,8 +1257,12 @@ 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)
>>>>> +		/* relax the rx rss requirement */
>>>>> +		if (dev_info.max_rx_queues == 1 || !local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
>>>>> +			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;
>>>>
>>>>Should we probably instead have a new commnad-line option to explicitly 
>>>>disable RSS?
>>> 
>>>>Something like: '--no-rss' or so?
>>> Trevor: the RSS capability for a certain port was got by the rte_eth_dev_info_get() automatically, and we think the user should not care about its status beforehand, but if it's missing, a warning notification for the degrade here would be proposed to make it run smoothly.
>>
>>Personally, I still think it would be better the user will
>>have an ability to disable it explicitly.
>>Same as l3fwd does now with 'parse-ptype'.
>>
>>>>
>>>>> +		}
>>>>>   
>>>>>   		if (local_port_conf.rx_adv_conf.rss_conf.rss_hf !=
>>>>>   				port_conf.rx_adv_conf.rss_conf.rss_hf) {
>>>>> @@ -1269,6 +1273,19 @@ 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);
>>>>> +			local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
>>>>> +			port_conf.rxmode.offloads = local_port_conf.rxmode.offloads;
>>>>
>>>>Why to remove offloads in port_conf?
>>>>There could be multiple ports, and on others desired HW offloads might 
>>> 
>>>>be supported.
>>> Trevor: Yes, there would be multiple ports, so if one of the ports lack HW offload, it would be ok to just use the relaxed requirement here, like we previously talked in https://github.com/DPDK/dpdk/blob/main/examples/l3fwd-power/main.c#L487, if you still think it's needed to use the per-port case, it would be ok to use the ol_flags as talked previously.
>>
>>
>>But then, depending on the ports order you can end-up with IP_CKSUM 
>>offload enabled on some ports (but not used), while completely disable 
>>on other ports - even if these ports do support IP_CKSUM.
>>I think the better way would be not to touch port_conf here, and above
>>use ol_flags to decide should we compute cksum in SW or not.
>>
>>
>>>>
>>>>> +			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)

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

[taozj888]

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

Hi Konstantin,


I thought your comments make sense, and addressed them with a patchset https://patches.dpdk.org/project/dpdk/list/?series=29709,
please check again.
Thanks,


Trevor Tao

At 2023-09-26 21:49:05, "Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru> wrote:
>
>Hi Trevor,
>>>
>>>
>>>> 
>>>> At 2023-09-18 02:04:19, "Konstantin Ananyev" <konstantin.v.ananyev@yandex.ru> wrote:
>>>>>03/09/2023 05:01, Trevor Tao пишет:
>>>>>> Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS, and offload
>>>>>> mode set to RTE_ETH_RX_OFFLOAD_CHECKSUM by default, but some hardware
>>>>>> 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 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:
>>>>>> 
>>>>>> virtio_dev_configure(): RSS support requested but not supported by
>>>>>> the device
>>>>>> Port0 dev_configure = -95
>>>>>> 
>>>>>> and:
>>>>>> 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 the
>>>>>> hw support missing.
>>>>>> 
>>>>>> Fixes: af75078fece3 ("first public release")
>>>>>> Cc: stable@dpdk.org
>>>>>
>>>>>I don't think there was abug here.
>>>>>We are talking about changing current requirements for the app.
>>>>>So not sure it is a real fix and that such change can be
>>>> 
>>>>>propagated to stable releases.
>>>> Trevor: I think it's not a bug fix but a feature enhancement, it would enable l3fwd to work smoothly on the HW/virtual interfaces which don't support RSS and/or cksum offloading.
>>>
>>>
>>>Yes. it seems like sort of an enhancement.
>>>While 'Fixes: ...' are for bugs only.
>>>AFAIK, only bug-fixes are take for backporting by stable releases.
>>>That's why there seems no point to add CC: stable@dpdk.org
>>>
>>>Another generic things:
>>  >- l3fwd doc and release notes probably need to be updated
>> *Trevor>>I think it's ok to update the l3fwd doc and release notes, but 
>> I would like to know which part of the doc/notes is approriate to add 
>> the enhancement declaration. *
>
>  think both:
>http://doc.dpdk.org/guides/sample_app_ug/l3_forward.html
>and elease notes in doc/guides/rel_notes/ need to be updated.
>
>>>- as you areintroducing 2 distinct features: no-rss and no-ipv4-cksum
>>>   it is probably better to split it into 2 different patches (in the 
>>  >same series).
>> *Trevor>>I think it's ok to split it into 2 patches here in the same 
>> series, if you would like to.*
>> *Thanks.*
>
>That is not my own desire, but usual contrution practise we all try
>to comply with.
>You can find more details at:
>https://doc.dpdk.org/guides/contributing/patches.html
>
>Thanks
>Konstantin
>
>
>>>
>>>> 
>>>> 
>>>>>
>>>>>> 
>>>>>> Signed-off-by: Trevor Tao <taozj888@163.com>
>>>>>> ---
>>>>>>   examples/l3fwd/l3fwd.h | 12 +++++++++++-
>>>>>>   examples/l3fwd/main.c  | 21 +++++++++++++++++++--
>>>>>>   2 files changed, 30 insertions(+), 3 deletions(-)
>>>>>> 
>>>>>> diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
>>>>>> index b55855c932..cc10643c4b 100644
>>>>>> --- a/examples/l3fwd/l3fwd.h
>>>>>> +++ b/examples/l3fwd/l3fwd.h
>>>>>> @@ -115,6 +115,8 @@ extern struct acl_algorithms acl_alg[];
>>>>>>   
>>>>>>   extern uint32_t max_pkt_len;
>>>>>>   
>>>>>> +extern struct rte_eth_conf port_conf;
>>>>>> +
>>>>>>   /* Send burst of packets on an output interface */
>>>>>>   static inline int
>>>>>>   send_burst(struct lcore_conf *qconf, uint16_t n, uint16_t port)
>>>>>> @@ -170,7 +172,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 ((port_conf.rxmode.offloads & RTE_ETH_RX_OFFLOAD_IPV4_CKSUM) == 0) {
>>>>>
>>>>>Might be better to check particular mbuf flag:
>>>>>if ((mbuf->ol_flags & RTE_MBUF_F_RX_IP_CKSUM_MASK) == 
>>>> 
>>>>>TE_MBUF_F_RX_IP_CKSUM_UNKNOWN) {...}
>>>> Trevor: the utility function is_valid_ipv4_pkt is just against an IPv4 pkt, and there's no mbuf information, and if needed, there would be an extra ol_flags added here to check if it was already done by the ethernet device, but look for a sample in:
>>>> https://github.com/DPDK/dpdk/blob/main/examples/l3fwd-power/main.c#L487
>>>> so I think it's ok to just use the port_conf here. If you still think it's better to use m->ol_flags, please tell me.
>>>
>>>
>>>Yep, passing ol_flags, or mbuf itself seems like a proper way to do it.
>>>Aproach taken in l3fwd-power doesn't look right to me, see below.
>>>
>>>>>
>>>>>> +		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/main.c b/examples/l3fwd/main.c
>>>>>> index 6063eb1399..37aec64718 100644
>>>>>> --- a/examples/l3fwd/main.c
>>>>>> +++ b/examples/l3fwd/main.c
>>>>>> @@ -117,7 +117,7 @@ static struct lcore_params * lcore_params = lcore_params_array_default;
>>>>>>   static uint16_t nb_lcore_params = sizeof(lcore_params_array_default) /
>>>>>>   				sizeof(lcore_params_array_default[0]);
>>>>>>   
>>>>>> -static struct rte_eth_conf port_conf = {
>>>>>> +struct rte_eth_conf port_conf = {
>>>>>>   	.rxmode = {
>>>>>>   		.mq_mode = RTE_ETH_MQ_RX_RSS,
>>>>>>   		.offloads = RTE_ETH_RX_OFFLOAD_CHECKSUM,
>>>>>> @@ -1257,8 +1257,12 @@ 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)
>>>>>> +		/* relax the rx rss requirement */
>>>>>> +		if (dev_info.max_rx_queues == 1 || !local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
>>>>>> +			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;
>>>>>
>>>>>Should we probably instead have a new commnad-line option to explicitly 
>>>>>disable RSS?
>>>> 
>>>>>Something like: '--no-rss' or so?
>>>> Trevor: the RSS capability for a certain port was got by the rte_eth_dev_info_get() automatically, and we think the user should not care about its status beforehand, but if it's missing, a warning notification for the degrade here would be proposed to make it run smoothly.
>>>
>>>Personally, I still think it would be better the user will
>>>have an ability to disable it explicitly.
>>>Same as l3fwd does now with 'parse-ptype'.
>>>
>>>>>
>>>>>> +		}
>>>>>>   
>>>>>>   		if (local_port_conf.rx_adv_conf.rss_conf.rss_hf !=
>>>>>>   				port_conf.rx_adv_conf.rss_conf.rss_hf) {
>>>>>> @@ -1269,6 +1273,19 @@ 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);
>>>>>> +			local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
>>>>>> +			port_conf.rxmode.offloads = local_port_conf.rxmode.offloads;
>>>>>
>>>>>Why to remove offloads in port_conf?
>>>>>There could be multiple ports, and on others desired HW offloads might 
>>>> 
>>>>>be supported.
>>>> Trevor: Yes, there would be multiple ports, so if one of the ports lack HW offload, it would be ok to just use the relaxed requirement here, like we previously talked in https://github.com/DPDK/dpdk/blob/main/examples/l3fwd-power/main.c#L487, if you still think it's needed to use the per-port case, it would be ok to use the ol_flags as talked previously.
>>>
>>>
>>>But then, depending on the ports order you can end-up with IP_CKSUM 
>>>offload enabled on some ports (but not used), while completely disable 
>>>on other ports - even if these ports do support IP_CKSUM.
>>>I think the better way would be not to touch port_conf here, and above
>>>use ol_flags to decide should we compute cksum in SW or not.
>>>
>>>
>>>>>
>>>>>> +			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)

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

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

[Trevor Tao]

Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS, and offload
mode set to RTE_ETH_RX_OFFLOAD_CHECKSUM by default, but some hardware
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 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:

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

and:
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 the
hw support missing.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Trevor Tao <taozj888@163.com>
---
 examples/l3fwd/l3fwd.h | 12 +++++++++++-
 examples/l3fwd/main.c  | 21 +++++++++++++++++++--
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/examples/l3fwd/l3fwd.h b/examples/l3fwd/l3fwd.h
index b55855c932..cc10643c4b 100644
--- a/examples/l3fwd/l3fwd.h
+++ b/examples/l3fwd/l3fwd.h
@@ -115,6 +115,8 @@ extern struct acl_algorithms acl_alg[];
 
 extern uint32_t max_pkt_len;
 
+extern struct rte_eth_conf port_conf;
+
 /* Send burst of packets on an output interface */
 static inline int
 send_burst(struct lcore_conf *qconf, uint16_t n, uint16_t port)
@@ -170,7 +172,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 ((port_conf.rxmode.offloads & 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/main.c b/examples/l3fwd/main.c
index 6063eb1399..37aec64718 100644
--- a/examples/l3fwd/main.c
+++ b/examples/l3fwd/main.c
@@ -117,7 +117,7 @@ static struct lcore_params * lcore_params = lcore_params_array_default;
 static uint16_t nb_lcore_params = sizeof(lcore_params_array_default) /
 				sizeof(lcore_params_array_default[0]);
 
-static struct rte_eth_conf port_conf = {
+struct rte_eth_conf port_conf = {
 	.rxmode = {
 		.mq_mode = RTE_ETH_MQ_RX_RSS,
 		.offloads = RTE_ETH_RX_OFFLOAD_CHECKSUM,
@@ -1257,8 +1257,12 @@ 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)
+		/* relax the rx rss requirement */
+		if (dev_info.max_rx_queues == 1 || !local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
+			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) {
@@ -1269,6 +1273,19 @@ 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);
+			local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
+			port_conf.rxmode.offloads = local_port_conf.rxmode.offloads;
+			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

RE: [PATCH v1] examples/l3fwd: relax the RSS/Offload requirement

[Konstantin Ananyev]

Hi Tao,
 
> HI Konstantin:
> 
> I do understand your requirement on the SW support for the IPV4 cksum verification, and I think it really can be added here later
> some time when missing HW support.
> Anyway, there is a "warning:" message had been sent out to notify the user there is a lack of HW capability support for packets, and it
> would enable this missing case can run smoothly for some urgent users which really exist with our experiences.
> On the other side, maybe another hint of "warning: no HW check for IPv4 checksum" or something alike could be helpful to users
> before the SW support added.

As I already said, right now l3fwd relies on these HW features to provide
essential part of its expected functionality.
Removing these checks mean that l3fwd will start to behave differently
(depending on the HW).
To me it is sort of change in behavior and break of existing functionality. 
I am afraid that simply printing a warning message is not enough here. 
Till SW equivalent for reduced HW offloads in place,
my vote is NACK for this patch.
Konstantin

P.S. as a side note - usually for dev mailing list, we use to put reply inline,
not on top of the message. 

> Thanks,
> 
> Best Regards,
> 
> Zijin Tao(Trevor Tao, 陶孜谨)
> ARM Electronic Technology (Shanghai) Co., Ltd
> 安谋电子科技（上海）有限公司
> Building 11, Shanghai Busininess ParkⅢ ,
> No.1016 Tianlin Rd, Minhang District, Shanghai, 200233 China
> 上海市闵行区田林路1016号科技绿洲三期2号楼10楼，200233
> Cell:      +86-153 7109 6192
> 
> -----Original Message-----
> From: Konstantin Ananyev <konstantin.ananyev@huawei.com>
> Sent: Friday, July 28, 2023 4:03 PM
> To: Trevor Tao <Trevor.Tao@arm.com>; thomas@monjalon.net
> Cc: dev@dpdk.org; nd <nd@arm.com>; stable@dpdk.org; Ruifeng Wang <Ruifeng.Wang@arm.com>; Feifei Wang
> <Feifei.Wang2@arm.com>
> Subject: RE: [PATCH v1] examples/l3fwd: relax the RSS/Offload requirement
> 
> 
> 
> > Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS, and offload
> > mode set to RTE_ETH_RX_OFFLOAD_CHECKSUM by default, but some hardware
> > 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 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:
> 
> Well, these HW offloads are there for the good reason - l3fwd app relies on these HW features to provide functionality requested.
> It relies on RTE_ETH_RX_OFFLOAD_IPV4_CKSUM to avoid checks of ip cksum in SW:
> static inline int
> is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len) {
>         /* From http://www.rfc-editor.org/rfc/rfc1812.txt section 5.2.2 */
>         /*
>          * 1. The packet length reported by the Link Layer must be large
>          * enough to hold the minimum length legal IP datagram (20 bytes).
>          */
>         if (link_len < sizeof(struct rte_ipv4_hdr))
>                 return -1;
> 
>         /* 2. The IP checksum must be correct. */
>         /* this is checked in H/W */
>         ....
> 
> By having RSS enabled it ensures that packets from the same 'flow' will be processed and send out in order. Probably not a strict
> requirement for l3fwd itself, but definitely nice to have feature that majority of DPDK customers are interested in.
> I do understand your desire to lower HW requirements for l3fwd, but then probably shouldn't be just blind disable, but instead add
> SW support for them when essential HW feature is missing.
> 
> Konstantin
> 
> > virtio_dev_configure(): RSS support requested but not supported by the
> > device
> > Port0 dev_configure = -95
> >
> > and:
> > 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.
> >
> > Fixes: af75078fece3 ("first public release")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Trevor Tao <trevor.tao@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > Reviewed-by: Feifei Wang <feifei.wang2@arm.com>
> > ---
> >  .mailmap              |  1 +
> >  examples/l3fwd/main.c | 19 ++++++++++++++++++-
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/.mailmap b/.mailmap
> > index 8e3940a253..602d8cbc6b 100644
> > --- a/.mailmap
> > +++ b/.mailmap
> > @@ -1403,6 +1403,7 @@ Tom Rix <trix@redhat.com>  Tone Zhang
> > <tone.zhang@arm.com>  Tonghao Zhang <xiangxia.m.yue@gmail.com>
> > <nic@opencloud.tech>  Tony Nguyen <anthony.l.nguyen@intel.com>
> > +Trevor Tao <trevor.tao@arm.com>
> >  Tsotne Chakhvadze <tsotne.chakhvadze@intel.com>  Tudor Brindus
> > <me@tbrindus.ca>  Tudor Cornea <tudor.cornea@gmail.com>
> > <tudor.cornea@keysight.com> diff --git a/examples/l3fwd/main.c
> > b/examples/l3fwd/main.c index a4f061537e..cec87d95d1 100644
> > --- a/examples/l3fwd/main.c
> > +++ b/examples/l3fwd/main.c
> > @@ -1233,8 +1233,12 @@ 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)
> > +		/* relax the rx rss requirement */
> > +		if (dev_info.max_rx_queues == 1 || !local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
> > +			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) { @@ -1245,6 +1249,19 @@
> > 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);
> > +			local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
> > +			port_conf.rxmode.offloads = local_port_conf.rxmode.offloads;
> > +			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.41.0
> >

RE: [PATCH v1] examples/l3fwd: relax the RSS/Offload requirement

[Trevor Tao]

HI Konstantin:

I do understand your requirement on the SW support for the IPV4 cksum verification, and I think it really can be added here later some time when missing HW support.
Anyway, there is a "warning:" message had been sent out to notify the user there is a lack of HW capability support for packets, and it would enable this missing case can run smoothly for some urgent users which really exist with our experiences.
On the other side, maybe another hint of "warning: no HW check for IPv4 checksum" or something alike could be helpful to users before the SW support added. 

Thanks,

Best Regards,
 
Zijin Tao(Trevor Tao, 陶孜谨)
ARM Electronic Technology (Shanghai) Co., Ltd
安谋电子科技（上海）有限公司
Building 11, Shanghai Busininess ParkⅢ ,
No.1016 Tianlin Rd, Minhang District, Shanghai, 200233 China
上海市闵行区田林路1016号科技绿洲三期2号楼10楼，200233
Cell:      +86-153 7109 6192

-----Original Message-----
From: Konstantin Ananyev <konstantin.ananyev@huawei.com> 
Sent: Friday, July 28, 2023 4:03 PM
To: Trevor Tao <Trevor.Tao@arm.com>; thomas@monjalon.net
Cc: dev@dpdk.org; nd <nd@arm.com>; stable@dpdk.org; Ruifeng Wang <Ruifeng.Wang@arm.com>; Feifei Wang <Feifei.Wang2@arm.com>
Subject: RE: [PATCH v1] examples/l3fwd: relax the RSS/Offload requirement



> Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS, and offload 
> mode set to RTE_ETH_RX_OFFLOAD_CHECKSUM by default, but some hardware 
> 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 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:

Well, these HW offloads are there for the good reason - l3fwd app relies on these HW features to provide functionality requested.
It relies on RTE_ETH_RX_OFFLOAD_IPV4_CKSUM to avoid checks of ip cksum in SW:
static inline int
is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len) {
        /* From http://www.rfc-editor.org/rfc/rfc1812.txt section 5.2.2 */
        /*
         * 1. The packet length reported by the Link Layer must be large
         * enough to hold the minimum length legal IP datagram (20 bytes).
         */
        if (link_len < sizeof(struct rte_ipv4_hdr))
                return -1;

        /* 2. The IP checksum must be correct. */
        /* this is checked in H/W */ 
        ....

By having RSS enabled it ensures that packets from the same 'flow' will be processed and send out in order. Probably not a strict requirement for l3fwd itself, but definitely nice to have feature that majority of DPDK customers are interested in.
I do understand your desire to lower HW requirements for l3fwd, but then probably shouldn't be just blind disable, but instead add SW support for them when essential HW feature is missing. 

Konstantin
 
> virtio_dev_configure(): RSS support requested but not supported by the 
> device
> Port0 dev_configure = -95
> 
> and:
> 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.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Trevor Tao <trevor.tao@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Feifei Wang <feifei.wang2@arm.com>
> ---
>  .mailmap              |  1 +
>  examples/l3fwd/main.c | 19 ++++++++++++++++++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/.mailmap b/.mailmap
> index 8e3940a253..602d8cbc6b 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1403,6 +1403,7 @@ Tom Rix <trix@redhat.com>  Tone Zhang 
> <tone.zhang@arm.com>  Tonghao Zhang <xiangxia.m.yue@gmail.com> 
> <nic@opencloud.tech>  Tony Nguyen <anthony.l.nguyen@intel.com>
> +Trevor Tao <trevor.tao@arm.com>
>  Tsotne Chakhvadze <tsotne.chakhvadze@intel.com>  Tudor Brindus 
> <me@tbrindus.ca>  Tudor Cornea <tudor.cornea@gmail.com> 
> <tudor.cornea@keysight.com> diff --git a/examples/l3fwd/main.c 
> b/examples/l3fwd/main.c index a4f061537e..cec87d95d1 100644
> --- a/examples/l3fwd/main.c
> +++ b/examples/l3fwd/main.c
> @@ -1233,8 +1233,12 @@ 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)
> +		/* relax the rx rss requirement */
> +		if (dev_info.max_rx_queues == 1 || !local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
> +			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) { @@ -1245,6 +1249,19 @@ 
> 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);
> +			local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
> +			port_conf.rxmode.offloads = local_port_conf.rxmode.offloads;
> +			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.41.0
> 

RE: [PATCH v1] examples/l3fwd: relax the RSS/Offload requirement

[Konstantin Ananyev]

> Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS, and offload
> mode set to RTE_ETH_RX_OFFLOAD_CHECKSUM by default, but some hardware
> 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 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:

Well, these HW offloads are there for the good reason -
l3fwd app relies on these HW features to provide functionality requested.
It relies on RTE_ETH_RX_OFFLOAD_IPV4_CKSUM to avoid checks of ip cksum in SW:
static inline int
is_valid_ipv4_pkt(struct rte_ipv4_hdr *pkt, uint32_t link_len)
{
        /* From http://www.rfc-editor.org/rfc/rfc1812.txt section 5.2.2 */
        /*
         * 1. The packet length reported by the Link Layer must be large
         * enough to hold the minimum length legal IP datagram (20 bytes).
         */
        if (link_len < sizeof(struct rte_ipv4_hdr))
                return -1;

        /* 2. The IP checksum must be correct. */
        /* this is checked in H/W */ 
        ....

By having RSS enabled it ensures that packets from the same 'flow' will
be processed and send out in order. Probably not a strict requirement
for l3fwd itself, but definitely nice to have feature that majority of DPDK
customers are interested in.
I do understand your desire to lower HW requirements for l3fwd, but
then probably shouldn't be just blind disable, but instead add SW
support for them when essential HW feature is missing. 

Konstantin
 
> virtio_dev_configure(): RSS support requested but not supported by
> the device
> Port0 dev_configure = -95
> 
> and:
> 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.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Trevor Tao <trevor.tao@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> Reviewed-by: Feifei Wang <feifei.wang2@arm.com>
> ---
>  .mailmap              |  1 +
>  examples/l3fwd/main.c | 19 ++++++++++++++++++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/.mailmap b/.mailmap
> index 8e3940a253..602d8cbc6b 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -1403,6 +1403,7 @@ Tom Rix <trix@redhat.com>
>  Tone Zhang <tone.zhang@arm.com>
>  Tonghao Zhang <xiangxia.m.yue@gmail.com> <nic@opencloud.tech>
>  Tony Nguyen <anthony.l.nguyen@intel.com>
> +Trevor Tao <trevor.tao@arm.com>
>  Tsotne Chakhvadze <tsotne.chakhvadze@intel.com>
>  Tudor Brindus <me@tbrindus.ca>
>  Tudor Cornea <tudor.cornea@gmail.com> <tudor.cornea@keysight.com>
> diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
> index a4f061537e..cec87d95d1 100644
> --- a/examples/l3fwd/main.c
> +++ b/examples/l3fwd/main.c
> @@ -1233,8 +1233,12 @@ 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)
> +		/* relax the rx rss requirement */
> +		if (dev_info.max_rx_queues == 1 || !local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
> +			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) {
> @@ -1245,6 +1249,19 @@ 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);
> +			local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
> +			port_conf.rxmode.offloads = local_port_conf.rxmode.offloads;
> +			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.41.0
> 

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

[Trevor Tao]

Now the port Rx mq_mode had been set to RTE_ETH_MQ_RX_RSS, and offload
mode set to RTE_ETH_RX_OFFLOAD_CHECKSUM by default, but some hardware
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 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:

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

and:
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.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Trevor Tao <trevor.tao@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
Reviewed-by: Feifei Wang <feifei.wang2@arm.com>
---
 .mailmap              |  1 +
 examples/l3fwd/main.c | 19 ++++++++++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/.mailmap b/.mailmap
index 8e3940a253..602d8cbc6b 100644
--- a/.mailmap
+++ b/.mailmap
@@ -1403,6 +1403,7 @@ Tom Rix <trix@redhat.com>
 Tone Zhang <tone.zhang@arm.com>
 Tonghao Zhang <xiangxia.m.yue@gmail.com> <nic@opencloud.tech>
 Tony Nguyen <anthony.l.nguyen@intel.com>
+Trevor Tao <trevor.tao@arm.com>
 Tsotne Chakhvadze <tsotne.chakhvadze@intel.com>
 Tudor Brindus <me@tbrindus.ca>
 Tudor Cornea <tudor.cornea@gmail.com> <tudor.cornea@keysight.com>
diff --git a/examples/l3fwd/main.c b/examples/l3fwd/main.c
index a4f061537e..cec87d95d1 100644
--- a/examples/l3fwd/main.c
+++ b/examples/l3fwd/main.c
@@ -1233,8 +1233,12 @@ 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)
+		/* relax the rx rss requirement */
+		if (dev_info.max_rx_queues == 1 || !local_port_conf.rx_adv_conf.rss_conf.rss_hf) {
+			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) {
@@ -1245,6 +1249,19 @@ 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);
+			local_port_conf.rxmode.offloads &= dev_info.rx_offload_capa;
+			port_conf.rxmode.offloads = local_port_conf.rxmode.offloads;
+			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.41.0