DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] app/testpmd: fix error detection in MTU command
@ 2020-05-18  9:27 Shy Shyman
  2020-05-19 11:10 ` Asaf Penso
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Shy Shyman @ 2020-05-18  9:27 UTC (permalink / raw)
  To: dev; +Cc: Wenzhuo Lu, Beilei Xing, Bernard Iremonger, xavier.huwei

MTU is used in testpmd to set the maximum payload size for packets.
According to testpmd, the setting influnce RX only.
In rte_ethdev there's no relation between MTU setting and JUMBO offload or
rx_max_pkt_len.

The previous fix in patch referenced below was meant to update the
correlated variables of max_pkt_len and JUMBO offload, but by doing
so it assumes that MTU setting can only exist when JUMBO offload
supported in the device. For example fail-safe device does supports set MTU
and doesn't support JUMBO offload, and in this case, though set MTU succeed
an error mesage still printed  since the JUMBO packet offload is disabled.

The fix separates the two conditions to make sure the error
triggers only in case the set_mtu action actually failed.
A warning message is provided in this special case to alert the user.

Fixes: 150c9ac2df13 ("app/testpmd: update Rx offload after setting MTU")
Cc: xavier.huwei@huawei.com

Signed-off-by: Shy Shyman <shys@mellanox.com>
---
 app/test-pmd/config.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 5381207cc..73b53c50b 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1277,8 +1277,9 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
 		return;
 	}
 	diag = rte_eth_dev_set_mtu(port_id, mtu);
-	if (diag == 0 &&
-	    dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) {
+	if (diag)
+		printf("Set MTU failed. diag=%d\n", diag);
+	else if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) {
 		/*
 		 * Ether overhead in driver is equal to the difference of
 		 * max_rx_pktlen and max_mtu in rte_eth_dev_info when the
@@ -1293,10 +1294,9 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
 		} else
 			rte_port->dev_conf.rxmode.offloads &=
 						~DEV_RX_OFFLOAD_JUMBO_FRAME;
-
-		return;
-	}
-	printf("Set MTU failed. diag=%d\n", diag);
+	} else
+		printf("WARNING: MTU was set while jumbo frame offload is"
+			" not supported by the device\n");
 }
 
 /* Generic flow management functions. */
-- 
2.21.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: fix error detection in MTU command
  2020-05-18  9:27 [dpdk-dev] [PATCH] app/testpmd: fix error detection in MTU command Shy Shyman
@ 2020-05-19 11:10 ` Asaf Penso
  2020-05-19 14:15 ` Iremonger, Bernard
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Asaf Penso @ 2020-05-19 11:10 UTC (permalink / raw)
  To: Shy Shyman, dev, Ferruh Yigit, Matan Azrad
  Cc: Wenzhuo Lu, Beilei Xing, Bernard Iremonger, xavier.huwei



Regards,
Asaf Penso

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Shy Shyman
> Sent: Monday, May 18, 2020 12:27 PM
> To: dev@dpdk.org
> Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>; Beilei Xing
> <beilei.xing@intel.com>; Bernard Iremonger
> <bernard.iremonger@intel.com>; xavier.huwei@huawei.com
> Subject: [dpdk-dev] [PATCH] app/testpmd: fix error detection in MTU
> command
> 
> MTU is used in testpmd to set the maximum payload size for packets.
> According to testpmd, the setting influnce RX only.
> In rte_ethdev there's no relation between MTU setting and JUMBO offload
> or
> rx_max_pkt_len.
> 
> The previous fix in patch referenced below was meant to update the
> correlated variables of max_pkt_len and JUMBO offload, but by doing
> so it assumes that MTU setting can only exist when JUMBO offload
> supported in the device. For example fail-safe device does supports set MTU
> and doesn't support JUMBO offload, and in this case, though set MTU
> succeed
> an error mesage still printed  since the JUMBO packet offload is disabled.
> 
> The fix separates the two conditions to make sure the error
> triggers only in case the set_mtu action actually failed.
> A warning message is provided in this special case to alert the user.
> 
> Fixes: 150c9ac2df13 ("app/testpmd: update Rx offload after setting MTU")
> Cc: xavier.huwei@huawei.com
> 
> Signed-off-by: Shy Shyman <shys@mellanox.com>
Reviewed-by: Asaf Penso <asafp@mellanox.com>

> ---
>  app/test-pmd/config.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 5381207cc..73b53c50b 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1277,8 +1277,9 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
>  		return;
>  	}
>  	diag = rte_eth_dev_set_mtu(port_id, mtu);
> -	if (diag == 0 &&
> -	    dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) {
> +	if (diag)
> +		printf("Set MTU failed. diag=%d\n", diag);
> +	else if (dev_info.rx_offload_capa &
> DEV_RX_OFFLOAD_JUMBO_FRAME) {
>  		/*
>  		 * Ether overhead in driver is equal to the difference of
>  		 * max_rx_pktlen and max_mtu in rte_eth_dev_info when
> the
> @@ -1293,10 +1294,9 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
>  		} else
>  			rte_port->dev_conf.rxmode.offloads &=
> 
> 	~DEV_RX_OFFLOAD_JUMBO_FRAME;
> -
> -		return;
> -	}
> -	printf("Set MTU failed. diag=%d\n", diag);
> +	} else
> +		printf("WARNING: MTU was set while jumbo frame offload
> is"
> +			" not supported by the device\n");
>  }
> 
>  /* Generic flow management functions. */
> --
> 2.21.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: fix error detection in MTU command
  2020-05-18  9:27 [dpdk-dev] [PATCH] app/testpmd: fix error detection in MTU command Shy Shyman
  2020-05-19 11:10 ` Asaf Penso
@ 2020-05-19 14:15 ` Iremonger, Bernard
  2020-05-19 14:57 ` Ferruh Yigit
  2020-06-08 14:17 ` [dpdk-dev] [PATCH v2] " Shy Shyman
  3 siblings, 0 replies; 10+ messages in thread
From: Iremonger, Bernard @ 2020-05-19 14:15 UTC (permalink / raw)
  To: Shy Shyman, dev; +Cc: Lu, Wenzhuo, Xing, Beilei, xavier.huwei

Hi Shy

> -----Original Message-----
> From: Shy Shyman <shys@mellanox.com>
> Sent: Monday, May 18, 2020 10:27 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; xavier.huwei@huawei.com
> Subject: [PATCH] app/testpmd: fix error detection in MTU command
> 
> MTU is used in testpmd to set the maximum payload size for packets.
> According to testpmd, the setting influnce RX only.
> In rte_ethdev there's no relation between MTU setting and JUMBO offload
> or rx_max_pkt_len.
> 
> The previous fix in patch referenced below was meant to update the
> correlated variables of max_pkt_len and JUMBO offload, but by doing so it
> assumes that MTU setting can only exist when JUMBO offload supported in
> the device. For example fail-safe device does supports set MTU and doesn't

Typo: supports should be support

> support JUMBO offload, and in this case, though set MTU succeed an error
> mesage still printed  since the JUMBO packet offload is disabled.

Typo: mesage  should be message

> The fix separates the two conditions to make sure the error triggers  
> A warning message is provided in this special case to alert the user.
> 
> Fixes: 150c9ac2df13 ("app/testpmd: update Rx offload after setting MTU")
> Cc: xavier.huwei@huawei.com

Should the patch be backported to 20.02 ?(ie add Cc: stable@dpdk.org)

> 
> Signed-off-by: Shy Shyman <shys@mellanox.com>
> ---
>  app/test-pmd/config.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index
> 5381207cc..73b53c50b 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1277,8 +1277,9 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
>  		return;
>  	}
>  	diag = rte_eth_dev_set_mtu(port_id, mtu);
> -	if (diag == 0 &&
> -	    dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) {
> +	if (diag)
> +		printf("Set MTU failed. diag=%d\n", diag);
> +	else if (dev_info.rx_offload_capa &
> DEV_RX_OFFLOAD_JUMBO_FRAME) {
>  		/*
>  		 * Ether overhead in driver is equal to the difference of
>  		 * max_rx_pktlen and max_mtu in rte_eth_dev_info when
> the @@ -1293,10 +1294,9 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
>  		} else
>  			rte_port->dev_conf.rxmode.offloads &=
> 
> 	~DEV_RX_OFFLOAD_JUMBO_FRAME;
> -
> -		return;
> -	}
> -	printf("Set MTU failed. diag=%d\n", diag);
> +	} else
> +		printf("WARNING: MTU was set while jumbo frame offload
> is"
> +			" not supported by the device\n");
>  }
> 
>  /* Generic flow management functions. */
> --
> 2.21.0

Otherwise 

Acked-by: Bernard Iremonger <bernard.iremonger@intel.com>


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: fix error detection in MTU command
  2020-05-18  9:27 [dpdk-dev] [PATCH] app/testpmd: fix error detection in MTU command Shy Shyman
  2020-05-19 11:10 ` Asaf Penso
  2020-05-19 14:15 ` Iremonger, Bernard
@ 2020-05-19 14:57 ` Ferruh Yigit
  2020-05-19 16:40   ` Shy Shyman
  2020-06-08 14:17 ` [dpdk-dev] [PATCH v2] " Shy Shyman
  3 siblings, 1 reply; 10+ messages in thread
From: Ferruh Yigit @ 2020-05-19 14:57 UTC (permalink / raw)
  To: Shy Shyman, dev; +Cc: Wenzhuo Lu, Beilei Xing, Bernard Iremonger, xavier.huwei

On 5/18/2020 10:27 AM, Shy Shyman wrote:
> MTU is used in testpmd to set the maximum payload size for packets.
> According to testpmd, the setting influnce RX only.
> In rte_ethdev there's no relation between MTU setting and JUMBO offload or
> rx_max_pkt_len.
> 
> The previous fix in patch referenced below was meant to update the
> correlated variables of max_pkt_len and JUMBO offload, but by doing
> so it assumes that MTU setting can only exist when JUMBO offload
> supported in the device. 

+1 and this is wrong, as far as I understand this is not the intention of the
previous fix.


> For example fail-safe device does supports set MTU
> and doesn't support JUMBO offload, and in this case, though set MTU succeed
> an error mesage still printed  since the JUMBO packet offload is disabled.
> 
> The fix separates the two conditions to make sure the error
> triggers only in case the set_mtu action actually failed.
> A warning message is provided in this special case to alert the user.

Not sure if this warning is required at all.
As you said above intention is to based on MTU value to correlate testpmd
(application) max_pkt_len and JUMBO offload values.

What about following change:

.....
	diag = rte_eth_dev_set_mtu(port_id, mtu);
	if (diag) {
		printf("Set MTU failed. diag=%d\n", diag);
		return;
	}

	if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) {
		....
		....
		....
	}
}


> 
> Fixes: 150c9ac2df13 ("app/testpmd: update Rx offload after setting MTU")
> Cc: xavier.huwei@huawei.com
> 
> Signed-off-by: Shy Shyman <shys@mellanox.com>
> ---
>  app/test-pmd/config.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
> index 5381207cc..73b53c50b 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1277,8 +1277,9 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
>  		return;
>  	}
>  	diag = rte_eth_dev_set_mtu(port_id, mtu);
> -	if (diag == 0 &&
> -	    dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) {
> +	if (diag)
> +		printf("Set MTU failed. diag=%d\n", diag);
> +	else if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) {
>  		/*
>  		 * Ether overhead in driver is equal to the difference of
>  		 * max_rx_pktlen and max_mtu in rte_eth_dev_info when the
> @@ -1293,10 +1294,9 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
>  		} else
>  			rte_port->dev_conf.rxmode.offloads &=
>  						~DEV_RX_OFFLOAD_JUMBO_FRAME;
> -
> -		return;
> -	}
> -	printf("Set MTU failed. diag=%d\n", diag);
> +	} else
> +		printf("WARNING: MTU was set while jumbo frame offload is"
> +			" not supported by the device\n");
>  }
>  
>  /* Generic flow management functions. */
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: fix error detection in MTU command
  2020-05-19 14:57 ` Ferruh Yigit
@ 2020-05-19 16:40   ` Shy Shyman
  2020-05-20 14:14     ` Ferruh Yigit
  0 siblings, 1 reply; 10+ messages in thread
From: Shy Shyman @ 2020-05-19 16:40 UTC (permalink / raw)
  To: Ferruh Yigit, dev
  Cc: Wenzhuo Lu, Beilei Xing, Bernard Iremonger, xavier.huwei

Hi 
Thank you for the feedback.
While I'm ok with the fix you suggested, the warning was placed in order to let the user understand that his/her MTU settings may not impact DPDK. After all in this case(no JUMBO) we are not doing anything besides changing the dev->data->mtu.

I think there is some confusion with MTU in DPDK and I'd like to take the opportunity the raise some questions related to MTU:

1. only the test-pmd application attempts to correlate MTU to other variables like max_ptk_len and JUMBO offload. should rte_ethdev  do that?
2. even in test-pmd case only RX settings are adjusted. Maybe TX should be regarded as well?
3. in general what should be the relation between MTU,max_pkt_let and JUMBO offload for both RX and TX?
4. currently the MTU is set upon initialization to 1500 by default. The MTU on the device itself could be different but DPDK doesn't check for that. Maybe it should be updated based on the device settings? On a similar note, let's say the user changed MTU in test-pmd. What is the desired behavior when existing the application? Today the MTU will remain changed even outside DPDK.

Thoughts?


Shy

-----Original Message-----
From: Ferruh Yigit <ferruh.yigit@intel.com> 
Sent: Tuesday, May 19, 2020 5:58 PM
To: Shy Shyman <shys@mellanox.com>; dev@dpdk.org
Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>; Beilei Xing <beilei.xing@intel.com>; Bernard Iremonger <bernard.iremonger@intel.com>; xavier.huwei@huawei.com
Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix error detection in MTU command

On 5/18/2020 10:27 AM, Shy Shyman wrote:
> MTU is used in testpmd to set the maximum payload size for packets.
> According to testpmd, the setting influnce RX only.
> In rte_ethdev there's no relation between MTU setting and JUMBO 
> offload or rx_max_pkt_len.
> 
> The previous fix in patch referenced below was meant to update the 
> correlated variables of max_pkt_len and JUMBO offload, but by doing so 
> it assumes that MTU setting can only exist when JUMBO offload 
> supported in the device.

+1 and this is wrong, as far as I understand this is not the intention 
+of the
previous fix.


> For example fail-safe device does supports set MTU and doesn't support 
> JUMBO offload, and in this case, though set MTU succeed an error 
> mesage still printed  since the JUMBO packet offload is disabled.
> 
> The fix separates the two conditions to make sure the error triggers 
> only in case the set_mtu action actually failed.
> A warning message is provided in this special case to alert the user.

Not sure if this warning is required at all.
As you said above intention is to based on MTU value to correlate testpmd
(application) max_pkt_len and JUMBO offload values.

What about following change:

.....
	diag = rte_eth_dev_set_mtu(port_id, mtu);
	if (diag) {
		printf("Set MTU failed. diag=%d\n", diag);
		return;
	}

	if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) {
		....
		....
		....
	}
}


> 
> Fixes: 150c9ac2df13 ("app/testpmd: update Rx offload after setting 
> MTU")
> Cc: xavier.huwei@huawei.com
> 
> Signed-off-by: Shy Shyman <shys@mellanox.com>
> ---
>  app/test-pmd/config.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index 
> 5381207cc..73b53c50b 100644
> --- a/app/test-pmd/config.c
> +++ b/app/test-pmd/config.c
> @@ -1277,8 +1277,9 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
>  		return;
>  	}
>  	diag = rte_eth_dev_set_mtu(port_id, mtu);
> -	if (diag == 0 &&
> -	    dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) {
> +	if (diag)
> +		printf("Set MTU failed. diag=%d\n", diag);
> +	else if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) {
>  		/*
>  		 * Ether overhead in driver is equal to the difference of
>  		 * max_rx_pktlen and max_mtu in rte_eth_dev_info when the @@ 
> -1293,10 +1294,9 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
>  		} else
>  			rte_port->dev_conf.rxmode.offloads &=
>  						~DEV_RX_OFFLOAD_JUMBO_FRAME;
> -
> -		return;
> -	}
> -	printf("Set MTU failed. diag=%d\n", diag);
> +	} else
> +		printf("WARNING: MTU was set while jumbo frame offload is"
> +			" not supported by the device\n");
>  }
>  
>  /* Generic flow management functions. */
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH] app/testpmd: fix error detection in MTU command
  2020-05-19 16:40   ` Shy Shyman
@ 2020-05-20 14:14     ` Ferruh Yigit
  0 siblings, 0 replies; 10+ messages in thread
From: Ferruh Yigit @ 2020-05-20 14:14 UTC (permalink / raw)
  To: Shy Shyman, dev; +Cc: Wenzhuo Lu, Beilei Xing, Bernard Iremonger, xavier.huwei

On 5/19/2020 5:40 PM, Shy Shyman wrote:
> Hi 
> Thank you for the feedback.
> While I'm ok with the fix you suggested, the warning was placed in order to let the user understand that his/her MTU settings may not impact DPDK. After all in this case(no JUMBO) we are not doing anything besides changing the dev->data->mtu.

Does anything needs to changed in the no jumbo case? I think no, but if there is
why no change it instead of warning message. I think warning can be confusing here.

> 
> I think there is some confusion with MTU in DPDK and I'd like to take the opportunity the raise some questions related to MTU:
> 
> 1. only the test-pmd application attempts to correlate MTU to other variables like max_ptk_len and JUMBO offload. should rte_ethdev  do that?
> 2. even in test-pmd case only RX settings are adjusted. Maybe TX should be regarded as well?
> 3. in general what should be the relation between MTU,max_pkt_let and JUMBO offload for both RX and TX?
> 4. currently the MTU is set upon initialization to 1500 by default. The MTU on the device itself could be different but DPDK doesn't check for that. Maybe it should be updated based on the device settings? On a similar note, let's say the user changed MTU in test-pmd. What is the desired behavior when existing the application? Today the MTU will remain changed even outside DPDK.
> 
> Thoughts?

+1 on the confusion. I don't have answers to all above question, but it would be
great if you tackle this clarification task.

> 
> 
> Shy
> 
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com> 
> Sent: Tuesday, May 19, 2020 5:58 PM
> To: Shy Shyman <shys@mellanox.com>; dev@dpdk.org
> Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>; Beilei Xing <beilei.xing@intel.com>; Bernard Iremonger <bernard.iremonger@intel.com>; xavier.huwei@huawei.com
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix error detection in MTU command
> 
> On 5/18/2020 10:27 AM, Shy Shyman wrote:
>> MTU is used in testpmd to set the maximum payload size for packets.
>> According to testpmd, the setting influnce RX only.
>> In rte_ethdev there's no relation between MTU setting and JUMBO 
>> offload or rx_max_pkt_len.
>>
>> The previous fix in patch referenced below was meant to update the 
>> correlated variables of max_pkt_len and JUMBO offload, but by doing so 
>> it assumes that MTU setting can only exist when JUMBO offload 
>> supported in the device.
> 
> +1 and this is wrong, as far as I understand this is not the intention 
> +of the
> previous fix.
> 
> 
>> For example fail-safe device does supports set MTU and doesn't support 
>> JUMBO offload, and in this case, though set MTU succeed an error 
>> mesage still printed  since the JUMBO packet offload is disabled.
>>
>> The fix separates the two conditions to make sure the error triggers 
>> only in case the set_mtu action actually failed.
>> A warning message is provided in this special case to alert the user.
> 
> Not sure if this warning is required at all.
> As you said above intention is to based on MTU value to correlate testpmd
> (application) max_pkt_len and JUMBO offload values.
> 
> What about following change:
> 
> .....
> 	diag = rte_eth_dev_set_mtu(port_id, mtu);
> 	if (diag) {
> 		printf("Set MTU failed. diag=%d\n", diag);
> 		return;
> 	}
> 
> 	if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) {
> 		....
> 		....
> 		....
> 	}
> }
> 
> 
>>
>> Fixes: 150c9ac2df13 ("app/testpmd: update Rx offload after setting 
>> MTU")
>> Cc: xavier.huwei@huawei.com
>>
>> Signed-off-by: Shy Shyman <shys@mellanox.com>
>> ---
>>  app/test-pmd/config.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c index 
>> 5381207cc..73b53c50b 100644
>> --- a/app/test-pmd/config.c
>> +++ b/app/test-pmd/config.c
>> @@ -1277,8 +1277,9 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
>>  		return;
>>  	}
>>  	diag = rte_eth_dev_set_mtu(port_id, mtu);
>> -	if (diag == 0 &&
>> -	    dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) {
>> +	if (diag)
>> +		printf("Set MTU failed. diag=%d\n", diag);
>> +	else if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) {
>>  		/*
>>  		 * Ether overhead in driver is equal to the difference of
>>  		 * max_rx_pktlen and max_mtu in rte_eth_dev_info when the @@ 
>> -1293,10 +1294,9 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
>>  		} else
>>  			rte_port->dev_conf.rxmode.offloads &=
>>  						~DEV_RX_OFFLOAD_JUMBO_FRAME;
>> -
>> -		return;
>> -	}
>> -	printf("Set MTU failed. diag=%d\n", diag);
>> +	} else
>> +		printf("WARNING: MTU was set while jumbo frame offload is"
>> +			" not supported by the device\n");
>>  }
>>  
>>  /* Generic flow management functions. */
>>
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [dpdk-dev] [PATCH v2] app/testpmd: fix error detection in MTU command
  2020-05-18  9:27 [dpdk-dev] [PATCH] app/testpmd: fix error detection in MTU command Shy Shyman
                   ` (2 preceding siblings ...)
  2020-05-19 14:57 ` Ferruh Yigit
@ 2020-06-08 14:17 ` Shy Shyman
  2020-06-08 15:04   ` Ferruh Yigit
  2020-06-09 14:44   ` Phil Yang
  3 siblings, 2 replies; 10+ messages in thread
From: Shy Shyman @ 2020-06-08 14:17 UTC (permalink / raw)
  To: dev; +Cc: wenzhuo.lu, beilei.xing, bernard.iremonger, xavier.huwei

MTU is used in testpmd to set the maximum payload size for packets.
According to testpmd, the setting influnce RX only.
In rte_ethdev there's no relation between MTU setting and JUMBO offload or
rx_max_pkt_len.

The previous fix in patch referenced below was meant to update the
correlated variables of max_pkt_len and JUMBO offload, but by doing
so it assumes that MTU setting can only exist when JUMBO offload
supported in the device. For example fail-safe device does supports set MTU
and doesn't support JUMBO offload, and in this case, though set MTU
succeeds, an error message is still printed since the JUMBO packet
offload is disabled.

The fix separates the two conditions to make sure the error
triggers only in case the set_mtu action actually failed.

Fixes: 150c9ac2df13 ("app/testpmd: update Rx offload after setting MTU")
Cc: xavier.huwei@huawei.com

Signed-off-by: Shy Shyman <shys@mellanox.com>
---
v2 - removed warning message.
---
 app/test-pmd/config.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/app/test-pmd/config.c b/app/test-pmd/config.c
index 5381207cc..3b54fb6a0 100644
--- a/app/test-pmd/config.c
+++ b/app/test-pmd/config.c
@@ -1277,8 +1277,9 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
 		return;
 	}
 	diag = rte_eth_dev_set_mtu(port_id, mtu);
-	if (diag == 0 &&
-	    dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) {
+	if (diag)
+		printf("Set MTU failed. diag=%d\n", diag);
+	else if (dev_info.rx_offload_capa & DEV_RX_OFFLOAD_JUMBO_FRAME) {
 		/*
 		 * Ether overhead in driver is equal to the difference of
 		 * max_rx_pktlen and max_mtu in rte_eth_dev_info when the
@@ -1293,10 +1294,7 @@ port_mtu_set(portid_t port_id, uint16_t mtu)
 		} else
 			rte_port->dev_conf.rxmode.offloads &=
 						~DEV_RX_OFFLOAD_JUMBO_FRAME;
-
-		return;
 	}
-	printf("Set MTU failed. diag=%d\n", diag);
 }
 
 /* Generic flow management functions. */
-- 
2.21.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH v2] app/testpmd: fix error detection in MTU command
  2020-06-08 14:17 ` [dpdk-dev] [PATCH v2] " Shy Shyman
@ 2020-06-08 15:04   ` Ferruh Yigit
  2020-06-09 14:44   ` Phil Yang
  1 sibling, 0 replies; 10+ messages in thread
From: Ferruh Yigit @ 2020-06-08 15:04 UTC (permalink / raw)
  To: Shy Shyman, dev; +Cc: wenzhuo.lu, beilei.xing, bernard.iremonger, xavier.huwei

On 6/8/2020 3:17 PM, Shy Shyman wrote:
> MTU is used in testpmd to set the maximum payload size for packets.
> According to testpmd, the setting influnce RX only.
> In rte_ethdev there's no relation between MTU setting and JUMBO offload or
> rx_max_pkt_len.
> 
> The previous fix in patch referenced below was meant to update the
> correlated variables of max_pkt_len and JUMBO offload, but by doing
> so it assumes that MTU setting can only exist when JUMBO offload
> supported in the device. For example fail-safe device does supports set MTU
> and doesn't support JUMBO offload, and in this case, though set MTU
> succeeds, an error message is still printed since the JUMBO packet
> offload is disabled.
> 
> The fix separates the two conditions to make sure the error
> triggers only in case the set_mtu action actually failed.
> 
> Fixes: 150c9ac2df13 ("app/testpmd: update Rx offload after setting MTU")
> Cc: xavier.huwei@huawei.com
> 
> Signed-off-by: Shy Shyman <shys@mellanox.com>

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH v2] app/testpmd: fix error detection in MTU command
  2020-06-08 14:17 ` [dpdk-dev] [PATCH v2] " Shy Shyman
  2020-06-08 15:04   ` Ferruh Yigit
@ 2020-06-09 14:44   ` Phil Yang
  2020-06-09 14:59     ` Ferruh Yigit
  1 sibling, 1 reply; 10+ messages in thread
From: Phil Yang @ 2020-06-09 14:44 UTC (permalink / raw)
  To: Shy Shyman, dev
  Cc: wenzhuo.lu, beilei.xing, bernard.iremonger, xavier.huwei, nd

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Shy Shyman
> Sent: Monday, June 8, 2020 10:18 PM
> To: dev@dpdk.org
> Cc: wenzhuo.lu@intel.com; beilei.xing@intel.com;
> bernard.iremonger@intel.com; xavier.huwei@huawei.com
> Subject: [dpdk-dev] [PATCH v2] app/testpmd: fix error detection in MTU
> command
> 
> MTU is used in testpmd to set the maximum payload size for packets.
> According to testpmd, the setting influnce RX only.
> In rte_ethdev there's no relation between MTU setting and JUMBO offload
> or
> rx_max_pkt_len.
> 
> The previous fix in patch referenced below was meant to update the
> correlated variables of max_pkt_len and JUMBO offload, but by doing
> so it assumes that MTU setting can only exist when JUMBO offload
> supported in the device. For example fail-safe device does supports set MTU
> and doesn't support JUMBO offload, and in this case, though set MTU
> succeeds, an error message is still printed since the JUMBO packet
> offload is disabled.
> 
> The fix separates the two conditions to make sure the error
> triggers only in case the set_mtu action actually failed.
> 
> Fixes: 150c9ac2df13 ("app/testpmd: update Rx offload after setting MTU")
> Cc: xavier.huwei@huawei.com
> 
> Signed-off-by: Shy Shyman <shys@mellanox.com>

Reviewed-by: Phil Yang <phil.yang@arm.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH v2] app/testpmd: fix error detection in MTU command
  2020-06-09 14:44   ` Phil Yang
@ 2020-06-09 14:59     ` Ferruh Yigit
  0 siblings, 0 replies; 10+ messages in thread
From: Ferruh Yigit @ 2020-06-09 14:59 UTC (permalink / raw)
  To: Phil Yang, Shy Shyman, dev
  Cc: wenzhuo.lu, beilei.xing, bernard.iremonger, xavier.huwei, nd

On 6/9/2020 3:44 PM, Phil Yang wrote:
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Shy Shyman
>> Sent: Monday, June 8, 2020 10:18 PM
>> To: dev@dpdk.org
>> Cc: wenzhuo.lu@intel.com; beilei.xing@intel.com;
>> bernard.iremonger@intel.com; xavier.huwei@huawei.com
>> Subject: [dpdk-dev] [PATCH v2] app/testpmd: fix error detection in MTU
>> command
>>
>> MTU is used in testpmd to set the maximum payload size for packets.
>> According to testpmd, the setting influnce RX only.
>> In rte_ethdev there's no relation between MTU setting and JUMBO offload
>> or
>> rx_max_pkt_len.
>>
>> The previous fix in patch referenced below was meant to update the
>> correlated variables of max_pkt_len and JUMBO offload, but by doing
>> so it assumes that MTU setting can only exist when JUMBO offload
>> supported in the device. For example fail-safe device does supports set MTU
>> and doesn't support JUMBO offload, and in this case, though set MTU
>> succeeds, an error message is still printed since the JUMBO packet
>> offload is disabled.
>>
>> The fix separates the two conditions to make sure the error
>> triggers only in case the set_mtu action actually failed.
>>
>> Fixes: 150c9ac2df13 ("app/testpmd: update Rx offload after setting MTU")
>> Cc: xavier.huwei@huawei.com
>>
>> Signed-off-by: Shy Shyman <shys@mellanox.com>
> 
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> 

Applied to dpdk-next-net/master, thanks.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-06-09 14:59 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-18  9:27 [dpdk-dev] [PATCH] app/testpmd: fix error detection in MTU command Shy Shyman
2020-05-19 11:10 ` Asaf Penso
2020-05-19 14:15 ` Iremonger, Bernard
2020-05-19 14:57 ` Ferruh Yigit
2020-05-19 16:40   ` Shy Shyman
2020-05-20 14:14     ` Ferruh Yigit
2020-06-08 14:17 ` [dpdk-dev] [PATCH v2] " Shy Shyman
2020-06-08 15:04   ` Ferruh Yigit
2020-06-09 14:44   ` Phil Yang
2020-06-09 14:59     ` Ferruh Yigit

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).