[PATCH v4 1/3] ethdev: introduce maximum Rx buffer size

[lihuisong (C)]

在 2023/11/3 11:30, Ferruh Yigit 写道:
> On 11/3/2023 2:21 AM, lihuisong (C) wrote:
>> 在 2023/11/3 0:35, Ferruh Yigit 写道:
>>> On 11/2/2023 12:16 PM, Huisong Li wrote:
>>>> The "min_rx_bufsize" in struct rte_eth_dev_info stands for the minimum
>>>> Rx buffer size supported by hardware. Actually, some engines also have
>>>> the maximum Rx buffer specification, like, hns3, i40e and so on. If mbuf
>>>> data room size in mempool is greater then the maximum Rx buffer size
>>>> per descriptor supported by HW, the data size application used in each
>>>> mbuf is just as much as the maximum Rx buffer size instead of the whole
>>>> data room size.
>>>>
>>>> So introduce maximum Rx buffer size which is not enforced just to
>>>> report user to avoid memory waste. In addition, fix the comment for
>>>> the "min_rx_bufsize" to make it be more specific.
>>>>
>>>> Signed-off-by: Huisong Li <lihuisong at huawei.com>
>>>> Acked-by: Chengwen Feng <fengchengwen at huawei.com>
>>>> Acked-by: Morten Brørup <mb at smartsharesystems.com>
>>>> ---
>>>>    doc/guides/rel_notes/release_23_11.rst |  7 +++++++
>>>>    lib/ethdev/rte_ethdev.c                |  7 +++++++
>>>>    lib/ethdev/rte_ethdev.h                | 10 +++++++++-
>>>>    3 files changed, 23 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/doc/guides/rel_notes/release_23_11.rst
>>>> b/doc/guides/rel_notes/release_23_11.rst
>>>> index 95db98d098..d4f7d5b266 100644
>>>> --- a/doc/guides/rel_notes/release_23_11.rst
>>>> +++ b/doc/guides/rel_notes/release_23_11.rst
>>>> @@ -122,6 +122,13 @@ New Features
>>>>      a group's miss actions, which are the actions to be performed on
>>>> packets
>>>>      that didn't match any of the flow rules in the group.
>>>>    +* **Added maximum Rx buffer size to report.**
>>>> +
>>>> +  Introduced the ``max_rx_bufsize`` field representing the maximum Rx
>>>> +  buffer size per descriptor supported by HW in structure
>>>> ``rte_eth_dev_info``
>>>> +  to report user and to avoid wasting space of mempool.
>>>> +  Its value is UINT32_MAX if driver doesn't report it.
>>>> +
>>>>    * **Updated Amazon ena (Elastic Network Adapter) net driver.**
>>>>        * Upgraded ENA HAL to latest version.
>>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>>> index af23ac0ad0..03539bb6c3 100644
>>>> --- a/lib/ethdev/rte_ethdev.c
>>>> +++ b/lib/ethdev/rte_ethdev.c
>>>> @@ -2112,6 +2112,7 @@ rte_eth_rx_queue_setup(uint16_t port_id,
>>>> uint16_t rx_queue_id,
>>>>        struct rte_eth_dev *dev;
>>>>        struct rte_eth_dev_info dev_info;
>>>>        struct rte_eth_rxconf local_conf;
>>>> +    uint32_t vld_bufsize;
>>>>    
>>> I guess 'vld' is valid, but that is not accurate. Can we rename it
>>> something like 'buf_data_size'?
>> Ack
>>>>        RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>>>        dev = &rte_eth_devices[port_id];
>>>> @@ -2158,6 +2159,11 @@ rte_eth_rx_queue_setup(uint16_t port_id,
>>>> uint16_t rx_queue_id,
>>>>                return ret;
>>>>              mbp_buf_size = rte_pktmbuf_data_room_size(mp);
>>>> +        vld_bufsize = mbp_buf_size - RTE_PKTMBUF_HEADROOM;
>>>> +        if (vld_bufsize > dev_info.max_rx_bufsize)
>>>> +            RTE_ETHDEV_LOG(INFO,
>>>> +                "For port_id=%u the max data size per mbuf in Rx is
>>>> %u instead of the whole data room(%u).\n",
>>>> +                port_id, dev_info.max_rx_bufsize, vld_bufsize);
>>>>
>>> This log gives some information, but it is not clear what user should
>>> do. My concern is this can confuse end users (not developers but users
>>> of dpdk application).
>>>
>>> Since audience of this message is DPDK application developer, most of
>>> the times end user don't have anything to do with this log, what do you
>>> think to change log level to 'debug'?
>> Ack
>>> Also can we add some action to it, something like:
>>> "mbuf data buffer size is bigger than what device can utilize, consider
>>> reducing mbuf size."
>> What do you think of this log, Ferruh?
>> "The mbuf data buffer size (%u) is bigger than the max size (%u) device
>> with port_id=%u can utilize, consider reducing mbuf size.\n",
>>
> Can move port_id to the beginning, and can make wording passive, what about:
>
> "For port_id=%u, the mbuf data buffer size (%u) is bigger than max
> buffer size (%u) device can utilize, mbuf size can be reduced.\n"
Ok, thanks for you advice.
>
>>>
>>>
>>>>        } else if (rx_conf != NULL && rx_conf->rx_nseg > 0) {
>>>>            const struct rte_eth_rxseg_split *rx_seg;
>>>>            uint16_t n_seg;
>>>> @@ -3757,6 +3763,7 @@ rte_eth_dev_info_get(uint16_t port_id, struct
>>>> rte_eth_dev_info *dev_info)
>>>>        dev_info->min_mtu = RTE_ETHER_MIN_LEN - RTE_ETHER_HDR_LEN -
>>>>            RTE_ETHER_CRC_LEN;
>>>>        dev_info->max_mtu = UINT16_MAX;
>>>> +    dev_info->max_rx_bufsize = UINT32_MAX;
>>>>          if (*dev->dev_ops->dev_infos_get == NULL)
>>>>            return -ENOTSUP;
>>>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>>>> index a53dd5a1ef..7133b73d26 100644
>>>> --- a/lib/ethdev/rte_ethdev.h
>>>> +++ b/lib/ethdev/rte_ethdev.h
>>>> @@ -1723,7 +1723,15 @@ struct rte_eth_dev_info {
>>>>        uint16_t min_mtu;    /**< Minimum MTU allowed */
>>>>        uint16_t max_mtu;    /**< Maximum MTU allowed */
>>>>        const uint32_t *dev_flags; /**< Device flags */
>>>> -    uint32_t min_rx_bufsize; /**< Minimum size of Rx buffer. */
>>>> +    /** Minimum Rx buffer size per descriptor supported by HW. */
>>>> +    uint32_t min_rx_bufsize;
>>>> +    /**
>>>> +     * Maximum Rx buffer size per descriptor supported by HW.
>>>> +     * The value is not enforced, information only to application to
>>>> +     * optimize mbuf size. Its value is UINT32_MAX when not specified
>>>> +     * by the driver.
>>>> +     */
>>>> +    uint32_t max_rx_bufsize;
>>>>        uint32_t max_rx_pktlen; /**< Maximum configurable length of Rx
>>>> pkt. */
>>>>        /** Maximum configurable size of LRO aggregated packet. */
>>>>        uint32_t max_lro_pkt_size;
>>> .
> .