[PATCH v1 0/3] app/testpmd: fix the command to set tunnel TSO

[PATCH v1 0/3] app/testpmd: fix the command to set tunnel TSO

[Huisong Li]

Please see each patch in the series for detail.

Huisong Li (3):
  app/testpmd: fix random value to set tunnel TSO
  app/testpmd: add the explicit check for tunnel TSO offload
  app/testpmd: fix unnecessary change when set tunnel TSO

 app/test-pmd/cmdline.c | 102 ++++++++++++++++++++---------------------
 1 file changed, 50 insertions(+), 52 deletions(-)

-- 
2.33.0

[PATCH v1 1/3] app/testpmd: fix random value to set tunnel TSO

[Huisong Li]

Currently, testpmd set tunnel TSO offload even if fail to get dev_info.
In this case, the 'tx_offload_capa' in dev_info is a random value,

Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 app/test-pmd/cmdline.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 05078f8377..25462bdbfc 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -5035,39 +5035,33 @@ struct cmd_tunnel_tso_set_result {
 	portid_t port_id;
 };
 
-static struct rte_eth_dev_info
-check_tunnel_tso_nic_support(portid_t port_id)
+static void
+check_tunnel_tso_nic_support(portid_t port_id, uint64_t tx_offload_capa)
 {
-	struct rte_eth_dev_info dev_info;
-
-	if (eth_dev_info_get_print_err(port_id, &dev_info) != 0)
-		return dev_info;
-
-	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO))
+	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO))
 		fprintf(stderr,
 			"Warning: VXLAN TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
-	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO))
+	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO))
 		fprintf(stderr,
 			"Warning: GRE TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
-	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO))
+	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO))
 		fprintf(stderr,
 			"Warning: IPIP TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
-	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO))
+	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO))
 		fprintf(stderr,
 			"Warning: GENEVE TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
-	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IP_TNL_TSO))
+	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IP_TNL_TSO))
 		fprintf(stderr,
 			"Warning: IP TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
-	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO))
+	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO))
 		fprintf(stderr,
 			"Warning: UDP TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
-	return dev_info;
 }
 
 static void
@@ -5077,6 +5071,7 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
 {
 	struct cmd_tunnel_tso_set_result *res = parsed_result;
 	struct rte_eth_dev_info dev_info;
+	int ret;
 
 	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
 		return;
@@ -5088,7 +5083,11 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
 	if (!strcmp(res->mode, "set"))
 		ports[res->port_id].tunnel_tso_segsz = res->tso_segsz;
 
-	dev_info = check_tunnel_tso_nic_support(res->port_id);
+	ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
+	if (ret != 0)
+		return;
+
+	check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
 	if (ports[res->port_id].tunnel_tso_segsz == 0) {
 		ports[res->port_id].dev_conf.txmode.offloads &=
 			~(RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
-- 
2.33.0

[PATCH v1 2/3] app/testpmd: add the explicit check for tunnel TSO offload

[Huisong Li]

If port don't support TSO of tunnel packets, tell user in advance and
no need to change other configuration of this port in case of fault spread.

In addition, if some tunnel offloads don't support, which is not an error
case, the log about this shouldn't be to stderr.

Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 app/test-pmd/cmdline.c | 51 +++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 28 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 25462bdbfc..d3243d016b 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -5039,28 +5039,22 @@ static void
 check_tunnel_tso_nic_support(portid_t port_id, uint64_t tx_offload_capa)
 {
 	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO))
-		fprintf(stderr,
-			"Warning: VXLAN TUNNEL TSO not supported therefore not enabled for port %d\n",
+		printf("Warning: VXLAN TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
 	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO))
-		fprintf(stderr,
-			"Warning: GRE TUNNEL TSO not supported therefore not enabled for port %d\n",
+		printf("Warning: GRE TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
 	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO))
-		fprintf(stderr,
-			"Warning: IPIP TUNNEL TSO not supported therefore not enabled for port %d\n",
+		printf("Warning: IPIP TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
 	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO))
-		fprintf(stderr,
-			"Warning: GENEVE TUNNEL TSO not supported therefore not enabled for port %d\n",
+		printf("Warning: GENEVE TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
 	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IP_TNL_TSO))
-		fprintf(stderr,
-			"Warning: IP TUNNEL TSO not supported therefore not enabled for port %d\n",
+		printf("Warning: IP TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
 	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO))
-		fprintf(stderr,
-			"Warning: UDP TUNNEL TSO not supported therefore not enabled for port %d\n",
+		printf("Warning: UDP TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
 }
 
@@ -5071,6 +5065,12 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
 {
 	struct cmd_tunnel_tso_set_result *res = parsed_result;
 	struct rte_eth_dev_info dev_info;
+	uint64_t all_tunnel_tso = RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
+				RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
+				RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
+				RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
+				RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
+				RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO;
 	int ret;
 
 	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
@@ -5087,26 +5087,21 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
 	if (ret != 0)
 		return;
 
-	check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
+	if (ports[res->port_id].tunnel_tso_segsz != 0) {
+		if ((all_tunnel_tso & dev_info.tx_offload_capa) == 0) {
+			fprintf(stderr, "Error: port=%u don't support tunnel TSO offloads.\n",
+				res->port_id);
+			return;
+		}
+		check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
+	}
+
 	if (ports[res->port_id].tunnel_tso_segsz == 0) {
-		ports[res->port_id].dev_conf.txmode.offloads &=
-			~(RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
-			  RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
-			  RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
-			  RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
-			  RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
-			  RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO);
+		ports[res->port_id].dev_conf.txmode.offloads &= ~all_tunnel_tso;
 		printf("TSO for tunneled packets is disabled\n");
 	} else {
-		uint64_t tso_offloads = (RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
-					 RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
-					 RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
-					 RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
-					 RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
-					 RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO);
-
 		ports[res->port_id].dev_conf.txmode.offloads |=
-			(tso_offloads & dev_info.tx_offload_capa);
+			(all_tunnel_tso & dev_info.tx_offload_capa);
 		printf("TSO segment size for tunneled packets is %d\n",
 			ports[res->port_id].tunnel_tso_segsz);
 
-- 
2.33.0

[PATCH v1 3/3] app/testpmd: fix unnecessary change when set tunnel TSO

[Huisong Li]

Currently, there are two conditions to set tunnel TSO, like "parse tunnel"
and "outer IP checksum". If these conditions are not satisfied, testpmd
should not change their configuration, like tx_offloads on port and per
queue, and no need to request "reconfig device".

Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 app/test-pmd/cmdline.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index d3243d016b..33e66d1d93 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -5093,17 +5093,6 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
 				res->port_id);
 			return;
 		}
-		check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
-	}
-
-	if (ports[res->port_id].tunnel_tso_segsz == 0) {
-		ports[res->port_id].dev_conf.txmode.offloads &= ~all_tunnel_tso;
-		printf("TSO for tunneled packets is disabled\n");
-	} else {
-		ports[res->port_id].dev_conf.txmode.offloads |=
-			(all_tunnel_tso & dev_info.tx_offload_capa);
-		printf("TSO segment size for tunneled packets is %d\n",
-			ports[res->port_id].tunnel_tso_segsz);
 
 		/* Below conditions are needed to make it work:
 		 * (1) tunnel TSO is supported by the NIC;
@@ -5116,14 +5105,29 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
 		 * is not necessary for IPv6 tunneled pkts because there's no
 		 * checksum in IP header anymore.
 		 */
-
-		if (!ports[res->port_id].parse_tunnel)
+		if (!ports[res->port_id].parse_tunnel) {
 			fprintf(stderr,
-				"Warning: csum parse_tunnel must be set so that tunneled packets are recognized\n");
+				"Error: csum parse_tunnel must be set so that tunneled packets are recognized\n");
+			return;
+		}
 		if (!(ports[res->port_id].dev_conf.txmode.offloads &
-		      RTE_ETH_TX_OFFLOAD_OUTER_IPV4_CKSUM))
+		      RTE_ETH_TX_OFFLOAD_OUTER_IPV4_CKSUM)) {
 			fprintf(stderr,
-				"Warning: csum set outer-ip must be set to hw if outer L3 is IPv4; not necessary for IPv6\n");
+				"Error: csum set outer-ip must be set to hw if outer L3 is IPv4; not necessary for IPv6\n");
+			return;
+		}
+
+		check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
+	}
+
+	if (ports[res->port_id].tunnel_tso_segsz == 0) {
+		ports[res->port_id].dev_conf.txmode.offloads &= ~all_tunnel_tso;
+		printf("TSO for tunneled packets is disabled\n");
+	} else {
+		ports[res->port_id].dev_conf.txmode.offloads |=
+			(all_tunnel_tso & dev_info.tx_offload_capa);
+		printf("TSO segment size for tunneled packets is %d\n",
+			ports[res->port_id].tunnel_tso_segsz);
 	}
 
 	cmd_config_queue_tx_offloads(&ports[res->port_id]);
-- 
2.33.0

Re: [PATCH v1 1/3] app/testpmd: fix random value to set tunnel TSO

[Ivan Malov]

Hi,

(minor question below)

On Fri, 10 Nov 2023, Huisong Li wrote:

> Currently, testpmd set tunnel TSO offload even if fail to get dev_info.
> In this case, the 'tx_offload_capa' in dev_info is a random value,
>
> Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
> Cc: stable@dpdk.org
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
> app/test-pmd/cmdline.c | 29 ++++++++++++++---------------
> 1 file changed, 14 insertions(+), 15 deletions(-)
>
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 05078f8377..25462bdbfc 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -5035,39 +5035,33 @@ struct cmd_tunnel_tso_set_result {
> 	portid_t port_id;
> };
>
> -static struct rte_eth_dev_info
> -check_tunnel_tso_nic_support(portid_t port_id)
> +static void
> +check_tunnel_tso_nic_support(portid_t port_id, uint64_t tx_offload_capa)
> {
> -	struct rte_eth_dev_info dev_info;

Why not just initialise this to zeros?

> -
> -	if (eth_dev_info_get_print_err(port_id, &dev_info) != 0)
> -		return dev_info;
> -
> -	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO))
> +	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO))
> 		fprintf(stderr,
> 			"Warning: VXLAN TUNNEL TSO not supported therefore not enabled for port %d\n",
> 			port_id);
> -	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO))
> +	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO))
> 		fprintf(stderr,
> 			"Warning: GRE TUNNEL TSO not supported therefore not enabled for port %d\n",
> 			port_id);
> -	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO))
> +	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO))
> 		fprintf(stderr,
> 			"Warning: IPIP TUNNEL TSO not supported therefore not enabled for port %d\n",
> 			port_id);
> -	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO))
> +	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO))
> 		fprintf(stderr,
> 			"Warning: GENEVE TUNNEL TSO not supported therefore not enabled for port %d\n",
> 			port_id);
> -	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IP_TNL_TSO))
> +	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IP_TNL_TSO))
> 		fprintf(stderr,
> 			"Warning: IP TUNNEL TSO not supported therefore not enabled for port %d\n",
> 			port_id);
> -	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO))
> +	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO))
> 		fprintf(stderr,
> 			"Warning: UDP TUNNEL TSO not supported therefore not enabled for port %d\n",
> 			port_id);
> -	return dev_info;
> }
>
> static void
> @@ -5077,6 +5071,7 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
> {
> 	struct cmd_tunnel_tso_set_result *res = parsed_result;
> 	struct rte_eth_dev_info dev_info;
> +	int ret;
>
> 	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
> 		return;
> @@ -5088,7 +5083,11 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
> 	if (!strcmp(res->mode, "set"))
> 		ports[res->port_id].tunnel_tso_segsz = res->tso_segsz;
>
> -	dev_info = check_tunnel_tso_nic_support(res->port_id);
> +	ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
> +	if (ret != 0)
> +		return;
> +
> +	check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
> 	if (ports[res->port_id].tunnel_tso_segsz == 0) {
> 		ports[res->port_id].dev_conf.txmode.offloads &=
> 			~(RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
> -- 
> 2.33.0
>
>

Thank you.

Re: [PATCH v1 1/3] app/testpmd: fix random value to set tunnel TSO

[lihuisong (C)]

Hi,

Thanks for your review.


在 2023/11/10 19:42, Ivan Malov 写道:
> Hi,
>
> (minor question below)
>
> On Fri, 10 Nov 2023, Huisong Li wrote:
>
>> Currently, testpmd set tunnel TSO offload even if fail to get dev_info.
>> In this case, the 'tx_offload_capa' in dev_info is a random value,
>>
>> Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>> app/test-pmd/cmdline.c | 29 ++++++++++++++---------------
>> 1 file changed, 14 insertions(+), 15 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 05078f8377..25462bdbfc 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -5035,39 +5035,33 @@ struct cmd_tunnel_tso_set_result {
>>     portid_t port_id;
>> };
>>
>> -static struct rte_eth_dev_info
>> -check_tunnel_tso_nic_support(portid_t port_id)
>> +static void
>> +check_tunnel_tso_nic_support(portid_t port_id, uint64_t 
>> tx_offload_capa)
>> {
>> -    struct rte_eth_dev_info dev_info;
>
> Why not just initialise this to zeros?
The main purpose of this function is to check which tunnel TSO offload 
port supports instead of getting dev_info, right?
>
>> -
>> -    if (eth_dev_info_get_print_err(port_id, &dev_info) != 0)
>> -        return dev_info;
>> -
>> -    if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO))
>> +    if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO))
>>         fprintf(stderr,
>>             "Warning: VXLAN TUNNEL TSO not supported therefore not 
>> enabled for port %d\n",
>>             port_id);
>> -    if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO))
>> +    if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO))
>>         fprintf(stderr,
>>             "Warning: GRE TUNNEL TSO not supported therefore not 
>> enabled for port %d\n",
>>             port_id);
>> -    if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO))
>> +    if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO))
>>         fprintf(stderr,
>>             "Warning: IPIP TUNNEL TSO not supported therefore not 
>> enabled for port %d\n",
>>             port_id);
>> -    if (!(dev_info.tx_offload_capa & 
>> RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO))
>> +    if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO))
>>         fprintf(stderr,
>>             "Warning: GENEVE TUNNEL TSO not supported therefore not 
>> enabled for port %d\n",
>>             port_id);
>> -    if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IP_TNL_TSO))
>> +    if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IP_TNL_TSO))
>>         fprintf(stderr,
>>             "Warning: IP TUNNEL TSO not supported therefore not 
>> enabled for port %d\n",
>>             port_id);
>> -    if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO))
>> +    if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO))
>>         fprintf(stderr,
>>             "Warning: UDP TUNNEL TSO not supported therefore not 
>> enabled for port %d\n",
>>             port_id);
>> -    return dev_info;
>> }
>>
>> static void
>> @@ -5077,6 +5071,7 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
>> {
>>     struct cmd_tunnel_tso_set_result *res = parsed_result;
>>     struct rte_eth_dev_info dev_info;
>> +    int ret;
>>
>>     if (port_id_is_invalid(res->port_id, ENABLED_WARN))
>>         return;
>> @@ -5088,7 +5083,11 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
>>     if (!strcmp(res->mode, "set"))
>>         ports[res->port_id].tunnel_tso_segsz = res->tso_segsz;
>>
>> -    dev_info = check_tunnel_tso_nic_support(res->port_id);
>> +    ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
>> +    if (ret != 0)
>> +        return;
>> +
>> +    check_tunnel_tso_nic_support(res->port_id, 
>> dev_info.tx_offload_capa);
>>     if (ports[res->port_id].tunnel_tso_segsz == 0) {
>>         ports[res->port_id].dev_conf.txmode.offloads &=
>>             ~(RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
>> -- 
>> 2.33.0
>>
>>
>
> Thank you.
>
> .

Re: [PATCH v1 1/3] app/testpmd: fix random value to set tunnel TSO

[Ferruh Yigit]

On 11/11/2023 1:17 AM, lihuisong (C) wrote:
> Hi,
> 
> Thanks for your review.
> 
> 
> 在 2023/11/10 19:42, Ivan Malov 写道:
>> Hi,
>>
>> (minor question below)
>>
>> On Fri, 10 Nov 2023, Huisong Li wrote:
>>
>>> Currently, testpmd set tunnel TSO offload even if fail to get dev_info.
>>> In this case, the 'tx_offload_capa' in dev_info is a random value,
>>>
>>> Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>>> ---
>>> app/test-pmd/cmdline.c | 29 ++++++++++++++---------------
>>> 1 file changed, 14 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>>> index 05078f8377..25462bdbfc 100644
>>> --- a/app/test-pmd/cmdline.c
>>> +++ b/app/test-pmd/cmdline.c
>>> @@ -5035,39 +5035,33 @@ struct cmd_tunnel_tso_set_result {
>>>     portid_t port_id;
>>> };
>>>
>>> -static struct rte_eth_dev_info
>>> -check_tunnel_tso_nic_support(portid_t port_id)
>>> +static void
>>> +check_tunnel_tso_nic_support(portid_t port_id, uint64_t
>>> tx_offload_capa)
>>> {
>>> -    struct rte_eth_dev_info dev_info;
>>
>> Why not just initialise this to zeros?
> The main purpose of this function is to check which tunnel TSO offload
> port supports instead of getting dev_info, right?
>

Agree with Huisong, zeroing out the dev_info doesn't help much, this
function still will fail to do its job, and caller function will still
get wrong dev info.


>>
>>> -
>>> -    if (eth_dev_info_get_print_err(port_id, &dev_info) != 0)
>>> -        return dev_info;
>>> -
>>> -    if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO))
>>> +    if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO))
>>>         fprintf(stderr,
>>>             "Warning: VXLAN TUNNEL TSO not supported therefore not
>>> enabled for port %d\n",
>>>             port_id);
>>> -    if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO))
>>> +    if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO))
>>>         fprintf(stderr,
>>>             "Warning: GRE TUNNEL TSO not supported therefore not
>>> enabled for port %d\n",
>>>             port_id);
>>> -    if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO))
>>> +    if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO))
>>>         fprintf(stderr,
>>>             "Warning: IPIP TUNNEL TSO not supported therefore not
>>> enabled for port %d\n",
>>>             port_id);
>>> -    if (!(dev_info.tx_offload_capa &
>>> RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO))
>>> +    if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO))
>>>         fprintf(stderr,
>>>             "Warning: GENEVE TUNNEL TSO not supported therefore not
>>> enabled for port %d\n",
>>>             port_id);
>>> -    if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IP_TNL_TSO))
>>> +    if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IP_TNL_TSO))
>>>         fprintf(stderr,
>>>             "Warning: IP TUNNEL TSO not supported therefore not
>>> enabled for port %d\n",
>>>             port_id);
>>> -    if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO))
>>> +    if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO))
>>>         fprintf(stderr,
>>>             "Warning: UDP TUNNEL TSO not supported therefore not
>>> enabled for port %d\n",
>>>             port_id);
>>> -    return dev_info;
>>> }
>>>
>>> static void
>>> @@ -5077,6 +5071,7 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
>>> {
>>>     struct cmd_tunnel_tso_set_result *res = parsed_result;
>>>     struct rte_eth_dev_info dev_info;
>>> +    int ret;
>>>
>>>     if (port_id_is_invalid(res->port_id, ENABLED_WARN))
>>>         return;
>>> @@ -5088,7 +5083,11 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
>>>     if (!strcmp(res->mode, "set"))
>>>         ports[res->port_id].tunnel_tso_segsz = res->tso_segsz;
>>>
>>> -    dev_info = check_tunnel_tso_nic_support(res->port_id);
>>> +    ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
>>> +    if (ret != 0)
>>> +        return;
>>> +
>>> +    check_tunnel_tso_nic_support(res->port_id,
>>> dev_info.tx_offload_capa);
>>>     if (ports[res->port_id].tunnel_tso_segsz == 0) {
>>>         ports[res->port_id].dev_conf.txmode.offloads &=
>>>             ~(RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
>>> -- 
>>> 2.33.0
>>>
>>>
>>
>> Thank you.
>>
>> .

Re: [PATCH v1 2/3] app/testpmd: add the explicit check for tunnel TSO offload

[Ferruh Yigit]

On 11/10/2023 8:19 AM, Huisong Li wrote:
> If port don't support TSO of tunnel packets, tell user in advance and
> no need to change other configuration of this port in case of fault spread.
> 
> In addition, if some tunnel offloads don't support, which is not an error
> case, the log about this shouldn't be to stderr.
> 

Overall patch looks good to me, please check a minor comment below.

> Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  app/test-pmd/cmdline.c | 51 +++++++++++++++++++-----------------------
>  1 file changed, 23 insertions(+), 28 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 25462bdbfc..d3243d016b 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -5039,28 +5039,22 @@ static void
>  check_tunnel_tso_nic_support(portid_t port_id, uint64_t tx_offload_capa)
>  {
>  	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO))
> -		fprintf(stderr,
> -			"Warning: VXLAN TUNNEL TSO not supported therefore not enabled for port %d\n",
> +		printf("Warning: VXLAN TUNNEL TSO not supported therefore not enabled for port %d\n",
>  			port_id);
>  	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO))
> -		fprintf(stderr,
> -			"Warning: GRE TUNNEL TSO not supported therefore not enabled for port %d\n",
> +		printf("Warning: GRE TUNNEL TSO not supported therefore not enabled for port %d\n",
>  			port_id);
>  	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO))
> -		fprintf(stderr,
> -			"Warning: IPIP TUNNEL TSO not supported therefore not enabled for port %d\n",
> +		printf("Warning: IPIP TUNNEL TSO not supported therefore not enabled for port %d\n",
>  			port_id);
>  	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO))
> -		fprintf(stderr,
> -			"Warning: GENEVE TUNNEL TSO not supported therefore not enabled for port %d\n",
> +		printf("Warning: GENEVE TUNNEL TSO not supported therefore not enabled for port %d\n",
>  			port_id);
>  	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IP_TNL_TSO))
> -		fprintf(stderr,
> -			"Warning: IP TUNNEL TSO not supported therefore not enabled for port %d\n",
> +		printf("Warning: IP TUNNEL TSO not supported therefore not enabled for port %d\n",
>  			port_id);
>  	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO))
> -		fprintf(stderr,
> -			"Warning: UDP TUNNEL TSO not supported therefore not enabled for port %d\n",
> +		printf("Warning: UDP TUNNEL TSO not supported therefore not enabled for port %d\n",
>  			port_id);
>  }
>  
> @@ -5071,6 +5065,12 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
>  {
>  	struct cmd_tunnel_tso_set_result *res = parsed_result;
>  	struct rte_eth_dev_info dev_info;
> +	uint64_t all_tunnel_tso = RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
> +				RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
> +				RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
> +				RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
> +				RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
> +				RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO;
>  	int ret;
>  
>  	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
> @@ -5087,26 +5087,21 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
>  	if (ret != 0)
>  		return;
>  
> -	check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
> +	if (ports[res->port_id].tunnel_tso_segsz != 0) {
> +		if ((all_tunnel_tso & dev_info.tx_offload_capa) == 0) {
> +			fprintf(stderr, "Error: port=%u don't support tunnel TSO offloads.\n",
> +				res->port_id);
> +			return;
> +		}
> +		check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
> +	}
> +

Instead of having a separate if block, else leg of below if block does
same check [1], what do you think to move above block to there?

>  	if (ports[res->port_id].tunnel_tso_segsz == 0) {
> -		ports[res->port_id].dev_conf.txmode.offloads &=
> -			~(RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
> -			  RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
> -			  RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
> -			  RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
> -			  RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
> -			  RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO);
> +		ports[res->port_id].dev_conf.txmode.offloads &= ~all_tunnel_tso;
>  		printf("TSO for tunneled packets is disabled\n");
>  	} else {
> -		uint64_t tso_offloads = (RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
> -					 RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
> -					 RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
> -					 RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
> -					 RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
> -					 RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO);
> -

[1] ---> here has same logical condition

>  		ports[res->port_id].dev_conf.txmode.offloads |=
> -			(tso_offloads & dev_info.tx_offload_capa);
> +			(all_tunnel_tso & dev_info.tx_offload_capa);
>  		printf("TSO segment size for tunneled packets is %d\n",
>  			ports[res->port_id].tunnel_tso_segsz);
>  

Re: [PATCH v1 3/3] app/testpmd: fix unnecessary change when set tunnel TSO

[Ferruh Yigit]

On 11/10/2023 8:19 AM, Huisong Li wrote:
> Currently, there are two conditions to set tunnel TSO, like "parse tunnel"
> and "outer IP checksum". If these conditions are not satisfied, testpmd
> should not change their configuration, like tx_offloads on port and per
> queue, and no need to request "reconfig device".
> 
> Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> ---
>  app/test-pmd/cmdline.c | 36 ++++++++++++++++++++----------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index d3243d016b..33e66d1d93 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -5093,17 +5093,6 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
>  				res->port_id);
>  			return;
>  		}
> -		check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
> -	}
> -
> -	if (ports[res->port_id].tunnel_tso_segsz == 0) {
> -		ports[res->port_id].dev_conf.txmode.offloads &= ~all_tunnel_tso;
> -		printf("TSO for tunneled packets is disabled\n");
> -	} else {
> -		ports[res->port_id].dev_conf.txmode.offloads |=
> -			(all_tunnel_tso & dev_info.tx_offload_capa);
> -		printf("TSO segment size for tunneled packets is %d\n",
> -			ports[res->port_id].tunnel_tso_segsz);
>  
>  		/* Below conditions are needed to make it work:
>  		 * (1) tunnel TSO is supported by the NIC;
> @@ -5116,14 +5105,29 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
>  		 * is not necessary for IPv6 tunneled pkts because there's no
>  		 * checksum in IP header anymore.
>  		 */
> -
> -		if (!ports[res->port_id].parse_tunnel)
> +		if (!ports[res->port_id].parse_tunnel) {
>  			fprintf(stderr,
> -				"Warning: csum parse_tunnel must be set so that tunneled packets are recognized\n");
> +				"Error: csum parse_tunnel must be set so that tunneled packets are recognized\n");
> +			return;
> +		}
>  		if (!(ports[res->port_id].dev_conf.txmode.offloads &
> -		      RTE_ETH_TX_OFFLOAD_OUTER_IPV4_CKSUM))
> +		      RTE_ETH_TX_OFFLOAD_OUTER_IPV4_CKSUM)) {
>  			fprintf(stderr,
> -				"Warning: csum set outer-ip must be set to hw if outer L3 is IPv4; not necessary for IPv6\n");
> +				"Error: csum set outer-ip must be set to hw if outer L3 is IPv4; not necessary for IPv6\n");
> +			return;
> +		}
> +
> +		check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
> +	}
> +
> +	if (ports[res->port_id].tunnel_tso_segsz == 0) {
> +		ports[res->port_id].dev_conf.txmode.offloads &= ~all_tunnel_tso;
> +		printf("TSO for tunneled packets is disabled\n");
> +	} else {
> +		ports[res->port_id].dev_conf.txmode.offloads |=
> +			(all_tunnel_tso & dev_info.tx_offload_capa);
> +		printf("TSO segment size for tunneled packets is %d\n",
> +			ports[res->port_id].tunnel_tso_segsz);
>

Similar comment with previous patch, why having separate if block for
same check, above block can be merged to it.

Re: [PATCH v1 2/3] app/testpmd: add the explicit check for tunnel TSO offload

[lihuisong (C)]

Hi Ferruh,

Thanks for you review so fast.

在 2023/11/11 11:30, Ferruh Yigit 写道:
> On 11/10/2023 8:19 AM, Huisong Li wrote:
>> If port don't support TSO of tunnel packets, tell user in advance and
>> no need to change other configuration of this port in case of fault spread.
>>
>> In addition, if some tunnel offloads don't support, which is not an error
>> case, the log about this shouldn't be to stderr.
>>
> Overall patch looks good to me, please check a minor comment below.
>
>> Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   app/test-pmd/cmdline.c | 51 +++++++++++++++++++-----------------------
>>   1 file changed, 23 insertions(+), 28 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index 25462bdbfc..d3243d016b 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -5039,28 +5039,22 @@ static void
>>   check_tunnel_tso_nic_support(portid_t port_id, uint64_t tx_offload_capa)
>>   {
>>   	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO))
>> -		fprintf(stderr,
>> -			"Warning: VXLAN TUNNEL TSO not supported therefore not enabled for port %d\n",
>> +		printf("Warning: VXLAN TUNNEL TSO not supported therefore not enabled for port %d\n",
>>   			port_id);
>>   	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO))
>> -		fprintf(stderr,
>> -			"Warning: GRE TUNNEL TSO not supported therefore not enabled for port %d\n",
>> +		printf("Warning: GRE TUNNEL TSO not supported therefore not enabled for port %d\n",
>>   			port_id);
>>   	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO))
>> -		fprintf(stderr,
>> -			"Warning: IPIP TUNNEL TSO not supported therefore not enabled for port %d\n",
>> +		printf("Warning: IPIP TUNNEL TSO not supported therefore not enabled for port %d\n",
>>   			port_id);
>>   	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO))
>> -		fprintf(stderr,
>> -			"Warning: GENEVE TUNNEL TSO not supported therefore not enabled for port %d\n",
>> +		printf("Warning: GENEVE TUNNEL TSO not supported therefore not enabled for port %d\n",
>>   			port_id);
>>   	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IP_TNL_TSO))
>> -		fprintf(stderr,
>> -			"Warning: IP TUNNEL TSO not supported therefore not enabled for port %d\n",
>> +		printf("Warning: IP TUNNEL TSO not supported therefore not enabled for port %d\n",
>>   			port_id);
>>   	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO))
>> -		fprintf(stderr,
>> -			"Warning: UDP TUNNEL TSO not supported therefore not enabled for port %d\n",
>> +		printf("Warning: UDP TUNNEL TSO not supported therefore not enabled for port %d\n",
>>   			port_id);
>>   }
>>   
>> @@ -5071,6 +5065,12 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
>>   {
>>   	struct cmd_tunnel_tso_set_result *res = parsed_result;
>>   	struct rte_eth_dev_info dev_info;
>> +	uint64_t all_tunnel_tso = RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
>> +				RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
>> +				RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
>> +				RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
>> +				RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
>> +				RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO;
>>   	int ret;
>>   
>>   	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
>> @@ -5087,26 +5087,21 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
>>   	if (ret != 0)
>>   		return;
>>   
>> -	check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
>> +	if (ports[res->port_id].tunnel_tso_segsz != 0) {
>> +		if ((all_tunnel_tso & dev_info.tx_offload_capa) == 0) {
>> +			fprintf(stderr, "Error: port=%u don't support tunnel TSO offloads.\n",
>> +				res->port_id);
>> +			return;
>> +		}
>> +		check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
>> +	}
>> +
> Instead of having a separate if block, else leg of below if block does
> same check [1], what do you think to move above block to there?
ok, will fixed in next version.
>
>>   	if (ports[res->port_id].tunnel_tso_segsz == 0) {
>> -		ports[res->port_id].dev_conf.txmode.offloads &=
>> -			~(RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
>> -			  RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
>> -			  RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
>> -			  RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
>> -			  RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
>> -			  RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO);
>> +		ports[res->port_id].dev_conf.txmode.offloads &= ~all_tunnel_tso;
>>   		printf("TSO for tunneled packets is disabled\n");
>>   	} else {
>> -		uint64_t tso_offloads = (RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
>> -					 RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
>> -					 RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
>> -					 RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
>> -					 RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
>> -					 RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO);
>> -
> [1] ---> here has same logical condition
>
>>   		ports[res->port_id].dev_conf.txmode.offloads |=
>> -			(tso_offloads & dev_info.tx_offload_capa);
>> +			(all_tunnel_tso & dev_info.tx_offload_capa);
>>   		printf("TSO segment size for tunneled packets is %d\n",
>>   			ports[res->port_id].tunnel_tso_segsz);
>>   
> .

Re: [PATCH v1 3/3] app/testpmd: fix unnecessary change when set tunnel TSO

[lihuisong (C)]

在 2023/11/11 11:37, Ferruh Yigit 写道:
> On 11/10/2023 8:19 AM, Huisong Li wrote:
>> Currently, there are two conditions to set tunnel TSO, like "parse tunnel"
>> and "outer IP checksum". If these conditions are not satisfied, testpmd
>> should not change their configuration, like tx_offloads on port and per
>> queue, and no need to request "reconfig device".
>>
>> Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> ---
>>   app/test-pmd/cmdline.c | 36 ++++++++++++++++++++----------------
>>   1 file changed, 20 insertions(+), 16 deletions(-)
>>
>> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
>> index d3243d016b..33e66d1d93 100644
>> --- a/app/test-pmd/cmdline.c
>> +++ b/app/test-pmd/cmdline.c
>> @@ -5093,17 +5093,6 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
>>   				res->port_id);
>>   			return;
>>   		}
>> -		check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
>> -	}
>> -
>> -	if (ports[res->port_id].tunnel_tso_segsz == 0) {
>> -		ports[res->port_id].dev_conf.txmode.offloads &= ~all_tunnel_tso;
>> -		printf("TSO for tunneled packets is disabled\n");
>> -	} else {
>> -		ports[res->port_id].dev_conf.txmode.offloads |=
>> -			(all_tunnel_tso & dev_info.tx_offload_capa);
>> -		printf("TSO segment size for tunneled packets is %d\n",
>> -			ports[res->port_id].tunnel_tso_segsz);
>>   
>>   		/* Below conditions are needed to make it work:
>>   		 * (1) tunnel TSO is supported by the NIC;
>> @@ -5116,14 +5105,29 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
>>   		 * is not necessary for IPv6 tunneled pkts because there's no
>>   		 * checksum in IP header anymore.
>>   		 */
>> -
>> -		if (!ports[res->port_id].parse_tunnel)
>> +		if (!ports[res->port_id].parse_tunnel) {
>>   			fprintf(stderr,
>> -				"Warning: csum parse_tunnel must be set so that tunneled packets are recognized\n");
>> +				"Error: csum parse_tunnel must be set so that tunneled packets are recognized\n");
>> +			return;
>> +		}
>>   		if (!(ports[res->port_id].dev_conf.txmode.offloads &
>> -		      RTE_ETH_TX_OFFLOAD_OUTER_IPV4_CKSUM))
>> +		      RTE_ETH_TX_OFFLOAD_OUTER_IPV4_CKSUM)) {
>>   			fprintf(stderr,
>> -				"Warning: csum set outer-ip must be set to hw if outer L3 is IPv4; not necessary for IPv6\n");
>> +				"Error: csum set outer-ip must be set to hw if outer L3 is IPv4; not necessary for IPv6\n");
>> +			return;
>> +		}
>> +
>> +		check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
>> +	}
>> +
>> +	if (ports[res->port_id].tunnel_tso_segsz == 0) {
>> +		ports[res->port_id].dev_conf.txmode.offloads &= ~all_tunnel_tso;
>> +		printf("TSO for tunneled packets is disabled\n");
>> +	} else {
>> +		ports[res->port_id].dev_conf.txmode.offloads |=
>> +			(all_tunnel_tso & dev_info.tx_offload_capa);
>> +		printf("TSO segment size for tunneled packets is %d\n",
>> +			ports[res->port_id].tunnel_tso_segsz);
>>
> Similar comment with previous patch, why having separate if block for
> same check, above block can be merged to it.
Ack
>
> .

[PATCH v2 0/3] app/testpmd: fix the command to set tunnel TSO

[Huisong Li]

Please see each patch in the series for detail.

---
 -v2: fix the position of check code based on Ferruh's comments

Huisong Li (3):
  app/testpmd: fix random value to set tunnel TSO
  app/testpmd: add the explicit check for tunnel TSO offload
  app/testpmd: fix unnecessary change when set tunnel TSO

 app/test-pmd/cmdline.c | 93 ++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 49 deletions(-)

-- 
2.33.0

[PATCH v2 1/3] app/testpmd: fix random value to set tunnel TSO

[Huisong Li]

Currently, testpmd set tunnel TSO offload even if fail to get dev_info.
In this case, the 'tx_offload_capa' in dev_info is a random value,

Fixes: 6f51deb903b2 ("app/testpmd: check status of getting ethdev info")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 app/test-pmd/cmdline.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 05078f8377..25462bdbfc 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -5035,39 +5035,33 @@ struct cmd_tunnel_tso_set_result {
 	portid_t port_id;
 };
 
-static struct rte_eth_dev_info
-check_tunnel_tso_nic_support(portid_t port_id)
+static void
+check_tunnel_tso_nic_support(portid_t port_id, uint64_t tx_offload_capa)
 {
-	struct rte_eth_dev_info dev_info;
-
-	if (eth_dev_info_get_print_err(port_id, &dev_info) != 0)
-		return dev_info;
-
-	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO))
+	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO))
 		fprintf(stderr,
 			"Warning: VXLAN TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
-	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO))
+	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO))
 		fprintf(stderr,
 			"Warning: GRE TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
-	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO))
+	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO))
 		fprintf(stderr,
 			"Warning: IPIP TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
-	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO))
+	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO))
 		fprintf(stderr,
 			"Warning: GENEVE TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
-	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_IP_TNL_TSO))
+	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IP_TNL_TSO))
 		fprintf(stderr,
 			"Warning: IP TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
-	if (!(dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO))
+	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO))
 		fprintf(stderr,
 			"Warning: UDP TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
-	return dev_info;
 }
 
 static void
@@ -5077,6 +5071,7 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
 {
 	struct cmd_tunnel_tso_set_result *res = parsed_result;
 	struct rte_eth_dev_info dev_info;
+	int ret;
 
 	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
 		return;
@@ -5088,7 +5083,11 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
 	if (!strcmp(res->mode, "set"))
 		ports[res->port_id].tunnel_tso_segsz = res->tso_segsz;
 
-	dev_info = check_tunnel_tso_nic_support(res->port_id);
+	ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
+	if (ret != 0)
+		return;
+
+	check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
 	if (ports[res->port_id].tunnel_tso_segsz == 0) {
 		ports[res->port_id].dev_conf.txmode.offloads &=
 			~(RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
-- 
2.33.0

[PATCH v2 2/3] app/testpmd: add the explicit check for tunnel TSO offload

[Huisong Li]

If port don't support TSO of tunnel packets, tell user in advance and
no need to change other configuration of this port in case of fault spread.

In addition, if some tunnel offloads don't support, which is not an error
case, the log about this shouldn't be to stderr.

Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 app/test-pmd/cmdline.c | 55 ++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 31 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 25462bdbfc..7a85cb0024 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -5039,28 +5039,22 @@ static void
 check_tunnel_tso_nic_support(portid_t port_id, uint64_t tx_offload_capa)
 {
 	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO))
-		fprintf(stderr,
-			"Warning: VXLAN TUNNEL TSO not supported therefore not enabled for port %d\n",
+		printf("Warning: VXLAN TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
 	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO))
-		fprintf(stderr,
-			"Warning: GRE TUNNEL TSO not supported therefore not enabled for port %d\n",
+		printf("Warning: GRE TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
 	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO))
-		fprintf(stderr,
-			"Warning: IPIP TUNNEL TSO not supported therefore not enabled for port %d\n",
+		printf("Warning: IPIP TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
 	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO))
-		fprintf(stderr,
-			"Warning: GENEVE TUNNEL TSO not supported therefore not enabled for port %d\n",
+		printf("Warning: GENEVE TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
 	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_IP_TNL_TSO))
-		fprintf(stderr,
-			"Warning: IP TUNNEL TSO not supported therefore not enabled for port %d\n",
+		printf("Warning: IP TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
 	if (!(tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO))
-		fprintf(stderr,
-			"Warning: UDP TUNNEL TSO not supported therefore not enabled for port %d\n",
+		printf("Warning: UDP TUNNEL TSO not supported therefore not enabled for port %d\n",
 			port_id);
 }
 
@@ -5071,6 +5065,12 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
 {
 	struct cmd_tunnel_tso_set_result *res = parsed_result;
 	struct rte_eth_dev_info dev_info;
+	uint64_t all_tunnel_tso = RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
+				RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
+				RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
+				RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
+				RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
+				RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO;
 	int ret;
 
 	if (port_id_is_invalid(res->port_id, ENABLED_WARN))
@@ -5083,30 +5083,23 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
 	if (!strcmp(res->mode, "set"))
 		ports[res->port_id].tunnel_tso_segsz = res->tso_segsz;
 
-	ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
-	if (ret != 0)
-		return;
-
-	check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
 	if (ports[res->port_id].tunnel_tso_segsz == 0) {
-		ports[res->port_id].dev_conf.txmode.offloads &=
-			~(RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
-			  RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
-			  RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
-			  RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
-			  RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
-			  RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO);
+		ports[res->port_id].dev_conf.txmode.offloads &= ~all_tunnel_tso;
 		printf("TSO for tunneled packets is disabled\n");
 	} else {
-		uint64_t tso_offloads = (RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO |
-					 RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO |
-					 RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO |
-					 RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO |
-					 RTE_ETH_TX_OFFLOAD_IP_TNL_TSO |
-					 RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO);
+		ret = eth_dev_info_get_print_err(res->port_id, &dev_info);
+		if (ret != 0)
+			return;
+
+		if ((all_tunnel_tso & dev_info.tx_offload_capa) == 0) {
+			fprintf(stderr, "Error: port=%u don't support tunnel TSO offloads.\n",
+				res->port_id);
+			return;
+		}
+		check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
 
 		ports[res->port_id].dev_conf.txmode.offloads |=
-			(tso_offloads & dev_info.tx_offload_capa);
+			(all_tunnel_tso & dev_info.tx_offload_capa);
 		printf("TSO segment size for tunneled packets is %d\n",
 			ports[res->port_id].tunnel_tso_segsz);
 
-- 
2.33.0

[PATCH v2 3/3] app/testpmd: fix unnecessary change when set tunnel TSO

[Huisong Li]

Currently, there are two conditions to set tunnel TSO, like "parse tunnel"
and "outer IP checksum". If these conditions are not satisfied, testpmd
should not change their configuration, like tx_offloads on port and per
queue, and no need to request "reconfig device".

Fixes: 597f9fafe13b ("app/testpmd: convert to new Tx offloads API")
Cc: stable@dpdk.org

Signed-off-by: Huisong Li <lihuisong@huawei.com>
---
 app/test-pmd/cmdline.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 7a85cb0024..a37fbe07c2 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -5096,12 +5096,6 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
 				res->port_id);
 			return;
 		}
-		check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
-
-		ports[res->port_id].dev_conf.txmode.offloads |=
-			(all_tunnel_tso & dev_info.tx_offload_capa);
-		printf("TSO segment size for tunneled packets is %d\n",
-			ports[res->port_id].tunnel_tso_segsz);
 
 		/* Below conditions are needed to make it work:
 		 * (1) tunnel TSO is supported by the NIC;
@@ -5114,14 +5108,23 @@ cmd_tunnel_tso_set_parsed(void *parsed_result,
 		 * is not necessary for IPv6 tunneled pkts because there's no
 		 * checksum in IP header anymore.
 		 */
-
-		if (!ports[res->port_id].parse_tunnel)
+		if (!ports[res->port_id].parse_tunnel) {
 			fprintf(stderr,
-				"Warning: csum parse_tunnel must be set so that tunneled packets are recognized\n");
+				"Error: csum parse_tunnel must be set so that tunneled packets are recognized\n");
+			return;
+		}
 		if (!(ports[res->port_id].dev_conf.txmode.offloads &
-		      RTE_ETH_TX_OFFLOAD_OUTER_IPV4_CKSUM))
+		      RTE_ETH_TX_OFFLOAD_OUTER_IPV4_CKSUM)) {
 			fprintf(stderr,
-				"Warning: csum set outer-ip must be set to hw if outer L3 is IPv4; not necessary for IPv6\n");
+				"Error: csum set outer-ip must be set to hw if outer L3 is IPv4; not necessary for IPv6\n");
+			return;
+		}
+
+		check_tunnel_tso_nic_support(res->port_id, dev_info.tx_offload_capa);
+		ports[res->port_id].dev_conf.txmode.offloads |=
+				(all_tunnel_tso & dev_info.tx_offload_capa);
+		printf("TSO segment size for tunneled packets is %d\n",
+			ports[res->port_id].tunnel_tso_segsz);
 	}
 
 	cmd_config_queue_tx_offloads(&ports[res->port_id]);
-- 
2.33.0

Re: [PATCH v2 0/3] app/testpmd: fix the command to set tunnel TSO

[Ferruh Yigit]

On 11/11/2023 4:59 AM, Huisong Li wrote:
> Please see each patch in the series for detail.
> 
> ---
>  -v2: fix the position of check code based on Ferruh's comments
> 
> Huisong Li (3):
>   app/testpmd: fix random value to set tunnel TSO
>   app/testpmd: add the explicit check for tunnel TSO offload
>   app/testpmd: fix unnecessary change when set tunnel TSO
> 

For series,
Reviewed-by: Ferruh Yigit <ferruh.yigit@amd.com>

Series applied to dpdk-next-net/main, thanks.