From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
by inbox.dpdk.org (Postfix) with ESMTP id 66850A00BE;
Fri, 11 Feb 2022 05:53:37 +0100 (CET)
Received: from [217.70.189.124] (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id 5825B41144;
Fri, 11 Feb 2022 05:53:37 +0100 (CET)
Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188])
by mails.dpdk.org (Postfix) with ESMTP id 80D5B410E5
for <dev@dpdk.org>; Fri, 11 Feb 2022 05:53:35 +0100 (CET)
Received: from dggeme756-chm.china.huawei.com (unknown [172.30.72.54])
by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4Jw1Sb34lvzbk0T;
Fri, 11 Feb 2022 12:52:31 +0800 (CST)
Received: from [10.67.103.128] (10.67.103.128) by
dggeme756-chm.china.huawei.com (10.3.19.102) with Microsoft SMTP Server
(version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256_P256) id
15.1.2308.21; Fri, 11 Feb 2022 12:53:33 +0800
Subject: Re: [PATCH v3] ethdev: introduce dump API
To: Ferruh Yigit <ferruh.yigit@intel.com>, <dev@dpdk.org>
CC: =?UTF-8?Q?Morten_Br=c3=b8rup?= <mb@smartsharesystems.com>, Ray Kinsella
<mdr@ashroe.eu>, Ajit Khaparde <ajit.khaparde@broadcom.com>, Thomas Monjalon
<thomas@monjalon.net>, Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
References: <20220111115437.32855-1-humin29@huawei.com>
<20220209012106.23404-1-humin29@huawei.com>
<480b7433-2d7b-be1f-ec52-40ab25006627@intel.com>
<4ed15350-d311-abf6-f85f-5285a169f036@intel.com>
From: "Min Hu (Connor)" <humin29@huawei.com>
Message-ID: <65eeabb2-85e7-6054-6328-0bab58b565b7@huawei.com>
Date: Fri, 11 Feb 2022 12:53:33 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
Thunderbird/68.3.1
MIME-Version: 1.0
In-Reply-To: <4ed15350-d311-abf6-f85f-5285a169f036@intel.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 8bit
X-Originating-IP: [10.67.103.128]
X-ClientProxiedBy: dggems705-chm.china.huawei.com (10.3.19.182) To
dggeme756-chm.china.huawei.com (10.3.19.102)
X-CFilter-Loop: Reflected
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
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 {
>>
>
> .