From: "lihuisong (C)" <lihuisong@huawei.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: <dev@dpdk.org>, Thomas Monjalon <thomas@monjalon.net>,
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
Subject: Re: [dpdk-dev] [PATCH v5 5/6] ethdev: unify MTU checks
Date: Tue, 12 Oct 2021 12:02:52 +0800 [thread overview]
Message-ID: <36f9c73f-596b-0f64-80d2-f800181e6e4e@huawei.com> (raw)
In-Reply-To: <8afc3490-84b1-37e8-6261-ec535cf98e80@intel.com>
在 2021/10/12 4:15, Ferruh Yigit 写道:
> On 10/9/2021 12:43 PM, lihuisong (C) wrote:
>> Hi, Ferruh
>>
>> 在 2021/10/8 0:56, Ferruh Yigit 写道:
>>> Both 'rte_eth_dev_configure()' & 'rte_eth_dev_set_mtu()' sets MTU but
>>> have slightly different checks. Like one checks min MTU against
>>> RTE_ETHER_MIN_MTU and other RTE_ETHER_MIN_LEN.
>>>
>>> Checks moved into common function to unify the checks. Also this has
>>> benefit to have common error logs.
>>>
>>> Suggested-by: Huisong Li <lihuisong@huawei.com>
>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>> ---
>>> lib/ethdev/rte_ethdev.c | 82
>>> ++++++++++++++++++++++++++---------------
>>> lib/ethdev/rte_ethdev.h | 2 +-
>>> 2 files changed, 54 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>> index c2b624aba1a0..0a6e952722ae 100644
>>> --- a/lib/ethdev/rte_ethdev.c
>>> +++ b/lib/ethdev/rte_ethdev.c
>>> @@ -1336,6 +1336,47 @@ eth_dev_get_overhead_len(uint32_t
>>> max_rx_pktlen, uint16_t max_mtu)
>>> return overhead_len;
>>> }
>>> +/* rte_eth_dev_info_get() should be called prior to this function */
>>> +static int
>>> +eth_dev_validate_mtu(uint16_t port_id, struct rte_eth_dev_info
>>> *dev_info,
>>> + uint16_t mtu)
>>> +{
>>> + uint16_t overhead_len;
>>> + uint32_t frame_size;
>>> +
>>> + if (mtu < dev_info->min_mtu) {
>>> + RTE_ETHDEV_LOG(ERR,
>>> + "MTU (%u) < device min MTU (%u) for port_id %u\n",
>>> + mtu, dev_info->min_mtu, port_id);
>>> + return -EINVAL;
>>> + }
>>> + if (mtu > dev_info->max_mtu) {
>>> + RTE_ETHDEV_LOG(ERR,
>>> + "MTU (%u) > device max MTU (%u) for port_id %u\n",
>>> + mtu, dev_info->max_mtu, port_id);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + overhead_len = eth_dev_get_overhead_len(dev_info->max_rx_pktlen,
>>> + dev_info->max_mtu);
>>> + frame_size = mtu + overhead_len;
>>> + if (frame_size < RTE_ETHER_MIN_LEN) {
>>> + RTE_ETHDEV_LOG(ERR,
>>> + "Frame size (%u) < min frame size (%u) for port_id %u\n",
>>> + frame_size, RTE_ETHER_MIN_LEN, port_id);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (frame_size > dev_info->max_rx_pktlen) {
>>> + RTE_ETHDEV_LOG(ERR,
>>> + "Frame size (%u) > device max frame size (%u) for
>>> port_id %u\n",
>>> + frame_size, dev_info->max_rx_pktlen, port_id);
>>> + return -EINVAL;
>>> + }
>>
>> This function is used to verify the MTU. So "frame_size" is redundant.
>>
>
> Yes it is redundant for the drivers that both announce 'max_rx_pktlen'
> & 'max_mtu',
> but stil some drivers doesn't announce the 'max_mtu' values and
> default value
> 'UINT16_MAX' is set by ethdev, specially virtual drivers.
> That is why I kept both to be in safe side.
>
Good job!
>> As modified by this patch, dev_info->min_mtu is calculated based on
>> RTE_ETHER_MIN_LEN.
>>
>
> And for the min check, for the default 'min_mtu' check is redundant,
> but for the cases
> driver sets "min_mtu < (RTE_ETHER_MIN_LEN - overhead_len)" second
> check becomes
> different limit. I don't know if this happens at all in practice but I
> think it
> doesn't hurt to have both checks to be on safe side.
It's a little ingenious.
is it better to add comments on the check for "frame_size"?
Another comment,
A few drivers report the minimum MTU using previous value.
Now, we have modified the "min_mtu" in ethdev layer.
Do you think we need to delete them?
>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> int
>>> rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t
>>> nb_tx_q,
>>> const struct rte_eth_conf *dev_conf)
>>> @@ -1463,26 +1504,13 @@ rte_eth_dev_configure(uint16_t port_id,
>>> uint16_t nb_rx_q, uint16_t nb_tx_q,
>>> goto rollback;
>>> }
>>> - /*
>>> - * Check that the maximum RX packet length is supported by the
>>> - * configured device.
>>> - */
>>> if (dev_conf->rxmode.mtu == 0)
>>> dev->data->dev_conf.rxmode.mtu = RTE_ETHER_MTU;
>>> - max_rx_pktlen = dev->data->dev_conf.rxmode.mtu + overhead_len;
>>> - if (max_rx_pktlen > dev_info.max_rx_pktlen) {
>>> - RTE_ETHDEV_LOG(ERR,
>>> - "Ethdev port_id=%u max_rx_pktlen %u > max valid value
>>> %u\n",
>>> - port_id, max_rx_pktlen, dev_info.max_rx_pktlen);
>>> - ret = -EINVAL;
>>> - goto rollback;
>>> - } else if (max_rx_pktlen < RTE_ETHER_MIN_LEN) {
>>> - RTE_ETHDEV_LOG(ERR,
>>> - "Ethdev port_id=%u max_rx_pktlen %u < min valid value
>>> %u\n",
>>> - port_id, max_rx_pktlen, RTE_ETHER_MIN_LEN);
>>> - ret = -EINVAL;
>>> +
>>> + ret = eth_dev_validate_mtu(port_id, &dev_info,
>>> + dev->data->dev_conf.rxmode.mtu);
>>> + if (ret != 0)
>>> goto rollback;
>>> - }
>>> dev->data->mtu = dev->data->dev_conf.rxmode.mtu;
>>> @@ -1491,6 +1519,9 @@ rte_eth_dev_configure(uint16_t port_id,
>>> uint16_t nb_rx_q, uint16_t nb_tx_q,
>>> * size is supported by the configured device.
>>> */
>>> if (dev_conf->rxmode.offloads & DEV_RX_OFFLOAD_TCP_LRO) {
>>> + overhead_len =
>>> eth_dev_get_overhead_len(dev_info.max_rx_pktlen,
>>> + dev_info.max_mtu);
>>> + max_rx_pktlen = dev->data->dev_conf.rxmode.mtu + overhead_len;
>>> if (dev_conf->rxmode.max_lro_pkt_size == 0)
>>> dev->data->dev_conf.rxmode.max_lro_pkt_size = max_rx_pktlen;
>>> ret = eth_dev_check_lro_pkt_size(port_id,
>>> @@ -3437,7 +3468,8 @@ rte_eth_dev_info_get(uint16_t port_id, struct
>>> rte_eth_dev_info *dev_info)
>>> dev_info->rx_desc_lim = lim;
>>> dev_info->tx_desc_lim = lim;
>>> dev_info->device = dev->device;
>>> - dev_info->min_mtu = RTE_ETHER_MIN_MTU;
>>> + dev_info->min_mtu = RTE_ETHER_MIN_LEN - RTE_ETHER_HDR_LEN -
>>> + RTE_ETHER_CRC_LEN;
>> I suggest that the adjustment to the minimum mtu size is also
>> explicitly reflected in the commit log.
>
> ack, I will
>
>>> dev_info->max_mtu = UINT16_MAX;
>>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
>>> @@ -3643,21 +3675,13 @@ rte_eth_dev_set_mtu(uint16_t port_id,
>>> uint16_t mtu)
>>> * which relies on dev->dev_ops->dev_infos_get.
>>> */
>>> if (*dev->dev_ops->dev_infos_get != NULL) {
>>> - uint16_t overhead_len;
>>> - uint32_t frame_size;
>>> -
>>> ret = rte_eth_dev_info_get(port_id, &dev_info);
>>> if (ret != 0)
>>> return ret;
>>> - if (mtu < dev_info.min_mtu || mtu > dev_info.max_mtu)
>>> - return -EINVAL;
>>> -
>>> - overhead_len =
>>> eth_dev_get_overhead_len(dev_info.max_rx_pktlen,
>>> - dev_info.max_mtu);
>>> - frame_size = mtu + overhead_len;
>>> - if (mtu < RTE_ETHER_MIN_MTU || frame_size >
>>> dev_info.max_rx_pktlen)
>>> - return -EINVAL;
>>> + ret = eth_dev_validate_mtu(port_id, &dev_info, mtu);
>>> + if (ret != 0)
>>> + return ret;
>>> }
>>> ret = (*dev->dev_ops->mtu_set)(dev, mtu);
>>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>>> index 4d0f956a4b28..50e124ff631f 100644
>>> --- a/lib/ethdev/rte_ethdev.h
>>> +++ b/lib/ethdev/rte_ethdev.h
>>> @@ -3056,7 +3056,7 @@ int rte_eth_macaddr_get(uint16_t port_id,
>>> struct rte_ether_addr *mac_addr);
>>> * };
>>> *
>>> * device = dev->device
>>> - * min_mtu = RTE_ETHER_MIN_MTU
>>> + * min_mtu = RTE_ETHER_MIN_LEN - RTE_ETHER_HDR_LEN - RTE_ETHER_CRC_LEN
>>> * max_mtu = UINT16_MAX
>>> *
>>> * The following fields will be populated if support for
>>> dev_infos_get()
>
> .
next prev parent reply other threads:[~2021-10-12 4:02 UTC|newest]
Thread overview: 112+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-09 17:29 [dpdk-dev] [PATCH 1/4] ethdev: fix max Rx packet length Ferruh Yigit
2021-07-09 17:29 ` [dpdk-dev] [PATCH 2/4] ethdev: move jumbo frame offload check to library Ferruh Yigit
2021-07-13 13:48 ` Andrew Rybchenko
2021-07-21 12:26 ` Ferruh Yigit
2021-07-18 7:49 ` Xu, Rosen
2021-07-19 14:38 ` Ajit Khaparde
2021-07-09 17:29 ` [dpdk-dev] [PATCH 3/4] ethdev: move check to library for MTU set Ferruh Yigit
2021-07-13 13:56 ` Andrew Rybchenko
2021-07-18 7:52 ` Xu, Rosen
2021-07-09 17:29 ` [dpdk-dev] [PATCH 4/4] ethdev: remove jumbo offload flag Ferruh Yigit
2021-07-13 14:07 ` Andrew Rybchenko
2021-07-21 12:26 ` Ferruh Yigit
2021-07-21 12:39 ` Ferruh Yigit
2021-07-18 7:53 ` Xu, Rosen
2021-07-13 12:47 ` [dpdk-dev] [PATCH 1/4] ethdev: fix max Rx packet length Andrew Rybchenko
2021-07-21 16:46 ` Ferruh Yigit
2021-07-22 1:31 ` Ajit Khaparde
2021-07-22 10:27 ` Ferruh Yigit
2021-07-22 10:38 ` Andrew Rybchenko
2021-07-18 7:45 ` Xu, Rosen
2021-07-19 3:35 ` Huisong Li
2021-07-21 15:29 ` Ferruh Yigit
2021-07-22 7:21 ` Huisong Li
2021-07-22 10:12 ` Ferruh Yigit
2021-07-22 10:15 ` Andrew Rybchenko
2021-07-22 14:43 ` Stephen Hemminger
2021-09-17 1:08 ` Min Hu (Connor)
2021-09-17 8:04 ` Ferruh Yigit
2021-09-17 8:16 ` Min Hu (Connor)
2021-09-17 8:17 ` Min Hu (Connor)
2021-07-22 17:21 ` [dpdk-dev] [PATCH v2 1/6] " Ferruh Yigit
2021-07-22 17:21 ` [dpdk-dev] [PATCH v2 2/6] ethdev: move jumbo frame offload check to library Ferruh Yigit
2021-07-22 17:21 ` [dpdk-dev] [PATCH v2 3/6] ethdev: move check to library for MTU set Ferruh Yigit
2021-07-22 17:21 ` [dpdk-dev] [PATCH v2 4/6] ethdev: remove jumbo offload flag Ferruh Yigit
2021-07-22 17:21 ` [dpdk-dev] [PATCH v2 5/6] ethdev: unify MTU checks Ferruh Yigit
2021-07-23 3:29 ` Huisong Li
2021-07-22 17:21 ` [dpdk-dev] [PATCH v2 6/6] examples/ip_reassembly: remove unused parameter Ferruh Yigit
2021-10-01 14:36 ` [dpdk-dev] [PATCH v3 1/6] ethdev: fix max Rx packet length Ferruh Yigit
2021-10-01 14:36 ` [dpdk-dev] [PATCH v3 2/6] ethdev: move jumbo frame offload check to library Ferruh Yigit
2021-10-04 5:08 ` Somnath Kotur
2021-10-01 14:36 ` [dpdk-dev] [PATCH v3 3/6] ethdev: move check to library for MTU set Ferruh Yigit
2021-10-04 5:09 ` Somnath Kotur
2021-10-01 14:36 ` [dpdk-dev] [PATCH v3 4/6] ethdev: remove jumbo offload flag Ferruh Yigit
[not found] ` <CAOBf=muYkU2dwgi3iC8Q7pdSNTJsMUwWYdXj14KeN_=_mUGa0w@mail.gmail.com>
2021-10-04 7:55 ` Somnath Kotur
2021-10-05 16:48 ` Ferruh Yigit
2021-10-01 14:36 ` [dpdk-dev] [PATCH v3 5/6] ethdev: unify MTU checks Ferruh Yigit
2021-10-01 14:36 ` [dpdk-dev] [PATCH v3 6/6] examples/ip_reassembly: remove unused parameter Ferruh Yigit
2021-10-01 15:07 ` [dpdk-dev] [PATCH v3 1/6] ethdev: fix max Rx packet length Stephen Hemminger
2021-10-05 16:46 ` Ferruh Yigit
2021-10-05 17:16 ` [dpdk-dev] [PATCH v4 " Ferruh Yigit
2021-10-05 17:16 ` [dpdk-dev] [PATCH v4 2/6] ethdev: move jumbo frame offload check to library Ferruh Yigit
2021-10-08 8:39 ` Xu, Rosen
2021-10-05 17:16 ` [dpdk-dev] [PATCH v4 3/6] ethdev: move check to library for MTU set Ferruh Yigit
2021-10-05 17:16 ` [dpdk-dev] [PATCH v4 4/6] ethdev: remove jumbo offload flag Ferruh Yigit
2021-10-08 8:38 ` Xu, Rosen
2021-10-05 17:16 ` [dpdk-dev] [PATCH v4 5/6] ethdev: unify MTU checks Ferruh Yigit
2021-10-05 17:16 ` [dpdk-dev] [PATCH v4 6/6] examples/ip_reassembly: remove unused parameter Ferruh Yigit
2021-10-05 22:07 ` [dpdk-dev] [PATCH v4 1/6] ethdev: fix max Rx packet length Ajit Khaparde
2021-10-06 6:08 ` Somnath Kotur
2021-10-08 8:36 ` Xu, Rosen
2021-10-10 6:30 ` Matan Azrad
2021-10-11 21:59 ` Ferruh Yigit
2021-10-12 7:03 ` Matan Azrad
2021-10-12 11:03 ` Ferruh Yigit
2021-10-07 16:56 ` [dpdk-dev] [PATCH v5 " Ferruh Yigit
2021-10-07 16:56 ` [dpdk-dev] [PATCH v5 2/6] ethdev: move jumbo frame offload check to library Ferruh Yigit
2021-10-08 17:20 ` Ananyev, Konstantin
2021-10-09 10:58 ` lihuisong (C)
2021-10-07 16:56 ` [dpdk-dev] [PATCH v5 3/6] ethdev: move check to library for MTU set Ferruh Yigit
2021-10-08 17:19 ` Ananyev, Konstantin
2021-10-07 16:56 ` [dpdk-dev] [PATCH v5 4/6] ethdev: remove jumbo offload flag Ferruh Yigit
2021-10-08 17:11 ` Ananyev, Konstantin
2021-10-09 11:09 ` lihuisong (C)
2021-10-10 5:46 ` Matan Azrad
2021-10-07 16:56 ` [dpdk-dev] [PATCH v5 5/6] ethdev: unify MTU checks Ferruh Yigit
2021-10-08 16:51 ` Ananyev, Konstantin
2021-10-11 19:50 ` Ferruh Yigit
2021-10-09 11:43 ` lihuisong (C)
2021-10-11 20:15 ` Ferruh Yigit
2021-10-12 4:02 ` lihuisong (C) [this message]
2021-10-07 16:56 ` [dpdk-dev] [PATCH v5 6/6] examples/ip_reassembly: remove unused parameter Ferruh Yigit
2021-10-08 16:53 ` Ananyev, Konstantin
2021-10-08 15:57 ` [dpdk-dev] [PATCH v5 1/6] ethdev: fix max Rx packet length Ananyev, Konstantin
2021-10-11 19:47 ` Ferruh Yigit
2021-10-09 10:56 ` lihuisong (C)
2021-10-11 23:53 ` [dpdk-dev] [PATCH v6 " Ferruh Yigit
2021-10-11 23:53 ` [dpdk-dev] [PATCH v6 2/6] ethdev: move jumbo frame offload check to library Ferruh Yigit
2021-10-11 23:53 ` [dpdk-dev] [PATCH v6 3/6] ethdev: move check to library for MTU set Ferruh Yigit
2021-10-11 23:53 ` [dpdk-dev] [PATCH v6 4/6] ethdev: remove jumbo offload flag Ferruh Yigit
2021-10-12 17:20 ` Hyong Youb Kim (hyonkim)
2021-10-13 7:16 ` Michał Krawczyk
2021-10-11 23:53 ` [dpdk-dev] [PATCH v6 5/6] ethdev: unify MTU checks Ferruh Yigit
2021-10-12 5:58 ` Andrew Rybchenko
2021-10-11 23:53 ` [dpdk-dev] [PATCH v6 6/6] examples/ip_reassembly: remove unused parameter Ferruh Yigit
2021-10-12 6:02 ` [dpdk-dev] [PATCH v6 1/6] ethdev: fix max Rx packet length Andrew Rybchenko
2021-10-12 9:42 ` Ananyev, Konstantin
2021-10-13 7:08 ` Xu, Rosen
2021-10-15 1:31 ` Hyong Youb Kim (hyonkim)
2021-10-16 0:24 ` Ferruh Yigit
2021-10-18 8:54 ` Ferruh Yigit
2021-10-18 13:48 ` [dpdk-dev] [PATCH v7 " Ferruh Yigit
2021-10-18 13:48 ` [dpdk-dev] [PATCH v7 2/6] ethdev: move jumbo frame offload check to library Ferruh Yigit
2021-10-18 13:48 ` [dpdk-dev] [PATCH v7 3/6] ethdev: move check to library for MTU set Ferruh Yigit
2021-10-18 13:48 ` [dpdk-dev] [PATCH v7 4/6] ethdev: remove jumbo offload flag Ferruh Yigit
2021-10-21 0:43 ` Thomas Monjalon
2021-10-22 11:25 ` Ferruh Yigit
2021-10-22 11:29 ` Andrew Rybchenko
2021-10-18 13:48 ` [dpdk-dev] [PATCH v7 5/6] ethdev: unify MTU checks Ferruh Yigit
2021-10-18 13:48 ` [dpdk-dev] [PATCH v7 6/6] examples/ip_reassembly: remove unused parameter Ferruh Yigit
2021-10-18 17:31 ` [dpdk-dev] [PATCH v7 1/6] ethdev: fix max Rx packet length Ferruh Yigit
2021-11-05 14:19 ` Xueming(Steven) Li
2021-11-05 14:39 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=36f9c73f-596b-0f64-80d2-f800181e6e4e@huawei.com \
--to=lihuisong@huawei.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).