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

Guo, Jia jia.guo at intel.com
Wed Oct 14 11:15:30 CEST 2020
Previous message: [dpdk-dev] [PATCH v5 04/10] ethdev: add simple power management API
Next message: [dpdk-dev] [PATCH v5 04/10] ethdev: add simple power management API
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
> -----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]?

> >
> >>
> >>   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
Previous message: [dpdk-dev] [PATCH v5 04/10] ethdev: add simple power management API
Next message: [dpdk-dev] [PATCH v5 04/10] ethdev: add simple power management API
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
More information about the dev mailing list