[v3,1/3] ethdev: fix MTU doesn't update when jumbo frame disabled
Checks
Commit Message
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
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.
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.
>
@@ -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.