Re: [dpdk-dev] [PATCH v11 1/6] ethdev: add simple power management API

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 02-Nov-20 11:10 AM, Liang Ma wrote:
> Add a simple API to allow getting address of getting notification
> information from the PMD, as well as release notes information.
> 
> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
> 
> Notes:
>      v11:
>      - Rework the API Doxygen documentation
> 
>      v10:
>      - Address minor issue on comments and release notes
> 
>      v8:
>      - Rename version map file name
> 
>      v7:
>      - Fixed race condition (Konstantin)
>      - Slight rework of the structure of monitor code
>      - Added missing inline for wakeup
> 
>      v6:
>      - Added wakeup mechanism for UMWAIT
>      - Removed memory allocation (everything is now allocated statically)
>      - Fixed various typos and comments
>      - Check for invalid queue ID
>      - Moved release notes to this patch
> 
>      v5:
>      - Make error checking more robust
>        - Prevent initializing scaling if ACPI or PSTATE env wasn't set
>        - Prevent initializing UMWAIT path if PMD doesn't support get_wake_addr
>      - Add some debug logging
>      - Replace x86-specific code path to generic path using the intrinsic check
> ---

<snip>

>   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 e341a08817..d208fe99ca 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -4364,6 +4364,47 @@ __rte_experimental
>   int rte_eth_tx_burst_mode_get(uint16_t port_id, uint16_t queue_id,
>   	struct rte_eth_burst_mode *mode);
>   
> +/**
> + * In order to make use of some PMD Power Management schemes, the user might
> + * want to wait (sleep/poll) until new packets arrive. This function
> + * retrieves the necessary information from the PMD to enter that wait/sleep
> + * state. The main parameter provided is the address to monitor while waiting
> + * to wake up. In addition to this wake address, the function also provides
> + * extra information including expected value, selection mask and data size to
> + * monitor. The user is expected to use this information to enter low power
> + * mode using the rte_power_monitor API, and the core will exit low power mode
> + * upon reaching the expected condition:
> + * (((uint64_t)read_mem(wake_addr, data_sz)) & mask) == expected).
> + * @note The low power mode can also exit in other cases, e.g. interrupt.

Could we maybe have some paragraphs and spacing here, instead of one 
solid block of text? Suggested formatting:

  * In order to make use of some PMD Power Management schemes, the user 
might
  * want to wait (sleep/poll) until new packets arrive. This function 
retrieves
  * the necessary information from the PMD to enter that wait/sleep state.
  *
  * The main parameter provided is the address to monitor while waiting 
to wake
  * up. In addition to this wake address, the function also provides extra
  * information including expected value, selection mask and data size to
  * monitor.
  *
  * The user is expected to use this information to enter low power mode 
using
  * the rte_power_monitor API, and the core will exit low power mode upon
  * reaching the expected condition:
  *
  *   `(((uint64_t)read_mem(wake_addr, data_sz)) & mask) == expected)`
  *
  * @note The low power mode can also exit in other cases, e.g. interrupt.

> + *
> + * @param[in] port_id
> + *  The port identifier of the Ethernet device.
> + * @param[in] queue_id
> + *  The Rx queue on the Ethernet device for which information will be
> + *  retrieved.
> + * @param[out] wake_addr
> + *  The pointer to the address which will be monitored.
> + * @param[out] expected
> + *  The pointer to the expected value to allow wakeup condition.
> + * @param[out] mask
> + *  The pointer to comparison bitmask for the expected value.
> + * @note a mask value of zero should be treated as:
> + *  “no special wakeup values for provided address from the driver”.

Wrong quotes.

> + * @param[out] data_sz
> + *  The pointer to data size for the expected value (in bytes)
> + * @note valid values are 1,2,4,8

Shouldn't @note be under @param, i.e. indented to the right at the same 
level the text is? Also, everywhere else in this file, the indentation 
of the text is two spaces, not one. So, should be e.g.

* @param[out] data_sz
*   The pointer to data......
*   @note valid values are.....

> + *
> + * @return
> + *  - 0: Success.
> + *  -ENOTSUP: Operation not supported.
> + *  -EINVAL: Invalid parameters.
> + *  -ENODEV: Invalid port ID.
> + */
> +__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,
> +		uint8_t *data_sz);
> +
>   /**
>    * 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 c63b9f7eb7..e7ce1e261d 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -752,6 +752,32 @@ typedef int (*eth_hairpin_queue_peer_unbind_t)
>   	(struct rte_eth_dev *dev, uint16_t cur_queue, uint32_t direction);
>   /**< @internal Unbind peer queue from the current queue. */
>   
> +/**
> + * @internal
> + * Get address of memory location whose contents will change whenever there is
> + * new data to be received.
> + *
> + * @param rxq
> + *   Ethdev queue pointer.
> + * @param tail_desc_addr
> + *   The pointer point to where the address will be stored.
> + * @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.
> + * @param data_sz
> + *   Data size for the expected value (can be 1, 2, 4, or 8 bytes)
> + * @return
> + *   Negative errno value on error, 0 on success.
> + *
> + * @retval 0
> + *   Success
> + * @retval -EINVAL
> + *   Invalid parameters
> + */
> +typedef int (*eth_get_wake_addr_t)(void *rxq, volatile void **tail_desc_addr,
> +		uint64_t *expected, uint64_t *mask, uint8_t *data_sz);
> +
>   /**
>    * @internal A structure containing the functions exported by an Ethernet driver.
>    */
> @@ -910,6 +936,8 @@ struct eth_dev_ops {
>   	/**< Set up the connection between the pair of hairpin queues. */
>   	eth_hairpin_queue_peer_unbind_t hairpin_queue_peer_unbind;
>   	/**< Disconnect the hairpin queues of a pair from each other. */
> +	eth_get_wake_addr_t get_wake_addr;
> +	/**< Get next RX queue ring entry address. */
>   };
>   
>   /**
> diff --git a/lib/librte_ethdev/version.map b/lib/librte_ethdev/version.map
> index 8ddda2547f..f9ce4e3c8d 100644
> --- a/lib/librte_ethdev/version.map
> +++ b/lib/librte_ethdev/version.map
> @@ -235,6 +235,7 @@ EXPERIMENTAL {
>   	rte_eth_fec_get_capability;
>   	rte_eth_fec_get;
>   	rte_eth_fec_set;
> +	rte_eth_get_wake_addr;
>   	rte_flow_shared_action_create;
>   	rte_flow_shared_action_destroy;
>   	rte_flow_shared_action_query;
> 


-- 
Thanks,
Anatoly