diff mbox series

[v3,1/3] ethdev: fix MTU doesn't update when jumbo frame disabled

Message ID 20210122090110.50453-2-stevex.yang@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers show
Series fix 'max-pkt-len' errors | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Steve Yang Jan. 22, 2021, 9:01 a.m. UTC
The MTU value should be updated to 'max_rx_pkt_len - overhead'
no matter if the JUMBO FRAME offload enabled. If not update this MTU,
use will get the wrong MTU info via some command.
E.g.: 'show port info all' in testpmd tool.

Actually, the 'max_rx_pkt_len' has been used for other purposes in many
places now, even though the 'max_rx_pkt_len' is expected 'Only used if
JUMBO_FRAME enabled'.

For examples,
'max_rx_pkt_len' perhaps can be used as the 'rx_ctx.rxmax' in i40e.

Fixes: bf0f90d92d30 ("ethdev: fix max Rx packet length check")

Signed-off-by: Steve Yang <stevex.yang@intel.com>
---
 lib/librte_ethdev/rte_ethdev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Huisong Li Jan. 25, 2021, 7:12 a.m. UTC | #1
Hi Steve,

In the current modification, the MTU is updated based on 
'max_rx_pkt_len' regardless of whether jumbo frame is enabled.

Now, MTU is correct when jumbo frmae is disabled. However, when jumbo 
frame is enabled, the MTU value may be inconsistent with

the definition of the enabled jumbo frame. Like:

1/ DEV_RX_OFFLOAD_JUMBO_FRAME is set;

2/ max_rx_pkt_len = 1200

3/ dev->data->mtu = 1200 - overhead_len(18) = 1182


In rte_eth_dev_configure API, the check for 'max_rx_pkt_len' is as follows:

if (dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) {  //jumbo 
frame enabled
         if (dev_conf->rxmode.max_rx_pkt_len > dev_info.max_rx_pktlen) {
             xxxx
             goto rollback;
         } else if (*dev_conf->rxmode.max_rx_pkt_len < RTE_ETHER_MIN_LEN*) {
             xxxx
             goto rollback;
         }
} else { //jumbo frame disabled

         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;

}

Since the applicatin sets DEV_RX_OFFLOAD_JUMBO_FRAME to enable jumbo 
frame, and the framework API needs to update

the MTU based on 'max_rx_pkt_len', but the framework API uses 
*RTE_ETHER_MIN_LEN(64)* to verify the boundary value of

'max_rx_pkt_len', instead of "RTE_ETHER_MTU + overhead_len".  As far as 
I know, if the applicatin sets DEV_RX_OFFLOAD_JUMBO_FRAME

and 'max_rx_pkt_len' is 1200, the framework API or driver should return 
a failure. As mentioned in this patch set, the jumbo frame

offload is set only when 'max_rx_pkt_len' requested is greater than 
"RTE_ETHER_MTU + eth_overhead" in testpmd.

I really don't understand it.  How do you understand this behavior?

Thanks.


在 2021/1/22 17:01, Steve Yang 写道:
> The MTU value should be updated to 'max_rx_pkt_len - overhead'
> no matter if the JUMBO FRAME offload enabled. If not update this MTU,
> use will get the wrong MTU info via some command.
> E.g.: 'show port info all' in testpmd tool.
>
> Actually, the 'max_rx_pkt_len' has been used for other purposes in many
> places now, even though the 'max_rx_pkt_len' is expected 'Only used if
> JUMBO_FRAME enabled'.
>
> For examples,
> 'max_rx_pkt_len' perhaps can be used as the 'rx_ctx.rxmax' in i40e.
>
> Fixes: bf0f90d92d30 ("ethdev: fix max Rx packet length check")
>
> Signed-off-by: Steve Yang <stevex.yang@intel.com>
> ---
>   lib/librte_ethdev/rte_ethdev.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index daf5f24f7e..42857e3b67 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -1421,10 +1421,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>   			ret = -EINVAL;
>   			goto rollback;
>   		}
> -
> -		/* Scale the MTU size to adapt max_rx_pkt_len */
> -		dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
> -				overhead_len;
>   	} else {
>   		uint16_t pktlen = dev_conf->rxmode.max_rx_pkt_len;
>   		if (pktlen < RTE_ETHER_MIN_MTU + overhead_len ||
> @@ -1434,6 +1430,10 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
>   						RTE_ETHER_MTU + overhead_len;
>   	}
>   
> +	/* Scale the MTU size to adapt max_rx_pkt_len */
> +	dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
> +				overhead_len;
> +
>   	/*
>   	 * If LRO is enabled, check that the maximum aggregated packet
>   	 * size is supported by the configured device.
Ferruh Yigit Jan. 25, 2021, 12:38 p.m. UTC | #2
On 1/25/2021 9:49 AM, Yang, SteveX wrote:
> Hi Huisong,
> 
> Thanks for your review.
> 
> The validity of the pair <DEV_RX_OFFLOAD_JUMBO_FRAME, max_rx_pkt_len> should be 
> checked from application layer (e.g.: testpmd),
> 
> and the RTE layer should keep open enough to adapt the high-layer requirement.
> 
> I’m not sure if exists some applications/NICs that treat ‘packet size < 1500’ as 
> JUMBO_FRAME. If so, that also can work as expect with current code.
> 
> @Yigit, Ferruh <mailto:ferruh.yigit@intel.com>, please correct me if something 
> understand wrong.
> 

Hi Huisong,

Agree that there is a grey area in the API, the question is if 'JUMBO_FRAME' is 
set, can application set the 'max_rx_pkt_len' less than "RTE_ETHER_MTU + 
overhead_len". Lijun (cc'ed) has the same concern.

The API documentation, and checks in the 'rte_eth_dev_configure()' enables 
setting this for a long time, I am reluctant to add this limitation now.
Although agree that application should set 'JUMBO_FRAME' properly based on 
requested 'MTU' value.


> BTW, there perhaps are some confused problems about jumbo frame and 
> max_rx_pkt_len, and Ferruh has scheduled to re-factor this part at release 21.11.
> 
> If you’re interesting about it, please refer to following link: [RFC,v2] doc: 
> announce max Rx packet len field deprecation - Patchwork (dpdk.org) 
> <http://patches.dpdk.org/patch/84522/>
> 
> Thanks & Regards,
> 
> Steve Yang.
> 
> *From:* Huisong Li <lihuisong@huawei.com>
> *Sent:* Monday, January 25, 2021 3:12 PM
> *To:* Yang, SteveX <stevex.yang@intel.com>; dev@dpdk.org
> *Cc:* Lu, Wenzhuo <wenzhuo.lu@intel.com>; Li, Xiaoyun <xiaoyun.li@intel.com>; 
> Iremonger, Bernard <bernard.iremonger@intel.com>; thomas@monjalon.net; Yigit, 
> Ferruh <ferruh.yigit@intel.com>; andrew.rybchenko@oktetlabs.ru; Yang, Qiming 
> <qiming.yang@intel.com>; oulijun@huawei.com; huangdaode@huawei.com
> *Subject:* Re: [dpdk-dev] [PATCH v3 1/3] ethdev: fix MTU doesn't update when 
> jumbo frame disabled
> 
> Hi Steve,
> 
> In the current modification, the MTU is updated based on 'max_rx_pkt_len' 
> regardless of whether jumbo frame is enabled.
> 
> Now, MTU is correct when jumbo frmae is disabled. However, when jumbo frame is 
> enabled, the MTU value may be inconsistent with
> 
> the definition of the enabled jumbo frame. Like:
> 
> 1/ DEV_RX_OFFLOAD_JUMBO_FRAME is set;
> 
> 2/ max_rx_pkt_len = 1200
> 
> 3/ dev->data->mtu = 1200 - overhead_len(18) = 1182
> 
> In rte_eth_dev_configure API, the check for 'max_rx_pkt_len' is as follows:
> 
> if (dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_JUMBO_FRAME) {  //jumbo frame enabled
>          if (dev_conf->rxmode.max_rx_pkt_len > dev_info.max_rx_pktlen) {
>              xxxx
>              goto rollback;
>          } else if (*dev_conf->rxmode.max_rx_pkt_len < RTE_ETHER_MIN_LEN*) {
>              xxxx
>              goto rollback;
>          }
> } else { //jumbo frame disabled
> 
>          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;
> 
> }
> 
> Since the applicatin sets DEV_RX_OFFLOAD_JUMBO_FRAME to enable jumbo frame, and 
> the framework API needs to update
> 
> the MTU based on 'max_rx_pkt_len', but the framework API uses 
> *RTE_ETHER_MIN_LEN(64)* to verify the boundary value of
> 
> 'max_rx_pkt_len', instead of "RTE_ETHER_MTU + overhead_len".  As far as I know, 
> if the applicatin sets DEV_RX_OFFLOAD_JUMBO_FRAME
> 
> and 'max_rx_pkt_len' is 1200, the framework API or driver should return a 
> failure. As mentioned in this patch set, the jumbo frame
> 
> offload is set only when 'max_rx_pkt_len' requested is greater than 
> "RTE_ETHER_MTU + eth_overhead" in testpmd.
> 
> I really don't understand it.  How do you understand this behavior?
> 
> Thanks.
> 
> 在 2021/1/22 17:01, Steve Yang 写道:
> 
>     The MTU value should be updated to 'max_rx_pkt_len - overhead'
> 
>     no matter if the JUMBO FRAME offload enabled. If not update this MTU,
> 
>     use will get the wrong MTU info via some command.
> 
>     E.g.: 'show port info all' in testpmd tool.
> 
>     Actually, the 'max_rx_pkt_len' has been used for other purposes in many
> 
>     places now, even though the 'max_rx_pkt_len' is expected 'Only used if
> 
>     JUMBO_FRAME enabled'.
> 
>     For examples,
> 
>     'max_rx_pkt_len' perhaps can be used as the 'rx_ctx.rxmax' in i40e.
> 
>     Fixes: bf0f90d92d30 ("ethdev: fix max Rx packet length check")
> 
>     Signed-off-by: Steve Yang<stevex.yang@intel.com>  <mailto:stevex.yang@intel.com>
> 
>     ---
> 
>       lib/librte_ethdev/rte_ethdev.c | 8 ++++----
> 
>       1 file changed, 4 insertions(+), 4 deletions(-)
> 
>     diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> 
>     index daf5f24f7e..42857e3b67 100644
> 
>     --- a/lib/librte_ethdev/rte_ethdev.c
> 
>     +++ b/lib/librte_ethdev/rte_ethdev.c
> 
>     @@ -1421,10 +1421,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
> 
>                              ret = -EINVAL;
> 
>                              goto rollback;
> 
>                      }
> 
>     -
> 
>     -               /* Scale the MTU size to adapt max_rx_pkt_len */
> 
>     -               dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
> 
>     -                              overhead_len;
> 
>              } else {
> 
>                      uint16_t pktlen = dev_conf->rxmode.max_rx_pkt_len;
> 
>                      if (pktlen < RTE_ETHER_MIN_MTU + overhead_len ||
> 
>     @@ -1434,6 +1430,10 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
> 
>                                                     RTE_ETHER_MTU + overhead_len;
> 
>              }
> 
>       
> 
>     +       /* Scale the MTU size to adapt max_rx_pkt_len */
> 
>     +       dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
> 
>     +                              overhead_len;
> 
>     +
> 
>              /*
> 
>               * If LRO is enabled, check that the maximum aggregated packet
> 
>               * size is supported by the configured device.
>
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index daf5f24f7e..42857e3b67 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -1421,10 +1421,6 @@  rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 			ret = -EINVAL;
 			goto rollback;
 		}
-
-		/* Scale the MTU size to adapt max_rx_pkt_len */
-		dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
-				overhead_len;
 	} else {
 		uint16_t pktlen = dev_conf->rxmode.max_rx_pkt_len;
 		if (pktlen < RTE_ETHER_MIN_MTU + overhead_len ||
@@ -1434,6 +1430,10 @@  rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 						RTE_ETHER_MTU + overhead_len;
 	}
 
+	/* Scale the MTU size to adapt max_rx_pkt_len */
+	dev->data->mtu = dev->data->dev_conf.rxmode.max_rx_pkt_len -
+				overhead_len;
+
 	/*
 	 * If LRO is enabled, check that the maximum aggregated packet
 	 * size is supported by the configured device.