[v5,5/6] ethdev: unify MTU checks
Checks
Commit Message
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(-)
Comments
> 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;
Again, I would just always use 32-bit arithmetic - safe and easy.
Apart from that:
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> + 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;
> + }
> +
> + 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;
> 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()
> --
> 2.31.1
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.
As modified by this patch, dev_info->min_mtu is calculated based on
RTE_ETHER_MIN_LEN.
> +
> + 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.
> 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()
On 10/8/2021 5:51 PM, Ananyev, Konstantin wrote:
>
>
>> 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;
>
> Again, I would just always use 32-bit arithmetic - safe and easy.
ack
> Apart from that:
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>
<...>
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.
> 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.
>> +
>> + 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()
在 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()
>
> .
@@ -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;
+ }
+
+ 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;
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);
@@ -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()