[dpdk-dev] [PATCH v5 04/10] ethdev: add simple power management API

[Burakov, Anatoly]

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?

> 
>>
>>   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.

> 
>> +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