[dpdk-dev] [PATCH v5 04/10] ethdev: add simple power management API
Burakov, Anatoly
anatoly.burakov at intel.com
Wed Oct 14 11:30:14 CEST 2020
On 14-Oct-20 10:15 AM, Guo, Jia wrote:
>
>> -----Original Message-----
>> From: Burakov, Anatoly <anatoly.burakov at intel.com>
>> Sent: Wednesday, October 14, 2020 5:07 PM
>> To: Guo, Jia <jia.guo at intel.com>; dev at dpdk.org
>> Cc: Ma, Liang J <liang.j.ma at intel.com>; Thomas Monjalon
>> <thomas at monjalon.net>; Yigit, Ferruh <ferruh.yigit at intel.com>; Andrew
>> Rybchenko <andrew.rybchenko at oktetlabs.ru>; Ray Kinsella
>> <mdr at ashroe.eu>; Neil Horman <nhorman at tuxdriver.com>; Hunt, David
>> <david.hunt at intel.com>; Ananyev, Konstantin
>> <konstantin.ananyev at intel.com>; jerinjacobk at gmail.com; Richardson,
>> Bruce <bruce.richardson at intel.com>; McDaniel, Timothy
>> <timothy.mcdaniel at intel.com>; Eads, Gage <gage.eads at intel.com>;
>> Macnamara, Chris <chris.macnamara at intel.com>
>> Subject: Re: [dpdk-dev] [PATCH v5 04/10] ethdev: add simple power
>> management API
>>
>> On 14-Oct-20 4:10 AM, Guo, Jia wrote:
>>>
>>>> -----Original Message-----
>>>> From: dev <dev-bounces at dpdk.org> On Behalf Of Anatoly Burakov
>>>> Sent: Saturday, October 10, 2020 12:02 AM
>>>> To: dev at dpdk.org
>>>> Cc: Ma, Liang J <liang.j.ma at intel.com>; Thomas Monjalon
>>>> <thomas at monjalon.net>; Yigit, Ferruh <ferruh.yigit at intel.com>; Andrew
>>>> Rybchenko <andrew.rybchenko at oktetlabs.ru>; Ray Kinsella
>>>> <mdr at ashroe.eu>; Neil Horman <nhorman at tuxdriver.com>; Hunt, David
>>>> <david.hunt at intel.com>; Ananyev, Konstantin
>>>> <konstantin.ananyev at intel.com>; jerinjacobk at gmail.com; Richardson,
>>>> Bruce <bruce.richardson at intel.com>; McDaniel, Timothy
>>>> <timothy.mcdaniel at intel.com>; Eads, Gage <gage.eads at intel.com>;
>>>> Macnamara, Chris <chris.macnamara at intel.com>
>>>> Subject: [dpdk-dev] [PATCH v5 04/10] ethdev: add simple power
>>>> management API
>>>>
>>>> From: Liang Ma <liang.j.ma at intel.com>
>>>>
>>>> Add a simple API to allow getting address of next RX descriptor from
>>>> the PMD, as well as release notes information.
>>>>
>>>> Signed-off-by: Liang Ma <liang.j.ma at intel.com>
>>>> Signed-off-by: Anatoly Burakov <anatoly.burakov at intel.com>
>>>> ---
>>
>> Hi Jia,
>>
>> Thanks for your review. Responses below.
>>
>>>>
>>>> Notes:
>>>> v5:
>>>> - Bring function format in line with other functions in the file
>>>> - Ensure the API is supported by the driver before calling it
>>>> (Konstantin)
>>>>
>>>> doc/guides/rel_notes/release_20_11.rst | 16 ++++++++++++++
>>>> lib/librte_ethdev/rte_ethdev.c | 17 ++++++++++++++
>>>> lib/librte_ethdev/rte_ethdev.h | 24 ++++++++++++++++++++
>>>> lib/librte_ethdev/rte_ethdev_driver.h | 28
>> ++++++++++++++++++++++++
>>>> lib/librte_ethdev/rte_ethdev_version.map | 1 +
>>>> 5 files changed, 86 insertions(+)
>>>>
>>>> diff --git a/doc/guides/rel_notes/release_20_11.rst
>>>> b/doc/guides/rel_notes/release_20_11.rst
>>>> index 808bdc4e54..e85af5d3e9 100644
>>>> --- a/doc/guides/rel_notes/release_20_11.rst
>>>> +++ b/doc/guides/rel_notes/release_20_11.rst
>>>> @@ -55,6 +55,11 @@ New Features
>>>> Also, make sure to start the actual text at the margin.
>>>>
>> =======================================================
>>>>
>>>> +* **ethdev: add 1 new EXPERIMENTAL API for PMD power
>>>> management.**
>>>> +
>>>> + * ``rte_eth_get_wake_addr()``
>>>> + * add new eth_dev_ops ``get_wake_addr``
>>>> +
>>>> * **Updated Broadcom bnxt driver.**
>>>>
>>>> Updated the Broadcom bnxt driver with new features and
>>>> improvements,
>>>> including:
>>>> @@ -136,6 +141,17 @@ New Features
>>>> * Extern objects and functions can be plugged into the pipeline.
>>>> * Transaction-oriented table updates.
>>>>
>>>> +* **Add PMD power management mechanism**
>>>> +
>>>> + 3 new Ethernet PMD power management mechanism is added through
>>>
>>> " mechanisms are " please.
>>>
>>>> + existing RX callback infrastructure.
>>>> +
>>>> + * Add power saving scheme based on UMWAIT instruction (x86 only)
>>>> + * Add power saving scheme based on ``rte_pause()``
>>>> + * Add power saving scheme based on frequency scaling through the
>>>> + power library
>>>> + * Add new EXPERIMENTAL API
>>>> ``rte_power_pmd_mgmt_queue_enable()``
>>>> + * Add new EXPERIMENTAL API
>>>> ``rte_power_pmd_mgmt_queue_disable()``
>>>> +
>>>
>>> Could this doc be separate to other specific patch if it is not related with this
>> patch?
>>
>> It is related - it's the doc changes that add mention of this API. I was under
>> the impression current policy was having doc updates in the same patch as
>> the changes made?
>>
>
> Do you think this part would be better separate into [PATCH v5 05/10]?
Oh, sorry, you're right, these aren't related, as this functionality
isn't in this patch yet. Will fix.
>
>>>
>>>>
>>>> Removed Items
>>>> -------------
>>>> diff --git a/lib/librte_ethdev/rte_ethdev.c
>>>> b/lib/librte_ethdev/rte_ethdev.c index 48d1333b17..352108f43c 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>>> @@ -4804,6 +4804,23 @@ rte_eth_tx_burst_mode_get(uint16_t port_id,
>>>> uint16_t queue_id,
>>>> dev->dev_ops->tx_burst_mode_get(dev, queue_id,
>> mode)); }
>>>>
>>>> +int
>>>> +rte_eth_get_wake_addr(uint16_t port_id, uint16_t queue_id,
>>>> + volatile void **wake_addr, uint64_t *expected, uint64_t
>>>> *mask) {
>>>> + struct rte_eth_dev *dev;
>>>> +
>>>> + 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->get_wake_addr, -
>>>> ENOTSUP);
>>>> +
>>>> + return eth_err(port_id,
>>>> + dev->dev_ops->get_wake_addr(dev->data-
>>>>> rx_queues[queue_id],
>>>> + wake_addr, expected, mask));
>>>> +}
>>>> +
>>>> int
>>>> rte_eth_dev_set_mc_addr_list(uint16_t port_id,
>>>> struct rte_ether_addr *mc_addr_set, diff --git
>>>> a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>>>> index
>>>> d2bf74f128..a6cfe3cd57 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev.h
>>>> +++ b/lib/librte_ethdev/rte_ethdev.h
>>>> @@ -4014,6 +4014,30 @@ __rte_experimental int
>>>> rte_eth_tx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
>>>> struct rte_eth_burst_mode *mode);
>>>>
>>>> +/**
>>>> + * Retrieve the wake up address from specific queue
>>>> + *
>>>> + * @param port_id
>>>> + * The port identifier of the Ethernet device.
>>>> + * @param queue_id
>>>> + * The Tx queue on the Ethernet device for which information
>>>> + * will be retrieved.
>>>> + * @param wake_addr
>>>> + * The pointer point to the address which is used for monitoring.
>>>> + * @param expected
>>>> + * The pointer point to value to be expected when descriptor is set.
>>>> + * @param mask
>>>> + * The pointer point to comparison bitmask for the expected value.
>>>> + *
>>>> + * @return
>>>> + * - 0: Success.
>>>> + * -EINVAL: Failed to get wake address.
>>>> + */
>>>
>>> Is that "-EINVAL " is the only error value which will be return?
>>
>> Also -ENOTSUP, i'll add this, thanks.
>>
>>>
>>>> +__rte_experimental
>>>> +int rte_eth_get_wake_addr(uint16_t port_id, uint16_t queue_id,
>>>> + volatile void **wake_addr,
>>>> + uint64_t *expected, uint64_t *mask);
>>>> +
>>>> /**
>>>> * Retrieve device registers and register attributes (number of registers
>> and
>>>> * register size)
>>>> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h
>>>> b/lib/librte_ethdev/rte_ethdev_driver.h
>>>> index c3062c246c..935d46f25c 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev_driver.h
>>>> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
>>>> @@ -574,6 +574,31 @@ typedef int (*eth_tx_hairpin_queue_setup_t)
>>>> uint16_t nb_tx_desc,
>>>> const struct rte_eth_hairpin_conf *hairpin_conf);
>>>>
>>>> +/**
>>>> + * @internal
>>>> + * Get the Wake up address.
>>>> + *
>>>> + * @param rxq
>>>> + * Ethdev queue pointer.
>>>> + * @param tail_desc_addr
>>>> + * The pointer point to descriptor address var.
>>>> + * @param expected
>>>> + * The pointer point to value to be expected when descriptor is set.
>>>> + * @param mask
>>>> + * The pointer point to comparison bitmask for the expected value.
>>>> + * @return
>>>> + * Negative errno value on error, 0 on success.
>>>> + *
>>>> + * @retval 0
>>>> + * Success.
>>>> + * @retval -EINVAL
>>>> + * Failed to get descriptor address.
>>>> + */
>>>
>>> The question is the same as above.
>>
>> This is a driver function pointer, so return value will depend on driver
>> implementation. So far we only see 0 or -EINVAL values from the driver itself,
>> while -ENOTSUP will be returned by ethdev in case there is no driver
>> implementation of this function. So, in this case this is correct.
>>
>
> Ok.
>
>>>
>>>> +typedef int (*eth_get_wake_addr_t)
>>>> + (void *rxq, volatile void **tail_desc_addr,
>>>> + uint64_t *expected, uint64_t *mask);
>>>> +
>>>> +
>>>> /**
>>>> * @internal A structure containing the functions exported by an
>> Ethernet
>>>> driver.
>>>> */
>>>> @@ -713,6 +738,9 @@ struct eth_dev_ops {
>>>> /**< Set up device RX hairpin queue. */
>>>> eth_tx_hairpin_queue_setup_t tx_hairpin_queue_setup;
>>>> /**< Set up device TX hairpin queue. */
>>>> + eth_get_wake_addr_t get_wake_addr;
>>>> + /**< Get wake up address. */
>>>> +
>>>> };
>>>>
>>>> /**
>>>> diff --git a/lib/librte_ethdev/rte_ethdev_version.map
>>>> b/lib/librte_ethdev/rte_ethdev_version.map
>>>> index c95ef5157a..3cb2093980 100644
>>>> --- a/lib/librte_ethdev/rte_ethdev_version.map
>>>> +++ b/lib/librte_ethdev/rte_ethdev_version.map
>>>> @@ -229,6 +229,7 @@ EXPERIMENTAL {
>>>> # added in 20.11
>>>> rte_eth_link_speed_to_str;
>>>> rte_eth_link_to_str;
>>>> + rte_eth_get_wake_addr;
>>>> };
>>>>
>>>> INTERNAL {
>>>> --
>>>> 2.17.1
>>
>>
>> --
>> Thanks,
>> Anatoly
--
Thanks,
Anatoly
More information about the dev
mailing list