Re: [dpdk-dev] [PATCH] app/testpmd: fix error detection in MTU command

[Ferruh Yigit <ferruh.yigit@intel.com>]

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. */
>