Re: [PATCH v3] ethdev: introduce dump API

["Min Hu (Connor)" <humin29@huawei.com>]

Hi, Ferruh,
	all fixed in v4, please check it, thanks.

在 2022/2/10 20:34, Ferruh Yigit 写道:
> On 2/10/2022 12:32 PM, Ferruh Yigit wrote:
>> On 2/9/2022 1:21 AM, Min Hu (Connor) wrote:
>>> Added the ethdev dump API which provides querying private info from 
>>> ethdev.
>>> There exists many private properties in different PMD drivers, such as
>>> adapter state, Rx/Tx func algorithm in hns3 PMD. The information of 
>>> these
>>> properties is important for debug. As the information is private, the 
>>> new
>>> API is introduced.
>>>
>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>>> Acked-by: Ray Kinsella <mdr@ashroe.eu>
>>> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
>>> ---
>>> v3:
>>> * change 'ethdev' to 'device'
>>> v2:
>>> * fixed comments from Ferruh.
>>> ---
>>>   doc/guides/rel_notes/release_22_03.rst |  7 +++++++
>>>   lib/ethdev/ethdev_driver.h             | 23 +++++++++++++++++++++++
>>>   lib/ethdev/rte_ethdev.c                | 17 +++++++++++++++++
>>>   lib/ethdev/rte_ethdev.h                | 20 ++++++++++++++++++++
>>>   lib/ethdev/version.map                 |  3 +++
>>>   5 files changed, 70 insertions(+)
>>>
>>> diff --git a/doc/guides/rel_notes/release_22_03.rst 
>>> b/doc/guides/rel_notes/release_22_03.rst
>>> index b20716c521..0e3d3ae365 100644
>>> --- a/doc/guides/rel_notes/release_22_03.rst
>>> +++ b/doc/guides/rel_notes/release_22_03.rst
>>> @@ -64,6 +64,13 @@ New Features
>>>       - ``rte_ipv6_udptcp_cksum_mbuf()``
>>>       - ``rte_ipv6_udptcp_cksum_mbuf_verify()``
>>> +* **Added the private dump API, for query private info from device.**
>>> +
>>> +  Added the private dump API which provides querying private info 
>>> from device.
>>> +  There exists many private properties in different PMD drivers. The
>>> +  information of these properties is important for debug. As the 
>>> information
>>> +  is private, the new API is introduced.
>>> +
>>>   * **Updated AF_XDP PMD**
>>>     * Added support for libxdp >=v1.2.2.
>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>> index 7d27781f7d..a0bd066ab3 100644
>>> --- a/lib/ethdev/ethdev_driver.h
>>> +++ b/lib/ethdev/ethdev_driver.h
>>> @@ -990,6 +990,26 @@ typedef int (*eth_representor_info_get_t)(struct 
>>> rte_eth_dev *dev,
>>>   typedef int (*eth_rx_metadata_negotiate_t)(struct rte_eth_dev *dev,
>>>                          uint64_t *features);
>>> +/**
>>> + * @internal
>>> + * Dump private info from device to a file.
>>> + *
>>> + * @param dev
>>> + *   Port (ethdev) handle.
>>> + * @param file
>>> + *   A pointer to a file for output.
>>> + *
>>> + * @retval 0
>>> + *   Success
>>> + * @retval -EINVAL
>>> + *   Invalid file
>>> + * @retval -ENOTSUP
>>> + *   Not supported
>>> + * @retval -ENODEV
>>> + *   Invalid port_id
>>
>> For dev_ops (eth_dev_priv_dump_t), I don't think '-ENOTSUP' and '-ENODEV'
>> are valid errors, those cases covered by API before dev_ops called.
>>
>> Also '@return' is missing.
>>
>> What about below:
>>    *
>>    * @return
>>    *   Negative value on error, 0 on success.
>>    *
>>    * @retval 0
>>    *   Success
>>    * @retval -EINVAL
>>    *   Invalid file
>>    */
>>
>>> + */
>>> +typedef int (*eth_dev_priv_dump_t)(struct rte_eth_dev *dev, FILE 
>>> *file);
>>> +
>>>   /**
>>>    * @internal A structure containing the functions exported by an 
>>> Ethernet driver.
>>>    */
>>> @@ -1186,6 +1206,9 @@ struct eth_dev_ops {
>>>        * kinds of metadata to the PMD
>>>        */
>>>       eth_rx_metadata_negotiate_t rx_metadata_negotiate;
>>> +
>>> +    /** Dump private info from device */
>>> +    eth_dev_priv_dump_t eth_dev_priv_dump;
>>>   };
>>>   /**
>>> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
>>> index 917a320afa..5ee316a3c0 100644
>>> --- a/lib/ethdev/rte_ethdev.c
>>> +++ b/lib/ethdev/rte_ethdev.c
>>> @@ -6485,6 +6485,23 @@ rte_eth_rx_metadata_negotiate(uint16_t 
>>> port_id, uint64_t *features)
>>>                  (*dev->dev_ops->rx_metadata_negotiate)(dev, features));
>>>   }
>>> +int
>>> +rte_eth_dev_priv_dump(uint16_t port_id, FILE *file)
>>> +{
>>> +    struct rte_eth_dev *dev;
>>> +
>>> +    RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>>> +    dev = &rte_eth_devices[port_id];
>>> +
>>> +    if (file == NULL) {
>>> +        RTE_ETHDEV_LOG(ERR, "Invalid file (NULL)\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->eth_dev_priv_dump, 
>>> -ENOTSUP);
>>> +    return eth_err(port_id, (*dev->dev_ops->eth_dev_priv_dump)(dev, 
>>> file));
>>> +}
>>> +
>>
>> Can you please move the function up in the file?
>> Bottom of the file is for static inline functions, APIs are above the
>> "#include <rte_ethdev_core.h>" line, can you move the function above
>> that?
>>
>>>   RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO);
>>>   RTE_INIT(ethdev_init_telemetry)
>>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
>>> index 147cc1ced3..e6ea294402 100644
>>> --- a/lib/ethdev/rte_ethdev.h
>>> +++ b/lib/ethdev/rte_ethdev.h
>>> @@ -5902,6 +5902,26 @@ rte_eth_tx_buffer(uint16_t port_id, uint16_t 
>>> queue_id,
>>>       return rte_eth_tx_buffer_flush(port_id, queue_id, buffer);
>>>   }
>>> +/**
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change, or be removed, without 
>>> prior notice
>>> + *
>>> + * Dump private info from device to a file. Provided data and the 
>>> order depends
>>> + * on the PMD.
>>> + *
>>> + * @param port_id
>>> + *   The port identifier of the Ethernet device.
>>> + * @param file
>>> + *   A pointer to a file for output.
>>> + * @return
>>> + *   - (-ENODEV) if *port_id* is invalid.
>>> + *   - (-EINVAL) if null file.
>>> + *   - (-ENOTSUP) if the device does not support this function.
>>> + *   - (0) on success
>>
>> Can you please list "(0) on success" to be consistent with (most :( of 
>> the) other comments?
>>
> 
> Can you please list "(0) on success" as first item ...
> 
> * @return
> *   - (0) on success
> *   - (-ENODEV) if *port_id* is invalid.
> *   - (-EINVAL) if null file.
> ...
> 
>> Also need to add "(-EIO) if device is removed." too, which can be
>> returned by 'eth_err()'.
>>
>>
>>> + */
>>> +__rte_experimental
>>> +int rte_eth_dev_priv_dump(uint16_t port_id, FILE *file);
>>> +
>>>   #ifdef __cplusplus
>>>   }
>>>   #endif
>>> diff --git a/lib/ethdev/version.map b/lib/ethdev/version.map
>>> index 1f7359c846..376ea139aa 100644
>>> --- a/lib/ethdev/version.map
>>> +++ b/lib/ethdev/version.map
>>> @@ -256,6 +256,9 @@ EXPERIMENTAL {
>>>       rte_flow_flex_item_create;
>>>       rte_flow_flex_item_release;
>>>       rte_flow_pick_transfer_proxy;
>>> +
>>> +    # added in 22.03
>>> +    rte_eth_dev_priv_dump;
>>
>> next version needs to be rebased on top of next-net, because of other
>> APIs merged, fyi
>>
>>>   };
>>>   INTERNAL {
>>
> 
> .