On 1/14/2021 5:29 PM, Andrew Boyer wrote:

quoted

On Jan 14, 2021, at 12:13 PM, Ferruh Yigit <ferruh.yigit@intel.com 
<mailto:ferruh.yigit@intel.com>> wrote:

On 1/14/2021 4:36 PM, Ferruh Yigit wrote:

quoted

On 1/14/2021 9:45 AM, Steve Yang wrote:

quoted

Ethdev is using default Ethernet overhead to decide if provided
'max_rx_pkt_len' value is bigger than max (non jumbo) MTU value,
and limits it to MAX if it is.

Since the application/driver used Ethernet overhead is different than
the ethdev one, check result is wrong.

If the driver is using Ethernet overhead bigger than the default one,
the provided 'max_rx_pkt_len' is trimmed down, and in the driver when
correct Ethernet overhead is used to convert back, the resulting MTU is
less than the intended one, causing some packets to be dropped.

Like,
app     -> max_rx_pkt_len = 1500/*mtu*/ + 22/*overhead*/ = 1522
ethdev  -> 1522 > 1518/*MAX*/; max_rx_pkt_len = 1518
driver  -> MTU = 1518 - 22 = 1496
Packets with size 1497-1500 are dropped although intention is to be able
to send/receive them.

The fix is to make ethdev use the correct Ethernet overhead for port,
instead of default one.

Fixes: 59d0ecdbf0e1 ("ethdev: MTU accessors")

Signed-off-by: Steve Yang <stevex.yang@intel.com <mailto:stevex.yang@intel.com>>

<...>

quoted

@@ -1410,11 +1422,18 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t 

nb_rx_q, uint16_t nb_tx_q,
goto rollback;
}
} else {
-        if (dev_conf->rxmode.max_rx_pkt_len < RTE_ETHER_MIN_LEN ||
-            dev_conf->rxmode.max_rx_pkt_len > RTE_ETHER_MAX_LEN)
+        uint16_t pktlen = dev_conf->rxmode.max_rx_pkt_len;
+        if (pktlen < RTE_ETHER_MIN_MTU + overhead_len ||
+            pktlen > RTE_ETHER_MTU + overhead_len)
/* Use default value */
dev->data->dev_conf.rxmode.max_rx_pkt_len =
-                            RTE_ETHER_MAX_LEN;
+                        RTE_ETHER_MTU + overhead_len;

What do you think removing the above check, the else block, completely?
Since the 'max_rx_pkt_len' should not be used when jumbo frame is not set.

As I tested removing this check is causing problem because some PMDs are using 
the 'max_rx_pkt_len' even jumbo frame is not set.

Perhaps better to keep it, and make a separate patch later to remove this 
check, after PMDs fixed.

Hello Ferruh -
Working on fixing our PMD here. Do you want PMDs to update the JUMBO_FRAME flag 
based on the mtu value in dev_set_mtu(), or do you want the application to be 
solely responsible for it?

Hi Andrew,

Technically JUMBO_FRAME flag is an user config and application should set it. It 
is application's responsibility to check the capability and set the flag when 
necessary.

But, after above said, many PMDs set it based on provided MTU value, if the 
explicitly requested MTU is bigger than the RTE_ETHER_MTU, this means user 
implied the JUMBO_FRAME support, for this case PMDs set the flag implicitly 
instead of failing.

In another thread Andrew R. & Konstantin suggested to remove the JUMBO_FRAME 
flag, since it is redundant and causing this kind of confusion, instead driver 
can decide based on requested MTU value, and driver reported 'max_mtu' value can 
be used by application to detect the capability. We will probably do this 
change, but it can be done only in the ABI break release, v21.11.

For now, PMD can set the flag itself if requested MTU > RTE_ETHER_MTU and driver 
supports jumbo frames.

Thanks,
Andrew

quoted

quoted

quoted

+    }
+
+    /* Scale the MTU size to adapt max_rx_pkt_len */
+    if (dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) {
+        dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
+                overhead_len;
}

Above if block has exact same check, why not move it above block?

Can you still send a new version for above change please?