在 2021/1/18 18:42, Ferruh Yigit 写道:

On 1/15/2021 10:44 AM, oulijun wrote:

quoted

Hi Steve
This is a very good job! But I have some question and suggestions.
Please check it.

在 2021/1/14 17:45, Steve Yang 写道:

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>
---
  lib/librte_ethdev/rte_ethdev.c | 27 ++++++++++++++++++++++++---
  1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/lib/librte_ethdev/rte_ethdev.c 

b/lib/librte_ethdev/rte_ethdev.c
index 17ddacc78d..19ca4c4512 100644

--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c

@@ -1292,8 +1292,10 @@ rte_eth_dev_configure(uint16_t port_id, 

uint16_t nb_rx_q, uint16_t nb_tx_q,
      struct rte_eth_dev *dev;
      struct rte_eth_dev_info dev_info;
      struct rte_eth_conf orig_conf;
+    uint16_t overhead_len;
      int diag;
      int ret;
+    uint16_t old_mtu;
      RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);

@@ -1319,10 +1321,20 @@ rte_eth_dev_configure(uint16_t port_id, 

uint16_t nb_rx_q, uint16_t nb_tx_q,
          memcpy(&dev->data->dev_conf, dev_conf,
                 sizeof(dev->data->dev_conf));
+    /* Backup mtu for rollback */
+    old_mtu = dev->data->mtu;
+
      ret = rte_eth_dev_info_get(port_id, &dev_info);
      if (ret != 0)
          goto rollback;
+    /* Get the real Ethernet overhead length */
+    if (dev_info.max_mtu != UINT16_MAX &&
+        dev_info.max_rx_pktlen > dev_info.max_mtu)
+        overhead_len = dev_info.max_rx_pktlen - dev_info.max_mtu;
+    else
+        overhead_len = RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN;
+

The Ethernet frame header length supported by each NIC may be 
different, which cause the difference of allowed packets size and drop 
you say.
The diference of Ethernet frame header length have an impact for the 
maximum Ethernet frame length, which is a boundary value
of enabling jumbo frame through the ' max_rx_pkt_len '.
However, we need to do the same thing you do above to get the 
overhead_len every time, which will
cause a lot of duplicate code in the framework and app. For examples, 
parsing and processing for '--max-pkt-len=xxx' parameter,
  and " cmd_config_max_pkt_len_parsed " in testpmd, and here modifying 
here dev_configure API.
It's a little redundant and troublesome.

Maybe, it is necessary for driver to directly report the supported 
maximum Ethernet frame length by rte_dev_info_get API.
As following:
struct rte_eth_dev_info {
     xxxx
     /**
      * The maximum Ethernet frame length supported by each
      * driver varies with the Ethernet header length.
      */
     uint16_t eth_max_len;
     xxxx
}

int
rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
{
     xxx
     dev_info->min_mtu = RTE_ETHER_MIN_MTU;
     dev_info->max_mtu = UINT16_MAX;
     dev_info->eth_max_len = RTE_ETHER_MAX_LEN;
     xxx
}

And then:
xxx_devv_infos_get(struct rte_eth_dev *eth_dev, struct 
rte_eth_dev_info *info)
{
     xxx
     info->eth_max_len = xxx_ETHER_MAX _LEN;
     xxx
}
What do you think?

Hi Lijun,

The 'max_mtu' has been added in the past to solve exact same problem, 
perhaps it would be better to add something like 'eth_max_len' at that 
time.

But as Steve mentioned we are planning to switch to more MTU based 
configuration, which should remove the need of the field you mentioned.

quoted

quoted

      /* If number of queues specified by application for both Rx and 
Tx is
       * zero, use driver preferred values. This cannot be done 
individually
       * as it is valid for either Tx or Rx (but not both) to be zero.

@@ -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;
+    }
+
+    /* 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;
      }

Now that we update mtu here when jumbo frame enabled. It is necessary 
to check validity of max_rx_pkt_len.
As following:
if (dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) {
     if (dev_conf->rxmode.max_rx_pkt_len > dev_info.max_rx_pktlen ||
         dev_conf->rxmode.max_rx_pkt_len <=  RTE_ETHER_MTU + 
overhead_len) {

It is not clear from API that if "max_rx_pkt_len <=  RTE_ETHER_MTU + 
overhead_len" is a valid value or not, the API says:
"max_rx_pkt_len;  /**< Only used if JUMBO_FRAME enabled. */"

But it doesn't say that when 'DEV_RX_OFFLOAD_JUMBO_FRAME' is set, 
'max_rx_pkt_len' should be less then 'RTE_ETHER_MTU'

The MTU setting entry does need to be adjusted. I agree! But I think 
this place should be modified, and it's probably more reasonable.

Generally speaking, max_rx_pkt_len will be also updated with 'mtu + 
overhead_len' also when application sets mtu > 1500, which means 
JUMBO_FRAME is enabled.
Namely, JUMBO_FRAME is enabled based on mtu > 1500 or max_rx_pkt_len > 
1500 + overhead_len.
Besides, having following check for "rxmode.offloads & 
DEV_RX_OFFLOAD_JUMBO_FRAME == 0":

@@ -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 {
         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_MTU + overhead_len;
}
One of check condition for the branch is "pktlen > RTE_ETHER_MTU + 
overhead_len ".

In view of the above two aspects, when application set 
DEV_RX_OFFLOAD_JUMBO_FRAME in offload, the max_rx_pkt_len must be 
greater than " RTE_ETHER_MTU + overhead_len "

Since DEV_RX_OFFLOAD_JUMBO_FRAME is not removed, so the check is necessary.

quoted

             xxxx
         }
} else {
         xxx
}

If it does not thing above, which will cause an unreasonable case.
Like,
dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME == 1
dev_conf->rxmode.max_rx_pkt_len = 1500
overhead_len = 26
dev->data->mtu = dev_conf->rxmode.max_rx_pkt_len - overhead_len = 1500 
- 26 = 1474

This looks correct, if the applicatin is requesting 'max_rx_pkt_len = 
1500', the requeted MTU is 1474,
if application wants an MTU as 1500, it should set the 'max_rx_pkt_len' 
as "1500 + overhead".

quoted

In fact, DEV_RX_OFFLOAD_JUMBO_FRAME is set to rxmode.offloads when mtu 

quoted

1500.

Overall the 'max_rx_pkt_len' setting is a little messy, it has been 
tried to fix in last release but we reverted them because of unexpected 
side affects.

Current long term plan is,
- Switch both 'rte_eth_dev_configure()' & 'rte_eth_dev_set_mtu()' to 
work on MTU values. (instead one working on frame lenght and other on MTU)
- Remove the 'DEV_RX_OFFLOAD_JUMBO_FRAME' and decide jumbo frame request 
by requested MTU value check
- App decide the jumbo frame capability from the 'max_mtu' check
- App calculate Ethernet overhead using 'max_mtu' and 'max_rx_pktlen'

quoted

quoted

      /*

@@ -1549,6 +1568,8 @@ rte_eth_dev_configure(uint16_t port_id, 

uint16_t nb_rx_q, uint16_t nb_tx_q,
      eth_dev_tx_queue_config(dev, 0);
  rollback:
      memcpy(&dev->data->dev_conf, &orig_conf, 
sizeof(dev->data->dev_conf));
+    if (old_mtu != dev->data->mtu)
+        dev->data->mtu = old_mtu;
      rte_ethdev_trace_configure(port_id, nb_rx_q, nb_tx_q, dev_conf, 
ret);
      return ret;

.