On Jan 14, 2021, at 12:13 PM, Ferruh Yigit [off-list ref] 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 <redacted>

<...>

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?

Thanks,
Andrew

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?