ethdev: introduce ethdev dump API
Checks
Commit Message
Added the ethdev dump API which provides functions for query private info
from device. 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>
---
doc/guides/rel_notes/release_22_03.rst | 7 +++++++
lib/ethdev/ethdev_driver.h | 17 +++++++++++++++++
lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
lib/ethdev/rte_ethdev.h | 16 ++++++++++++++++
lib/ethdev/version.map | 3 +++
5 files changed, 58 insertions(+)
Comments
On 2/7/2022 1:47 AM, Min Hu (Connor) wrote:
> Added the ethdev dump API which provides functions for query private info
Isn't API and function are same thing in this contexts?
> from device. 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.>
In the patch title 'ethdev' is duplicated, can you fix it?
> 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>
> ---
> doc/guides/rel_notes/release_22_03.rst | 7 +++++++
> lib/ethdev/ethdev_driver.h | 17 +++++++++++++++++
> lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
> lib/ethdev/rte_ethdev.h | 16 ++++++++++++++++
> lib/ethdev/version.map | 3 +++
> 5 files changed, 58 insertions(+)
>
> diff --git a/doc/guides/rel_notes/release_22_03.rst b/doc/guides/rel_notes/release_22_03.rst
> index b20716c521..5895194cde 100644
> --- a/doc/guides/rel_notes/release_22_03.rst
> +++ b/doc/guides/rel_notes/release_22_03.rst
> @@ -92,6 +92,13 @@ New Features
> * Called ``rte_ipv4/6_udptcp_cksum_mbuf()`` functions in testpmd csum mode
> to support software UDP/TCP checksum over multiple segments.
>
> +* **Added the private ethdev dump API, for query private info of ethdev.**
> +
> + Added the private ethdev dump API which provides functions for query
> + 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.
> +
>
Can you please move the update up, just above the ethdev PMD updates?
> Removed Items
> -------------
> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index 7d27781f7d..d41d8a2c9d 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -990,6 +990,20 @@ 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 ethdev private info to a file.
> + *
It doesn't dump the 'ethdev' private info, it dumps the private info from device.
> + * @param file
> + * A pointer to a file for output.
> + * @param dev
> + * Port (ethdev) handle.
> + *
> + * @return
> + * Negative errno value on error, positive value on success.
> + */
> +typedef int (*eth_dev_priv_dump_t)(FILE *file, struct rte_eth_dev *dev);
> +
> /**
> * @internal A structure containing the functions exported by an Ethernet driver.
> */
> @@ -1186,6 +1200,9 @@ struct eth_dev_ops {
> * kinds of metadata to the PMD
> */
> eth_rx_metadata_negotiate_t rx_metadata_negotiate;
> +
> + /** Dump ethdev private info */
Ditto, not 'ethdev' private info.
> + 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..07838cc2d9 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -6485,6 +6485,21 @@ 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(FILE *file, uint16_t port_id)
> +{
> + struct rte_eth_dev *dev;
> + int ret;
> +
> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> + dev = &rte_eth_devices[port_id];
> +
What do you think to check 'file' pointer against NULL before passing it to
dev_ops?
> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->eth_dev_priv_dump, -ENOTSUP);
> + ret = (*dev->dev_ops->eth_dev_priv_dump)(file, dev);
Should 'eth_err()' used here?
> +
> + return ret;
> +}
> +
> 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..1c00fdbcd2 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -5902,6 +5902,22 @@ 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 ethdev private info to a file.
> + *
Ditto.
Should we highlight that provided data and the order depends on the PMD?
> + * @param file
> + * A pointer to a file for output.
> + * @param port_id
> + * The port identifier of the Ethernet device.
> + * @return
> + * Negative errno value on error, positive value on success.
What does 'errno' mean here, does the API cause errno to be set and return it?
Also it is possible to list possible specific error values, as done with other
API documentation, like:
(0) if successful.
(-ENODEV) if *port_id* is invalid.
(-ENOTSUP) if hardware doesn't support.
....
> + */
> +__rte_experimental
> +int rte_eth_dev_priv_dump(FILE *file, uint16_t port_id);
> +
What do you think to have the 'port_id' as first argument to be consistent
with the other APIs?
> #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;
> };
>
> INTERNAL {
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Monday, 7 February 2022 12.46
>
> On 2/7/2022 1:47 AM, Min Hu (Connor) wrote:
> > Added the ethdev dump API which provides functions for query private
> info
>
> Isn't API and function are same thing in this contexts?
>
> > from device. 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.>
>
> In the patch title 'ethdev' is duplicated, can you fix it?
>
>
> > 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>
[...]
> > @@ -990,6 +990,20 @@ 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 ethdev private info to a file.
> > + *
>
> It doesn't dump the 'ethdev' private info, it dumps the private info
> from device.
It seems perfectly clear to me. How would you prefer it phrased instead?
[...]
>
> > + */
> > +__rte_experimental
> > +int rte_eth_dev_priv_dump(FILE *file, uint16_t port_id);
> > +
>
> What do you think to have the 'port_id' as first argument to be
> consistent
> with the other APIs?
The _dump APIs in other libraries have the file pointer as the first parameter, so let's follow that convention here too. No need to move the port_id parameter here.
Only rte_dma_dump() has the file pointer last, and I didn't catch it when the function was defined.
On 2/7/2022 12:18 PM, Morten Brørup wrote:
>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>> Sent: Monday, 7 February 2022 12.46
>>
>> On 2/7/2022 1:47 AM, Min Hu (Connor) wrote:
>>> Added the ethdev dump API which provides functions for query private
>> info
>>
>> Isn't API and function are same thing in this contexts?
>>
>>> from device. 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.>
>>
>> In the patch title 'ethdev' is duplicated, can you fix it?
>>
>>
>>> 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>
>
> [...]
>
>>> @@ -990,6 +990,20 @@ 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 ethdev private info to a file.
>>> + *
>>
>> It doesn't dump the 'ethdev' private info, it dumps the private info
>> from device.
>
> It seems perfectly clear to me. How would you prefer it phrased instead?
>
What described in the document is more accurate,
"query private info from device".
What we are dumping here is not ethdev private info, it is device private info,
and we really don't know what that data may be in the ethdev layer.
Also there is a chance that 'ethdev private info' can be confused with
'ethdev->data->dev_private'
> [...]
>
>>
>>> + */
>>> +__rte_experimental
>>> +int rte_eth_dev_priv_dump(FILE *file, uint16_t port_id);
>>> +
>>
>> What do you think to have the 'port_id' as first argument to be
>> consistent
>> with the other APIs?
>
> The _dump APIs in other libraries have the file pointer as the first parameter, so let's follow that convention here too. No need to move the port_id parameter here.
>
Yes, for most of the _dump() APIs, file pointer seems is the first argument,
bu they are from various libraries.
Within the ethdev APIs, I think it makes sense that all APIs start with
'port_id' parameter for consistency, like done in:
rte_flow_dev_dump(uint16_t port_id, ...)
> Only rte_dma_dump() has the file pointer last, and I didn't catch it when the function was defined.
>
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Monday, 7 February 2022 13.36
>
> On 2/7/2022 12:18 PM, Morten Brørup wrote:
> >> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> >> Sent: Monday, 7 February 2022 12.46
> >>
> >> On 2/7/2022 1:47 AM, Min Hu (Connor) wrote:
> >>> Added the ethdev dump API which provides functions for query
> private
> >> info
> >>
> >> Isn't API and function are same thing in this contexts?
> >>
> >>> from device. 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.>
> >>
> >> In the patch title 'ethdev' is duplicated, can you fix it?
> >>
> >>
> >>> 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>
> >
> > [...]
> >
> >>> @@ -990,6 +990,20 @@ 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 ethdev private info to a file.
> >>> + *
> >>
> >> It doesn't dump the 'ethdev' private info, it dumps the private info
> >> from device.
> >
> > It seems perfectly clear to me. How would you prefer it phrased
> instead?
> >
>
> What described in the document is more accurate,
> "query private info from device".
>
> What we are dumping here is not ethdev private info, it is device
> private info,
> and we really don't know what that data may be in the ethdev layer.
>
> Also there is a chance that 'ethdev private info' can be confused with
> 'ethdev->data->dev_private'
OK. Now I got your point! The difference is very subtle.
>
> > [...]
> >
> >>
> >>> + */
> >>> +__rte_experimental
> >>> +int rte_eth_dev_priv_dump(FILE *file, uint16_t port_id);
> >>> +
> >>
> >> What do you think to have the 'port_id' as first argument to be
> >> consistent
> >> with the other APIs?
> >
> > The _dump APIs in other libraries have the file pointer as the first
> parameter, so let's follow that convention here too. No need to move
> the port_id parameter here.
> >
>
> Yes, for most of the _dump() APIs, file pointer seems is the first
> argument,
> bu they are from various libraries.
>
> Within the ethdev APIs, I think it makes sense that all APIs start with
> 'port_id' parameter for consistency, like done in:
> rte_flow_dev_dump(uint16_t port_id, ...)
>
> > Only rte_dma_dump() has the file pointer last, and I didn't catch it
> when the function was defined.
> >
OK. Then I agree with you about following the convention like rte_flow_dev_dump() with the port_id first.
I even think Connor got it right the first time, and I proposed following the other convention.
It's not easy when there are two opposite conventions. :-)
-Morten
On 2/7/2022 12:56 PM, Morten Brørup wrote:
>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>> Sent: Monday, 7 February 2022 13.36
>>
>> On 2/7/2022 12:18 PM, Morten Brørup wrote:
>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>> Sent: Monday, 7 February 2022 12.46
>>>>
>>>> On 2/7/2022 1:47 AM, Min Hu (Connor) wrote:
>>>>> Added the ethdev dump API which provides functions for query
>> private
>>>> info
>>>>
>>>> Isn't API and function are same thing in this contexts?
>>>>
>>>>> from device. 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.>
>>>>
>>>> In the patch title 'ethdev' is duplicated, can you fix it?
>>>>
>>>>
>>>>> 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>
>>>
>>> [...]
>>>
>>>>> @@ -990,6 +990,20 @@ 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 ethdev private info to a file.
>>>>> + *
>>>>
>>>> It doesn't dump the 'ethdev' private info, it dumps the private info
>>>> from device.
>>>
>>> It seems perfectly clear to me. How would you prefer it phrased
>> instead?
>>>
>>
>> What described in the document is more accurate,
>> "query private info from device".
>>
>> What we are dumping here is not ethdev private info, it is device
>> private info,
>> and we really don't know what that data may be in the ethdev layer.
>>
>> Also there is a chance that 'ethdev private info' can be confused with
>> 'ethdev->data->dev_private'
>
> OK. Now I got your point! The difference is very subtle.
>
>>
>>> [...]
>>>
>>>>
>>>>> + */
>>>>> +__rte_experimental
>>>>> +int rte_eth_dev_priv_dump(FILE *file, uint16_t port_id);
>>>>> +
>>>>
>>>> What do you think to have the 'port_id' as first argument to be
>>>> consistent
>>>> with the other APIs?
>>>
>>> The _dump APIs in other libraries have the file pointer as the first
>> parameter, so let's follow that convention here too. No need to move
>> the port_id parameter here.
>>>
>>
>> Yes, for most of the _dump() APIs, file pointer seems is the first
>> argument,
>> bu they are from various libraries.
>>
>> Within the ethdev APIs, I think it makes sense that all APIs start with
>> 'port_id' parameter for consistency, like done in:
>> rte_flow_dev_dump(uint16_t port_id, ...)
>>
>>> Only rte_dma_dump() has the file pointer last, and I didn't catch it
>> when the function was defined.
>>>
>
> OK. Then I agree with you about following the convention like rte_flow_dev_dump() with the port_id first.
>
> I even think Connor got it right the first time, and I proposed following the other convention.
>
Ahh, may bad I missed that, sorry for not commenting on time.
> It's not easy when there are two opposite conventions. :-)
>
Yep, that is the main issue.
Hi, Ferruh,
在 2022/2/7 23:35, Ferruh Yigit 写道:
> On 2/7/2022 12:56 PM, Morten Brørup wrote:
>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>> Sent: Monday, 7 February 2022 13.36
>>>
>>> On 2/7/2022 12:18 PM, Morten Brørup wrote:
>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>>> Sent: Monday, 7 February 2022 12.46
>>>>>
>>>>> On 2/7/2022 1:47 AM, Min Hu (Connor) wrote:
>>>>>> Added the ethdev dump API which provides functions for query
>>> private
>>>>> info
>>>>>
>>>>> Isn't API and function are same thing in this contexts?
>>>>>
>>>>>> from device. 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.>
>>>>>
>>>>> In the patch title 'ethdev' is duplicated, can you fix it?
>>>>>
>>>>>
>>>>>> 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>
>>>>
>>>> [...]
>>>>
>>>>>> @@ -990,6 +990,20 @@ 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 ethdev private info to a file.
>>>>>> + *
>>>>>
>>>>> It doesn't dump the 'ethdev' private info, it dumps the private info
>>>>> from device.
>>>>
>>>> It seems perfectly clear to me. How would you prefer it phrased
>>> instead?
>>>>
>>>
>>> What described in the document is more accurate,
>>> "query private info from device".
>>>
>>> What we are dumping here is not ethdev private info, it is device
>>> private info,
what is the difference between ethdev and device?
>>> and we really don't know what that data may be in the ethdev layer.
>>>
>>> Also there is a chance that 'ethdev private info' can be confused with
>>> 'ethdev->data->dev_private'
what I want to dump is exactly the 'ethdev->data->dev_private'.
'ethdev private info' means 'ethdev->data->dev_private'.
why confused?
>>
>> OK. Now I got your point! The difference is very subtle.
>>
>>>
>>>> [...]
>>>>
>>>>>
>>>>>> + */
>>>>>> +__rte_experimental
>>>>>> +int rte_eth_dev_priv_dump(FILE *file, uint16_t port_id);
>>>>>> +
>>>>>
>>>>> What do you think to have the 'port_id' as first argument to be
>>>>> consistent
>>>>> with the other APIs?
>>>>
>>>> The _dump APIs in other libraries have the file pointer as the first
>>> parameter, so let's follow that convention here too. No need to move
>>> the port_id parameter here.
>>>>
>>>
>>> Yes, for most of the _dump() APIs, file pointer seems is the first
>>> argument,
>>> bu they are from various libraries.
>>>
>>> Within the ethdev APIs, I think it makes sense that all APIs start with
>>> 'port_id' parameter for consistency, like done in:
>>> rte_flow_dev_dump(uint16_t port_id, ...)
>>>
>>>> Only rte_dma_dump() has the file pointer last, and I didn't catch it
>>> when the function was defined.
>>>>
>>
>> OK. Then I agree with you about following the convention like
>> rte_flow_dev_dump() with the port_id first.
>>
>> I even think Connor got it right the first time, and I proposed
>> following the other convention.
>>
>
> Ahh, may bad I missed that, sorry for not commenting on time.
>
>
>> It's not easy when there are two opposite conventions. :-)
>>
>
> Yep, that is the main issue.
>
> .
HI, Ferruh,
v2 has been sent according comments, please check it, thanks.
在 2022/2/7 19:46, Ferruh Yigit 写道:
> On 2/7/2022 1:47 AM, Min Hu (Connor) wrote:
>> Added the ethdev dump API which provides functions for query private info
>
> Isn't API and function are same thing in this contexts?
>
>> from device. 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.>
>
> In the patch title 'ethdev' is duplicated, can you fix it?
>
>
>> 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>
>> ---
>> doc/guides/rel_notes/release_22_03.rst | 7 +++++++
>> lib/ethdev/ethdev_driver.h | 17 +++++++++++++++++
>> lib/ethdev/rte_ethdev.c | 15 +++++++++++++++
>> lib/ethdev/rte_ethdev.h | 16 ++++++++++++++++
>> lib/ethdev/version.map | 3 +++
>> 5 files changed, 58 insertions(+)
>>
>> diff --git a/doc/guides/rel_notes/release_22_03.rst
>> b/doc/guides/rel_notes/release_22_03.rst
>> index b20716c521..5895194cde 100644
>> --- a/doc/guides/rel_notes/release_22_03.rst
>> +++ b/doc/guides/rel_notes/release_22_03.rst
>> @@ -92,6 +92,13 @@ New Features
>> * Called ``rte_ipv4/6_udptcp_cksum_mbuf()`` functions in testpmd
>> csum mode
>> to support software UDP/TCP checksum over multiple segments.
>> +* **Added the private ethdev dump API, for query private info of
>> ethdev.**
>> +
>> + Added the private ethdev dump API which provides functions for query
>> + 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.
>> +
>
> Can you please move the update up, just above the ethdev PMD updates?
>
>> Removed Items
>> -------------
>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>> index 7d27781f7d..d41d8a2c9d 100644
>> --- a/lib/ethdev/ethdev_driver.h
>> +++ b/lib/ethdev/ethdev_driver.h
>> @@ -990,6 +990,20 @@ 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 ethdev private info to a file.
>> + *
>
> It doesn't dump the 'ethdev' private info, it dumps the private info
> from device.
>
>> + * @param file
>> + * A pointer to a file for output.
>> + * @param dev
>> + * Port (ethdev) handle.
>> + *
>> + * @return
>> + * Negative errno value on error, positive value on success.
>> + */
>> +typedef int (*eth_dev_priv_dump_t)(FILE *file, struct rte_eth_dev *dev);
>> +
>> /**
>> * @internal A structure containing the functions exported by an
>> Ethernet driver.
>> */
>> @@ -1186,6 +1200,9 @@ struct eth_dev_ops {
>> * kinds of metadata to the PMD
>> */
>> eth_rx_metadata_negotiate_t rx_metadata_negotiate;
>> +
>> + /** Dump ethdev private info */
>
> Ditto, not 'ethdev' private info.
>
>> + 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..07838cc2d9 100644
>> --- a/lib/ethdev/rte_ethdev.c
>> +++ b/lib/ethdev/rte_ethdev.c
>> @@ -6485,6 +6485,21 @@ 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(FILE *file, uint16_t port_id)
>> +{
>> + struct rte_eth_dev *dev;
>> + int ret;
>> +
>> + RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> + dev = &rte_eth_devices[port_id];
>> +
>
> What do you think to check 'file' pointer against NULL before passing it to
> dev_ops?
>
>> + RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->eth_dev_priv_dump, -ENOTSUP);
>> + ret = (*dev->dev_ops->eth_dev_priv_dump)(file, dev);
>
> Should 'eth_err()' used here?
>
>> +
>> + return ret;
>> +}
>> +
>> 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..1c00fdbcd2 100644
>> --- a/lib/ethdev/rte_ethdev.h
>> +++ b/lib/ethdev/rte_ethdev.h
>> @@ -5902,6 +5902,22 @@ 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 ethdev private info to a file.
>> + *
>
> Ditto.
>
> Should we highlight that provided data and the order depends on the PMD?
>
>> + * @param file
>> + * A pointer to a file for output.
>> + * @param port_id
>> + * The port identifier of the Ethernet device.
>> + * @return
>> + * Negative errno value on error, positive value on success.
>
>
> What does 'errno' mean here, does the API cause errno to be set and
> return it?
>
>
> Also it is possible to list possible specific error values, as done with
> other
> API documentation, like:
> (0) if successful.
> (-ENODEV) if *port_id* is invalid.
> (-ENOTSUP) if hardware doesn't support.
> ....
>
>> + */
>> +__rte_experimental
>> +int rte_eth_dev_priv_dump(FILE *file, uint16_t port_id);
>> +
>
> What do you think to have the 'port_id' as first argument to be consistent
> with the other APIs?
>
>> #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;
>> };
>> INTERNAL {
>
> .
On 2/8/2022 12:39 AM, Min Hu (Connor) wrote:
> Hi, Ferruh,
>
> 在 2022/2/7 23:35, Ferruh Yigit 写道:
>> On 2/7/2022 12:56 PM, Morten Brørup wrote:
>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>> Sent: Monday, 7 February 2022 13.36
>>>>
>>>> On 2/7/2022 12:18 PM, Morten Brørup wrote:
>>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>>>> Sent: Monday, 7 February 2022 12.46
>>>>>>
>>>>>> On 2/7/2022 1:47 AM, Min Hu (Connor) wrote:
>>>>>>> Added the ethdev dump API which provides functions for query
>>>> private
>>>>>> info
>>>>>>
>>>>>> Isn't API and function are same thing in this contexts?
>>>>>>
>>>>>>> from device. 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.>
>>>>>>
>>>>>> In the patch title 'ethdev' is duplicated, can you fix it?
>>>>>>
>>>>>>
>>>>>>> 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>
>>>>>
>>>>> [...]
>>>>>
>>>>>>> @@ -990,6 +990,20 @@ 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 ethdev private info to a file.
>>>>>>> + *
>>>>>>
>>>>>> It doesn't dump the 'ethdev' private info, it dumps the private info
>>>>>> from device.
>>>>>
>>>>> It seems perfectly clear to me. How would you prefer it phrased
>>>> instead?
>>>>>
>>>>
>>>> What described in the document is more accurate,
>>>> "query private info from device".
>>>>
>>>> What we are dumping here is not ethdev private info, it is device
>>>> private info,
>
> what is the difference between ethdev and device?
It is not very clear, but for me 'ethdev' is refers to device abstract
layer (ethdev library) specific private data and device refers to ethdev
device (PMD) private data. ethdev is common for all drivers.
>>>> and we really don't know what that data may be in the ethdev layer.
>>>>
>>>> Also there is a chance that 'ethdev private info' can be confused with
>>>> 'ethdev->data->dev_private'
> what I want to dump is exactly the 'ethdev->data->dev_private'.
> 'ethdev private info' means 'ethdev->data->dev_private'.
> why confused?
What I understand was this API can return any device private information,
it is not limited to 'ethdev->data->dev_private', (although most of the data
is represented in this struct), like if you want to dump queue state,
this is out of 'ethdev->data->dev_private'.
>>>
>>> OK. Now I got your point! The difference is very subtle.
>>>
>>>>
>>>>> [...]
>>>>>
>>>>>>
>>>>>>> + */
>>>>>>> +__rte_experimental
>>>>>>> +int rte_eth_dev_priv_dump(FILE *file, uint16_t port_id);
>>>>>>> +
>>>>>>
>>>>>> What do you think to have the 'port_id' as first argument to be
>>>>>> consistent
>>>>>> with the other APIs?
>>>>>
>>>>> The _dump APIs in other libraries have the file pointer as the first
>>>> parameter, so let's follow that convention here too. No need to move
>>>> the port_id parameter here.
>>>>>
>>>>
>>>> Yes, for most of the _dump() APIs, file pointer seems is the first
>>>> argument,
>>>> bu they are from various libraries.
>>>>
>>>> Within the ethdev APIs, I think it makes sense that all APIs start with
>>>> 'port_id' parameter for consistency, like done in:
>>>> rte_flow_dev_dump(uint16_t port_id, ...)
>>>>
>>>>> Only rte_dma_dump() has the file pointer last, and I didn't catch it
>>>> when the function was defined.
>>>>>
>>>
>>> OK. Then I agree with you about following the convention like rte_flow_dev_dump() with the port_id first.
>>>
>>> I even think Connor got it right the first time, and I proposed following the other convention.
>>>
>>
>> Ahh, may bad I missed that, sorry for not commenting on time.
>>
>>
>>> It's not easy when there are two opposite conventions. :-)
>>>
>>
>> Yep, that is the main issue.
>>
>> .
Hi, Ferruh,
在 2022/2/8 18:21, Ferruh Yigit 写道:
> On 2/8/2022 12:39 AM, Min Hu (Connor) wrote:
>> Hi, Ferruh,
>>
>> 在 2022/2/7 23:35, Ferruh Yigit 写道:
>>> On 2/7/2022 12:56 PM, Morten Brørup wrote:
>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>>> Sent: Monday, 7 February 2022 13.36
>>>>>
>>>>> On 2/7/2022 12:18 PM, Morten Brørup wrote:
>>>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>>>>> Sent: Monday, 7 February 2022 12.46
>>>>>>>
>>>>>>> On 2/7/2022 1:47 AM, Min Hu (Connor) wrote:
>>>>>>>> Added the ethdev dump API which provides functions for query
>>>>> private
>>>>>>> info
>>>>>>>
>>>>>>> Isn't API and function are same thing in this contexts?
>>>>>>>
>>>>>>>> from device. 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.>
>>>>>>>
>>>>>>> In the patch title 'ethdev' is duplicated, can you fix it?
>>>>>>>
>>>>>>>
>>>>>>>> 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>
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>> @@ -990,6 +990,20 @@ 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 ethdev private info to a file.
>>>>>>>> + *
>>>>>>>
>>>>>>> It doesn't dump the 'ethdev' private info, it dumps the private info
>>>>>>> from device.
>>>>>>
>>>>>> It seems perfectly clear to me. How would you prefer it phrased
>>>>> instead?
>>>>>>
>>>>>
>>>>> What described in the document is more accurate,
>>>>> "query private info from device".
>>>>>
>>>>> What we are dumping here is not ethdev private info, it is device
>>>>> private info,
>>
>> what is the difference between ethdev and device?
>
> It is not very clear, but for me 'ethdev' is refers to device abstract
> layer (ethdev library) specific private data
Could you give an example for 'ethdev'specific private data ?
and device refers to ethdev
> device (PMD) private data. ethdev is common for all drivers.
OK, we could treat it as convention in future.
>
>>>>> and we really don't know what that data may be in the ethdev layer.
>>>>>
>>>>> Also there is a chance that 'ethdev private info' can be confused with
>>>>> 'ethdev->data->dev_private'
>> what I want to dump is exactly the 'ethdev->data->dev_private'.
>> 'ethdev private info' means 'ethdev->data->dev_private'.
>> why confused?
>
> What I understand was this API can return any device private information,
> it is not limited to 'ethdev->data->dev_private', (although most of the
I think this API is limited to 'ethdev->data->dev_private'.
> data
> is represented in this struct), like if you want to dump queue state,
> this is out of 'ethdev->data->dev_private'.
Queue state can be dumped using API 'rte_eth_tx_queue_info_get'.
>
>>>>
>>>> OK. Now I got your point! The difference is very subtle.
>>>>
>>>>>
>>>>>> [...]
>>>>>>
>>>>>>>
>>>>>>>> + */
>>>>>>>> +__rte_experimental
>>>>>>>> +int rte_eth_dev_priv_dump(FILE *file, uint16_t port_id);
>>>>>>>> +
>>>>>>>
>>>>>>> What do you think to have the 'port_id' as first argument to be
>>>>>>> consistent
>>>>>>> with the other APIs?
>>>>>>
>>>>>> The _dump APIs in other libraries have the file pointer as the first
>>>>> parameter, so let's follow that convention here too. No need to move
>>>>> the port_id parameter here.
>>>>>>
>>>>>
>>>>> Yes, for most of the _dump() APIs, file pointer seems is the first
>>>>> argument,
>>>>> bu they are from various libraries.
>>>>>
>>>>> Within the ethdev APIs, I think it makes sense that all APIs start
>>>>> with
>>>>> 'port_id' parameter for consistency, like done in:
>>>>> rte_flow_dev_dump(uint16_t port_id, ...)
>>>>>
>>>>>> Only rte_dma_dump() has the file pointer last, and I didn't catch it
>>>>> when the function was defined.
>>>>>>
>>>>
>>>> OK. Then I agree with you about following the convention like
>>>> rte_flow_dev_dump() with the port_id first.
>>>>
>>>> I even think Connor got it right the first time, and I proposed
>>>> following the other convention.
>>>>
>>>
>>> Ahh, may bad I missed that, sorry for not commenting on time.
>>>
>>>
>>>> It's not easy when there are two opposite conventions. :-)
>>>>
>>>
>>> Yep, that is the main issue.
>>>
>>> .
>
> .
On 2/8/2022 11:14 AM, Min Hu (Connor) wrote:
> Hi, Ferruh,
>
> 在 2022/2/8 18:21, Ferruh Yigit 写道:
>> On 2/8/2022 12:39 AM, Min Hu (Connor) wrote:
>>> Hi, Ferruh,
>>>
>>> 在 2022/2/7 23:35, Ferruh Yigit 写道:
>>>> On 2/7/2022 12:56 PM, Morten Brørup wrote:
>>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>>>> Sent: Monday, 7 February 2022 13.36
>>>>>>
>>>>>> On 2/7/2022 12:18 PM, Morten Brørup wrote:
>>>>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>>>>>> Sent: Monday, 7 February 2022 12.46
>>>>>>>>
>>>>>>>> On 2/7/2022 1:47 AM, Min Hu (Connor) wrote:
>>>>>>>>> Added the ethdev dump API which provides functions for query
>>>>>> private
>>>>>>>> info
>>>>>>>>
>>>>>>>> Isn't API and function are same thing in this contexts?
>>>>>>>>
>>>>>>>>> from device. 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.>
>>>>>>>>
>>>>>>>> In the patch title 'ethdev' is duplicated, can you fix it?
>>>>>>>>
>>>>>>>>
>>>>>>>>> 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>
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>>> @@ -990,6 +990,20 @@ 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 ethdev private info to a file.
>>>>>>>>> + *
>>>>>>>>
>>>>>>>> It doesn't dump the 'ethdev' private info, it dumps the private info
>>>>>>>> from device.
>>>>>>>
>>>>>>> It seems perfectly clear to me. How would you prefer it phrased
>>>>>> instead?
>>>>>>>
>>>>>>
>>>>>> What described in the document is more accurate,
>>>>>> "query private info from device".
>>>>>>
>>>>>> What we are dumping here is not ethdev private info, it is device
>>>>>> private info,
>>>
>>> what is the difference between ethdev and device?
>>
>> It is not very clear, but for me 'ethdev' is refers to device abstract
>> layer (ethdev library) specific private data
> Could you give an example for 'ethdev'specific private data ?
>
I think 'struct rte_eth_dev' content can be a sample.
But I hear you, diff is not clear, it is subtle as Morten said,
when doc and commit log refers it as "private info from device",
I think we can use the same in the API documentation as well.
> and device refers to ethdev
>> device (PMD) private data. ethdev is common for all drivers.
> OK, we could treat it as convention in future.
>>
>>>>>> and we really don't know what that data may be in the ethdev layer.
>>>>>>
>>>>>> Also there is a chance that 'ethdev private info' can be confused with
>>>>>> 'ethdev->data->dev_private'
>>> what I want to dump is exactly the 'ethdev->data->dev_private'.
>>> 'ethdev private info' means 'ethdev->data->dev_private'.
>>> why confused?
>>
>> What I understand was this API can return any device private information,
>> it is not limited to 'ethdev->data->dev_private', (although most of the
> I think this API is limited to 'ethdev->data->dev_private'.
>> data
>> is represented in this struct), like if you want to dump queue state,
>> this is out of 'ethdev->data->dev_private'.
> Queue state can be dumped using API 'rte_eth_tx_queue_info_get'.
>
Yes it can be. But as far as I can see there is nothing prevents the dump()
API to provide the same, it is up to PMD.
If the intention is to limit what can be dump to 'ethdev->data->dev_private',
it is not clear from API documentation/implementation.
>>
>>>>>
>>>>> OK. Now I got your point! The difference is very subtle.
>>>>>
>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>>
>>>>>>>>> + */
>>>>>>>>> +__rte_experimental
>>>>>>>>> +int rte_eth_dev_priv_dump(FILE *file, uint16_t port_id);
>>>>>>>>> +
>>>>>>>>
>>>>>>>> What do you think to have the 'port_id' as first argument to be
>>>>>>>> consistent
>>>>>>>> with the other APIs?
>>>>>>>
>>>>>>> The _dump APIs in other libraries have the file pointer as the first
>>>>>> parameter, so let's follow that convention here too. No need to move
>>>>>> the port_id parameter here.
>>>>>>>
>>>>>>
>>>>>> Yes, for most of the _dump() APIs, file pointer seems is the first
>>>>>> argument,
>>>>>> bu they are from various libraries.
>>>>>>
>>>>>> Within the ethdev APIs, I think it makes sense that all APIs start with
>>>>>> 'port_id' parameter for consistency, like done in:
>>>>>> rte_flow_dev_dump(uint16_t port_id, ...)
>>>>>>
>>>>>>> Only rte_dma_dump() has the file pointer last, and I didn't catch it
>>>>>> when the function was defined.
>>>>>>>
>>>>>
>>>>> OK. Then I agree with you about following the convention like rte_flow_dev_dump() with the port_id first.
>>>>>
>>>>> I even think Connor got it right the first time, and I proposed following the other convention.
>>>>>
>>>>
>>>> Ahh, may bad I missed that, sorry for not commenting on time.
>>>>
>>>>
>>>>> It's not easy when there are two opposite conventions. :-)
>>>>>
>>>>
>>>> Yep, that is the main issue.
>>>>
>>>> .
>>
>> .
08/02/2022 13:59, Ferruh Yigit:
> On 2/8/2022 11:14 AM, Min Hu (Connor) wrote:
> > 在 2022/2/8 18:21, Ferruh Yigit 写道:
> >> What I understand was this API can return any device private information,
> >> it is not limited to 'ethdev->data->dev_private', (although most of the
> > I think this API is limited to 'ethdev->data->dev_private'.
> >> data
> >> is represented in this struct), like if you want to dump queue state,
> >> this is out of 'ethdev->data->dev_private'.
> > Queue state can be dumped using API 'rte_eth_tx_queue_info_get'.
> >
>
> Yes it can be. But as far as I can see there is nothing prevents the dump()
> API to provide the same, it is up to PMD.
>
> If the intention is to limit what can be dump to 'ethdev->data->dev_private',
> it is not clear from API documentation/implementation.
Why limiting?
The dump could even print debug infos which are nowhere else.
Hi,
在 2022/2/8 20:59, Ferruh Yigit 写道:
> On 2/8/2022 11:14 AM, Min Hu (Connor) wrote:
>> Hi, Ferruh,
>>
>> 在 2022/2/8 18:21, Ferruh Yigit 写道:
>>> On 2/8/2022 12:39 AM, Min Hu (Connor) wrote:
>>>> Hi, Ferruh,
>>>>
>>>> 在 2022/2/7 23:35, Ferruh Yigit 写道:
>>>>> On 2/7/2022 12:56 PM, Morten Brørup wrote:
>>>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>>>>> Sent: Monday, 7 February 2022 13.36
>>>>>>>
>>>>>>> On 2/7/2022 12:18 PM, Morten Brørup wrote:
>>>>>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
>>>>>>>>> Sent: Monday, 7 February 2022 12.46
>>>>>>>>>
>>>>>>>>> On 2/7/2022 1:47 AM, Min Hu (Connor) wrote:
>>>>>>>>>> Added the ethdev dump API which provides functions for query
>>>>>>> private
>>>>>>>>> info
>>>>>>>>>
>>>>>>>>> Isn't API and function are same thing in this contexts?
>>>>>>>>>
>>>>>>>>>> from device. 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.>
>>>>>>>>>
>>>>>>>>> In the patch title 'ethdev' is duplicated, can you fix it?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> 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>
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>>> @@ -990,6 +990,20 @@ 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 ethdev private info to a file.
>>>>>>>>>> + *
>>>>>>>>>
>>>>>>>>> It doesn't dump the 'ethdev' private info, it dumps the private
>>>>>>>>> info
>>>>>>>>> from device.
>>>>>>>>
>>>>>>>> It seems perfectly clear to me. How would you prefer it phrased
>>>>>>> instead?
>>>>>>>>
>>>>>>>
>>>>>>> What described in the document is more accurate,
>>>>>>> "query private info from device".
>>>>>>>
>>>>>>> What we are dumping here is not ethdev private info, it is device
>>>>>>> private info,
>>>>
>>>> what is the difference between ethdev and device?
>>>
>>> It is not very clear, but for me 'ethdev' is refers to device abstract
>>> layer (ethdev library) specific private data
>> Could you give an example for 'ethdev'specific private data ?
>>
>
> I think 'struct rte_eth_dev' content can be a sample.
>
> But I hear you, diff is not clear, it is subtle as Morten said,
> when doc and commit log refers it as "private info from device",
> I think we can use the same in the API documentation as well.
>
Agreed， I will fix it in release_22_03.rst.
>> and device refers to ethdev
>>> device (PMD) private data. ethdev is common for all drivers.
>> OK, we could treat it as convention in future.
>>>
>>>>>>> and we really don't know what that data may be in the ethdev layer.
>>>>>>>
>>>>>>> Also there is a chance that 'ethdev private info' can be confused
>>>>>>> with
>>>>>>> 'ethdev->data->dev_private'
>>>> what I want to dump is exactly the 'ethdev->data->dev_private'.
>>>> 'ethdev private info' means 'ethdev->data->dev_private'.
>>>> why confused?
>>>
>>> What I understand was this API can return any device private
>>> information,
>>> it is not limited to 'ethdev->data->dev_private', (although most of the
>> I think this API is limited to 'ethdev->data->dev_private'.
>>> data
>>> is represented in this struct), like if you want to dump queue state,
>>> this is out of 'ethdev->data->dev_private'.
>> Queue state can be dumped using API 'rte_eth_tx_queue_info_get'.
>>
>
> Yes it can be. But as far as I can see there is nothing prevents the dump()
> API to provide the same, it is up to PMD.
>
> If the intention is to limit what can be dump to
> 'ethdev->data->dev_private',
> it is not clear from API documentation/implementation.
>
>>>
>>>>>>
>>>>>> OK. Now I got your point! The difference is very subtle.
>>>>>>
>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> + */
>>>>>>>>>> +__rte_experimental
>>>>>>>>>> +int rte_eth_dev_priv_dump(FILE *file, uint16_t port_id);
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> What do you think to have the 'port_id' as first argument to be
>>>>>>>>> consistent
>>>>>>>>> with the other APIs?
>>>>>>>>
>>>>>>>> The _dump APIs in other libraries have the file pointer as the
>>>>>>>> first
>>>>>>> parameter, so let's follow that convention here too. No need to move
>>>>>>> the port_id parameter here.
>>>>>>>>
>>>>>>>
>>>>>>> Yes, for most of the _dump() APIs, file pointer seems is the first
>>>>>>> argument,
>>>>>>> bu they are from various libraries.
>>>>>>>
>>>>>>> Within the ethdev APIs, I think it makes sense that all APIs
>>>>>>> start with
>>>>>>> 'port_id' parameter for consistency, like done in:
>>>>>>> rte_flow_dev_dump(uint16_t port_id, ...)
>>>>>>>
>>>>>>>> Only rte_dma_dump() has the file pointer last, and I didn't
>>>>>>>> catch it
>>>>>>> when the function was defined.
>>>>>>>>
>>>>>>
>>>>>> OK. Then I agree with you about following the convention like
>>>>>> rte_flow_dev_dump() with the port_id first.
>>>>>>
>>>>>> I even think Connor got it right the first time, and I proposed
>>>>>> following the other convention.
>>>>>>
>>>>>
>>>>> Ahh, may bad I missed that, sorry for not commenting on time.
>>>>>
>>>>>
>>>>>> It's not easy when there are two opposite conventions. :-)
>>>>>>
>>>>>
>>>>> Yep, that is the main issue.
>>>>>
>>>>> .
>>>
>>> .
>
> .
在 2022/2/8 21:52, Thomas Monjalon 写道:
> 08/02/2022 13:59, Ferruh Yigit:
>> On 2/8/2022 11:14 AM, Min Hu (Connor) wrote:
>>> 在 2022/2/8 18:21, Ferruh Yigit 写道:
>>>> What I understand was this API can return any device private information,
>>>> it is not limited to 'ethdev->data->dev_private', (although most of the
>>> I think this API is limited to 'ethdev->data->dev_private'.
>>>> data
>>>> is represented in this struct), like if you want to dump queue state,
>>>> this is out of 'ethdev->data->dev_private'.
>>> Queue state can be dumped using API 'rte_eth_tx_queue_info_get'.
>>>
>>
>> Yes it can be. But as far as I can see there is nothing prevents the dump()
>> API to provide the same, it is up to PMD.
>>
>> If the intention is to limit what can be dump to 'ethdev->data->dev_private',
>> it is not clear from API documentation/implementation.
>
> Why limiting?
> The dump could even print debug infos which are nowhere else.
Yes, no need to limit, it is up to PMD, just as Ferruh said.
>
>
>
> .
>
@@ -92,6 +92,13 @@ New Features
* Called ``rte_ipv4/6_udptcp_cksum_mbuf()`` functions in testpmd csum mode
to support software UDP/TCP checksum over multiple segments.
+* **Added the private ethdev dump API, for query private info of ethdev.**
+
+ Added the private ethdev dump API which provides functions for query
+ 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.
+
Removed Items
-------------
@@ -990,6 +990,20 @@ 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 ethdev private info to a file.
+ *
+ * @param file
+ * A pointer to a file for output.
+ * @param dev
+ * Port (ethdev) handle.
+ *
+ * @return
+ * Negative errno value on error, positive value on success.
+ */
+typedef int (*eth_dev_priv_dump_t)(FILE *file, struct rte_eth_dev *dev);
+
/**
* @internal A structure containing the functions exported by an Ethernet driver.
*/
@@ -1186,6 +1200,9 @@ struct eth_dev_ops {
* kinds of metadata to the PMD
*/
eth_rx_metadata_negotiate_t rx_metadata_negotiate;
+
+ /** Dump ethdev private info */
+ eth_dev_priv_dump_t eth_dev_priv_dump;
};
/**
@@ -6485,6 +6485,21 @@ 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(FILE *file, uint16_t port_id)
+{
+ struct rte_eth_dev *dev;
+ int ret;
+
+ RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+ dev = &rte_eth_devices[port_id];
+
+ RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->eth_dev_priv_dump, -ENOTSUP);
+ ret = (*dev->dev_ops->eth_dev_priv_dump)(file, dev);
+
+ return ret;
+}
+
RTE_LOG_REGISTER_DEFAULT(rte_eth_dev_logtype, INFO);
RTE_INIT(ethdev_init_telemetry)
@@ -5902,6 +5902,22 @@ 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 ethdev private info to a file.
+ *
+ * @param file
+ * A pointer to a file for output.
+ * @param port_id
+ * The port identifier of the Ethernet device.
+ * @return
+ * Negative errno value on error, positive value on success.
+ */
+__rte_experimental
+int rte_eth_dev_priv_dump(FILE *file, uint16_t port_id);
+
#ifdef __cplusplus
}
#endif
@@ -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;
};
INTERNAL {