From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id BEE6AA034F; Tue, 12 Oct 2021 06:02:57 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3BB99410E4; Tue, 12 Oct 2021 06:02:57 +0200 (CEST) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 2EC40410DC for ; Tue, 12 Oct 2021 06:02:55 +0200 (CEST) Received: from dggemv711-chm.china.huawei.com (unknown [172.30.72.53]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4HT22T65Grzbmjx; Tue, 12 Oct 2021 11:58:25 +0800 (CST) Received: from dggema767-chm.china.huawei.com (10.1.198.209) by dggemv711-chm.china.huawei.com (10.1.198.66) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.2308.8; Tue, 12 Oct 2021 12:02:52 +0800 Received: from [10.67.103.231] (10.67.103.231) by dggema767-chm.china.huawei.com (10.1.198.209) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id 15.1.2308.8; Tue, 12 Oct 2021 12:02:52 +0800 Message-ID: <36f9c73f-596b-0f64-80d2-f800181e6e4e@huawei.com> Date: Tue, 12 Oct 2021 12:02:52 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 To: Ferruh Yigit CC: , Thomas Monjalon , Andrew Rybchenko References: <20211001143624.3744505-1-ferruh.yigit@intel.com> <20211007165626.2941995-1-ferruh.yigit@intel.com> <20211007165626.2941995-5-ferruh.yigit@intel.com> <5f1104a7-2e57-d95d-447d-81616f5dc8ea@huawei.com> <8afc3490-84b1-37e8-6261-ec535cf98e80@intel.com> From: "lihuisong (C)" In-Reply-To: <8afc3490-84b1-37e8-6261-ec535cf98e80@intel.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.231] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To dggema767-chm.china.huawei.com (10.1.198.209) X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH v5 5/6] ethdev: unify MTU checks X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 在 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 >>> Signed-off-by: Ferruh Yigit >>> --- >>>   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() > > .