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.yi...@intel.com>
> Sent: Tuesday, May 19, 2020 5:58 PM
> To: Shy Shyman <s...@mellanox.com>; dev@dpdk.org
> Cc: Wenzhuo Lu <wenzhuo...@intel.com>; Beilei Xing <beilei.x...@intel.com>;
> Bernard Iremonger <bernard.iremon...@intel.com>; xavier.hu...@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.hu...@huawei.com
>>
>> Signed-off-by: Shy Shyman <s...@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. */
>>
>
