Enhancements for PMD power management

Use RTM and WAITPKG instructions to perform a wait-for-writes similar to what UMWAIT does, but without the limitation of having to listen for just one event. This works because the optimized power state used by the TPAUSE instruction will cause a wake up on RTM transaction abort, so if we add the addresses we're interested in to the read-set, any write to those addresses will wake us up.

Signed-off-by Konstantin Ananyev <[email protected]>

Signed-off-by Anatoly Burakov <[email protected]>

---

Notes:
v4:
- Fixed bugs in accessing the monitor condition
- Abort on any monitor condition not having a defined callback

v2:
- Adapt to callback mechanism

 doc/guides/rel_notes/release_21_08.rst        |  2 +
 lib/eal/arm/rte_power_intrinsics.c            | 11 +++
 lib/eal/include/generic/rte_cpuflags.h        |  2 +
 .../include/generic/rte_power_intrinsics.h    | 35 +++++++++
 lib/eal/ppc/rte_power_intrinsics.c            | 11 +++
 lib/eal/version.map                           |  3 +
 lib/eal/x86/rte_cpuflags.c                    |  2 +
 lib/eal/x86/rte_power_intrinsics.c            | 73 +++++++++++++++++++
 8 files changed, 139 insertions(+)

Currently, we expect that only one callback can be active at any given moment, for a particular queue configuration, which is relatively easy to implement in a thread-safe way. However, we're about to add support for multiple queues per lcore, which will greatly increase the possibility of various race conditions.

We could have used something like an RCU for this use case, but absent of a pressing need for thread safety we'll go the easy way and just mandate that the API's are to be called when all affected ports are stopped, and document this limitation. This greatly simplifies the rte_power_monitor-related code.

Signed-off-by Anatoly Burakov <[email protected]>

---

2.25.1

Currently, there is a hard limitation on the PMD power management support that only allows it to support a single queue per lcore. This is not ideal as most DPDK use cases will poll multiple queues per core.

The PMD power management mechanism relies on ethdev Rx callbacks, so it is very difficult to implement such support because callbacks are effectively stateless and have no visibility into what the other ethdev devices are doing. This places limitations on what we can do within the framework of Rx callbacks, but the basics of this implementation are as follows:

- Replace per-queue structures with per-lcore ones, so that any device polled from the same lcore can share data
- Any queue that is going to be polled from a specific lcore has to be added to the list of queues to poll, so that the callback is aware of other queues being polled by the same lcore
- Both the empty poll counter and the actual power saving mechanism is shared between all queues polled on a particular lcore, and is only activated when all queues in the list were polled and were determined to have no traffic.
- The limitation on UMWAIT-based polling is not removed because UMWAIT is incapable of monitoring more than one address.

Also, while we're at it, update and improve the docs.

Signed-off-by Anatoly Burakov <[email protected]>

---

Notes:
v6:
- Track each individual queue sleep status (Konstantin)
- Fix segfault (Dave)

v5:
- Remove the "power save queue" API and replace it with mechanism suggested by Konstantin

v3:
- Move the list of supported NICs to NIC feature table

v2:
- Use a TAILQ for queues instead of a static array
- Address feedback from Konstantin
- Add additional checks for stopped queues

 doc/guides/nics/features.rst           |  10 +
 doc/guides/prog_guide/power_man.rst    |  65 ++--
 doc/guides/rel_notes/release_21_08.rst |   3 +
 lib/power/rte_power_pmd_mgmt.c         | 452 +++++++++++++++++++------
 4 files changed, 394 insertions(+), 136 deletions(-)

doc/guides/nics/features.rst +10 -0

index 403c2b03a3..a96e12d155 100644
@@ -912,6 +912,16 @@ Supports to get Rx/Tx packet burst mode information.
 * **[implements] eth_dev_ops**: ``rx_burst_mode_get``, ``tx_burst_mode_get``.
 * **[related] API**: ``rte_eth_rx_burst_mode_get()``, ``rte_eth_tx_burst_mode_get()``.
 
+.. _nic_features_get_monitor_addr:
+
+PMD power management using monitor addresses
+--------------------------------------------
+
+Supports getting a monitoring condition to use together with Ethernet PMD power
+management (see :doc:`../prog_guide/power_man` for more details).
+
+* **[implements] eth_dev_ops**: ``get_monitor_addr``
+
 .. _nic_features_other:
 
 Other dev ops not represented by a Feature

doc/guides/prog_guide/power_man.rst +35 -28

index c70ae128ac..ec04a72108 100644
@@ -198,34 +198,41 @@ Ethernet PMD Power Management API
 Abstract
 ~~~~~~~~
 
-Existing power management mechanisms require developers
-to change application design or change code to make use of it.
-The PMD power management API provides a convenient alternative
-by utilizing Ethernet PMD RX callbacks,
-and triggering power saving whenever empty poll count reaches a certain number.
-
-Monitor
-   This power saving scheme will put the CPU into optimized power state
-   and use the ``rte_power_monitor()`` function
-   to monitor the Ethernet PMD RX descriptor address,
-   and wake the CPU up whenever there's new traffic.
-
-Pause
-   This power saving scheme will avoid busy polling
-   by either entering power-optimized sleep state
-   with ``rte_power_pause()`` function,
-   or, if it's not available, use ``rte_pause()``.
-
-Frequency scaling
-   This power saving scheme will use ``librte_power`` library
-   functionality to scale the core frequency up/down
-   depending on traffic volume.
-
-.. note::
-
-   Currently, this power management API is limited to mandatory mapping
-   of 1 queue to 1 core (multiple queues are supported,
-   but they must be polled from different cores).
+Existing power management mechanisms require developers to change application
+design or change code to make use of it. The PMD power management API provides a
+convenient alternative by utilizing Ethernet PMD RX callbacks, and triggering
+power saving whenever empty poll count reaches a certain number.
+
+* Monitor
+   This power saving scheme will put the CPU into optimized power state and
+   monitor the Ethernet PMD RX descriptor address, waking the CPU up whenever
+   there's new traffic. Support for this scheme may not be available on all
+   platforms, and further limitations may apply (see below).
+
+* Pause
+   This power saving scheme will avoid busy polling by either entering
+   power-optimized sleep state with ``rte_power_pause()`` function, or, if it's
+   not supported by the underlying platform, use ``rte_pause()``.
+
+* Frequency scaling
+   This power saving scheme will use ``librte_power`` library functionality to
+   scale the core frequency up/down depending on traffic volume.
+
+The "monitor" mode is only supported in the following configurations and scenarios:
+
+* If ``rte_cpu_get_intrinsics_support()`` function indicates that
+  ``rte_power_monitor()`` is supported by the platform, then monitoring will be
+  limited to a mapping of 1 core 1 queue (thus, each Rx queue will have to be
+  monitored from a different lcore).
+
+* If ``rte_cpu_get_intrinsics_support()`` function indicates that the
+  ``rte_power_monitor()`` function is not supported, then monitor mode will not
+  be supported.
+
+* Not all Ethernet drivers support monitoring, even if the underlying
+  platform may support the necessary CPU instructions. Please refer to
+  :doc:`../nics/overview` for more information.
+

doc/guides/prog_guide/power_man.rst +29 -0

+static inline void
+queue_reset(struct pmd_core_cfg *cfg, struct queue_list_entry *qcfg)
+{
+	const bool is_ready_to_sleep = qcfg->n_empty_polls > EMPTYPOLL_MAX;
+
+	/* reset empty poll counter for this queue */
+	qcfg->n_empty_polls = 0;
+	/* reset the queue sleep counter as well */
+	qcfg->n_sleeps = 0;
+	/* remove the queue from list of cores ready to sleep */
+	if (is_ready_to_sleep)
+		cfg->n_queues_ready_to_sleep--;
+	/*
+	 * no need change the lcore sleep target counter because this lcore will
+	 * reach the n_sleeps anyway, and the other cores are already counted so
+	 * there's no need to do anything else.
+	 */
+}
+
+static inline bool
+queue_can_sleep(struct pmd_core_cfg *cfg, struct queue_list_entry *qcfg)
+{
+	/* this function is called - that means we have an empty poll */
+	qcfg->n_empty_polls++;
+
+	/* if we haven't reached threshold for empty polls, we can't sleep */
+	if (qcfg->n_empty_polls <= EMPTYPOLL_MAX)
+		return false;
+

doc/guides/prog_guide/power_man.rst +25 -10

+
+	return true;
+}
+
+static inline bool
+lcore_can_sleep(struct pmd_core_cfg *cfg)
+{
+	/* are all queues ready to sleep? */
+	if (cfg->n_queues_ready_to_sleep != cfg->n_queues)
+		return false;
+
+	/* we've reached an iteration where we can sleep, reset sleep counter */
+	cfg->n_queues_ready_to_sleep = 0;
+	cfg->sleep_target++;
+
+	return true;
+}
+
 static uint16_t
 clb_umwait(uint16_t port_id, uint16_t qidx, struct rte_mbuf **pkts __rte_unused,
-		uint16_t nb_rx, uint16_t max_pkts __rte_unused,
-		void *addr __rte_unused)
+		uint16_t nb_rx, uint16_t max_pkts __rte_unused, void *arg)
 {
+	struct queue_list_entry *queue_conf = arg;
 
-	struct pmd_queue_cfg *q_conf;
-
-	q_conf = &port_cfg[port_id][qidx];
-
+	/* this callback can't do more than one queue, omit multiqueue logic */
 	if (unlikely(nb_rx == 0)) {
-		q_conf->empty_poll_stats++;
-		if (unlikely(q_conf->empty_poll_stats > EMPTYPOLL_MAX)) {
+		queue_conf->n_empty_polls++;
+		if (unlikely(queue_conf->n_empty_polls > EMPTYPOLL_MAX)) {
 			struct rte_power_monitor_cond pmc;
-			uint16_t ret;
+			int ret;
 
 			/* use monitoring condition to sleep */
 			ret = rte_eth_get_monitor_addr(port_id, qidx,

@@ -97,60 +231,77 @@ clb_umwait(uint16_t port_id, uint16_t qidx, struct rte_mbuf **pkts __rte_unused,
 				rte_power_monitor(&pmc, UINT64_MAX);
 		}
 	} else
-		q_conf->empty_poll_stats = 0;
+		queue_conf->n_empty_polls = 0;
 
 	return nb_rx;
 }
 

Quoted 46 lines

Currently, there is a hard limitation on the PMD power management support that only allows it to support a single queue per lcore. This is not ideal as most DPDK use cases will poll multiple queues per core.

The PMD power management mechanism relies on ethdev Rx callbacks, so it is very difficult to implement such support because callbacks are effectively stateless and have no visibility into what the other ethdev devices are doing. This places limitations on what we can do within the framework of Rx callbacks, but the basics of this implementation are as follows:

- Replace per-queue structures with per-lcore ones, so that any device polled from the same lcore can share data
- Any queue that is going to be polled from a specific lcore has to be added to the list of queues to poll, so that the callback is aware of other queues being polled by the same lcore
- Both the empty poll counter and the actual power saving mechanism is shared between all queues polled on a particular lcore, and is only activated when all queues in the list were polled and were determined to have no traffic.
- The limitation on UMWAIT-based polling is not removed because UMWAIT is incapable of monitoring more than one address.

Also, while we're at it, update and improve the docs.

Signed-off-by Anatoly Burakov <[email protected]>

---

Notes:
v6:
- Track each individual queue sleep status (Konstantin)
- Fix segfault (Dave)

v5:
- Remove the "power save queue" API and replace it with mechanism suggested by Konstantin

v3:
- Move the list of supported NICs to NIC feature table

v2:
- Use a TAILQ for queues instead of a static array
- Address feedback from Konstantin
- Add additional checks for stopped queues

....

Quoted 29 lines

lib/power/rte_power_pmd_mgmt.c +29 -0

+static inline void
+queue_reset(struct pmd_core_cfg *cfg, struct queue_list_entry *qcfg)
+{
+     const bool is_ready_to_sleep = qcfg->n_empty_polls > EMPTYPOLL_MAX;
+
+     /* reset empty poll counter for this queue */
+     qcfg->n_empty_polls = 0;
+     /* reset the queue sleep counter as well */
+     qcfg->n_sleeps = 0;
+     /* remove the queue from list of cores ready to sleep */
+     if (is_ready_to_sleep)
+             cfg->n_queues_ready_to_sleep--;
+     /*
+      * no need change the lcore sleep target counter because this lcore will
+      * reach the n_sleeps anyway, and the other cores are already counted so
+      * there's no need to do anything else.
+      */
+}
+
+static inline bool
+queue_can_sleep(struct pmd_core_cfg *cfg, struct queue_list_entry *qcfg)
+{
+     /* this function is called - that means we have an empty poll */
+     qcfg->n_empty_polls++;
+
+     /* if we haven't reached threshold for empty polls, we can't sleep */
+     if (qcfg->n_empty_polls <= EMPTYPOLL_MAX)
+             return false;
+

lib/power/rte_power_pmd_mgmt.c calc_tsc(void) · line 195

Quoted 10 lines

lib/power/rte_power_pmd_mgmt.c +10 -0

+     /*
+      * we've reached a point where we are able to sleep, but we still need
+      * to check if this queue has already been marked for sleeping.
+      */
+     if (qcfg->n_sleeps == cfg->sleep_target)
+             return true;
+
+     /* mark this queue as ready for sleep */
+     qcfg->n_sleeps = cfg->sleep_target;
+     cfg->n_queues_ready_to_sleep++;

So, assuming there is no incoming traffic, should it be:
1) poll_all_queues(times=EMPTYPOLL_MAX); sleep; poll_all_queues(times=1); sleep; poll_all_queues(times=1); sleep; ...
OR
2) poll_all_queues(times=EMPTYPOLL_MAX); sleep; poll_all_queues(times= EMPTYPOLL_MAX); sleep; poll_all_queues(times= EMPTYPOLL_MAX); sleep; ... ?

My initial thought was 2) but might be the intention is 1)?

Quoted 46 lines

Quoted 46 lines

Currently, there is a hard limitation on the PMD power management support that only allows it to support a single queue per lcore. This is not ideal as most DPDK use cases will poll multiple queues per core.

The PMD power management mechanism relies on ethdev Rx callbacks, so it is very difficult to implement such support because callbacks are effectively stateless and have no visibility into what the other ethdev devices are doing. This places limitations on what we can do within the framework of Rx callbacks, but the basics of this implementation are as follows:

- Replace per-queue structures with per-lcore ones, so that any device polled from the same lcore can share data
- Any queue that is going to be polled from a specific lcore has to be added to the list of queues to poll, so that the callback is aware of other queues being polled by the same lcore
- Both the empty poll counter and the actual power saving mechanism is shared between all queues polled on a particular lcore, and is only activated when all queues in the list were polled and were determined to have no traffic.
- The limitation on UMWAIT-based polling is not removed because UMWAIT is incapable of monitoring more than one address.

Also, while we're at it, update and improve the docs.

Signed-off-by Anatoly Burakov <[email protected]>

---

Notes:
v6:
- Track each individual queue sleep status (Konstantin)
- Fix segfault (Dave)

v5:
- Remove the "power save queue" API and replace it with mechanism suggested by Konstantin

v3:
- Move the list of supported NICs to NIC feature table

v2:
- Use a TAILQ for queues instead of a static array
- Address feedback from Konstantin
- Add additional checks for stopped queues

<snip>

Quoted 37 lines

....

Quoted 36 lines

lib/power/rte_power_pmd_mgmt.c +36 -0

+static inline void
+queue_reset(struct pmd_core_cfg *cfg, struct queue_list_entry *qcfg)
+{
+     const bool is_ready_to_sleep = qcfg->n_empty_polls > EMPTYPOLL_MAX;
+
+     /* reset empty poll counter for this queue */
+     qcfg->n_empty_polls = 0;
+     /* reset the queue sleep counter as well */
+     qcfg->n_sleeps = 0;
+     /* remove the queue from list of cores ready to sleep */
+     if (is_ready_to_sleep)
+             cfg->n_queues_ready_to_sleep--;
+     /*
+      * no need change the lcore sleep target counter because this lcore will
+      * reach the n_sleeps anyway, and the other cores are already counted so
+      * there's no need to do anything else.
+      */
+}
+
+static inline bool
+queue_can_sleep(struct pmd_core_cfg *cfg, struct queue_list_entry *qcfg)
+{
+     /* this function is called - that means we have an empty poll */
+     qcfg->n_empty_polls++;
+
+     /* if we haven't reached threshold for empty polls, we can't sleep */
+     if (qcfg->n_empty_polls <= EMPTYPOLL_MAX)
+             return false;
+
+     /*
+      * we've reached a point where we are able to sleep, but we still need
+      * to check if this queue has already been marked for sleeping.
+      */
+     if (qcfg->n_sleeps == cfg->sleep_target)
+             return true;
+

Quoted 8 lines

lib/power/rte_power_pmd_mgmt.c calc_tsc(void) · line 195

Quoted 3 lines

lib/power/rte_power_pmd_mgmt.c +3 -0

+     /* mark this queue as ready for sleep */
+     qcfg->n_sleeps = cfg->sleep_target;
+     cfg->n_queues_ready_to_sleep++;

So, assuming there is no incoming traffic, should it be:
1) poll_all_queues(times=EMPTYPOLL_MAX); sleep; poll_all_queues(times=1); sleep; poll_all_queues(times=1); sleep; ... OR
2) poll_all_queues(times=EMPTYPOLL_MAX); sleep; poll_all_queues(times= EMPTYPOLL_MAX); sleep; poll_all_queues(times=

EMPTYPOLL_MAX); sleep; ...

Quoted 3 lines

?

My initial thought was 2) but might be the intention is 1)?

The intent is 1), not 2). There's no need to wait for more empty polls once we pass the threshold - we keep sleeping until there's traffic.

Quoted 94 lines

Quoted 46 lines

Quoted 46 lines

Currently, there is a hard limitation on the PMD power management support that only allows it to support a single queue per lcore. This is not ideal as most DPDK use cases will poll multiple queues per core.

The PMD power management mechanism relies on ethdev Rx callbacks, so it is very difficult to implement such support because callbacks are effectively stateless and have no visibility into what the other ethdev devices are doing. This places limitations on what we can do within the framework of Rx callbacks, but the basics of this implementation are as follows:

- Replace per-queue structures with per-lcore ones, so that any device polled from the same lcore can share data
- Any queue that is going to be polled from a specific lcore has to be added to the list of queues to poll, so that the callback is aware of other queues being polled by the same lcore
- Both the empty poll counter and the actual power saving mechanism is shared between all queues polled on a particular lcore, and is only activated when all queues in the list were polled and were determined to have no traffic.
- The limitation on UMWAIT-based polling is not removed because UMWAIT is incapable of monitoring more than one address.

Also, while we're at it, update and improve the docs.

Signed-off-by Anatoly Burakov <[email protected]>

---

Notes:
v6:
- Track each individual queue sleep status (Konstantin)
- Fix segfault (Dave)

v5:
- Remove the "power save queue" API and replace it with mechanism suggested by Konstantin

v3:
- Move the list of supported NICs to NIC feature table

v2:
- Use a TAILQ for queues instead of a static array
- Address feedback from Konstantin
- Add additional checks for stopped queues

<snip>

Quoted 37 lines

....

Quoted 36 lines

lib/power/rte_power_pmd_mgmt.c +36 -0

+static inline void
+queue_reset(struct pmd_core_cfg *cfg, struct queue_list_entry *qcfg)
+{
+     const bool is_ready_to_sleep = qcfg->n_empty_polls > EMPTYPOLL_MAX;
+
+     /* reset empty poll counter for this queue */
+     qcfg->n_empty_polls = 0;
+     /* reset the queue sleep counter as well */
+     qcfg->n_sleeps = 0;
+     /* remove the queue from list of cores ready to sleep */
+     if (is_ready_to_sleep)
+             cfg->n_queues_ready_to_sleep--;
+     /*
+      * no need change the lcore sleep target counter because this lcore will
+      * reach the n_sleeps anyway, and the other cores are already counted so
+      * there's no need to do anything else.
+      */
+}
+
+static inline bool
+queue_can_sleep(struct pmd_core_cfg *cfg, struct queue_list_entry *qcfg)
+{
+     /* this function is called - that means we have an empty poll */
+     qcfg->n_empty_polls++;
+
+     /* if we haven't reached threshold for empty polls, we can't sleep */
+     if (qcfg->n_empty_polls <= EMPTYPOLL_MAX)
+             return false;
+
+     /*
+      * we've reached a point where we are able to sleep, but we still need
+      * to check if this queue has already been marked for sleeping.
+      */
+     if (qcfg->n_sleeps == cfg->sleep_target)
+             return true;
+

Quoted 8 lines

lib/power/rte_power_pmd_mgmt.c calc_tsc(void) · line 195

Quoted 3 lines

lib/power/rte_power_pmd_mgmt.c +3 -0

+     /* mark this queue as ready for sleep */
+     qcfg->n_sleeps = cfg->sleep_target;
+     cfg->n_queues_ready_to_sleep++;

So, assuming there is no incoming traffic, should it be:
1) poll_all_queues(times=EMPTYPOLL_MAX); sleep; poll_all_queues(times=1); sleep; poll_all_queues(times=1); sleep; ... OR
2) poll_all_queues(times=EMPTYPOLL_MAX); sleep; poll_all_queues(times= EMPTYPOLL_MAX); sleep; poll_all_queues(times=

Quoted 9 lines

EMPTYPOLL_MAX); sleep; ...

Quoted 3 lines

?

My initial thought was 2) but might be the intention is 1)?

The intent is 1), not 2). There's no need to wait for more empty polls once we pass the threshold - we keep sleeping until there's traffic.

Ok, then:

Acked-by Konstantin Ananyev <[email protected]>

Probably worth to put extra explanation here on in the doc, to help people avoid wrong assumptions😉

-----Original Message-----
From: Burakov, Anatoly <[email protected]>
Sent: Wednesday, July 7, 2021 12:54 PM
To: Ananyev, Konstantin <[email protected]>; [email protected]; Hunt, David <[email protected]>
Cc: Loftus, Ciara <[email protected]>
Subject: Re: [PATCH v6 5/7] power: support callbacks for multiple Rx queues

Quoted 107 lines · Konstantin

Quoted 94 lines

Quoted 46 lines

Quoted 46 lines

Currently, there is a hard limitation on the PMD power management support that only allows it to support a single queue per lcore. This is not ideal as most DPDK use cases will poll multiple queues per core.

The PMD power management mechanism relies on ethdev Rx callbacks, so it is very difficult to implement such support because callbacks are effectively stateless and have no visibility into what the other ethdev devices are doing. This places limitations on what we can do within the framework of Rx callbacks, but the basics of this implementation are as follows:

- Replace per-queue structures with per-lcore ones, so that any device polled from the same lcore can share data
- Any queue that is going to be polled from a specific lcore has to be added to the list of queues to poll, so that the callback is aware of other queues being polled by the same lcore
- Both the empty poll counter and the actual power saving mechanism is shared between all queues polled on a particular lcore, and is only activated when all queues in the list were polled and were determined to have no traffic.
- The limitation on UMWAIT-based polling is not removed because UMWAIT is incapable of monitoring more than one address.

Also, while we're at it, update and improve the docs.

Signed-off-by Anatoly Burakov <[email protected]>

---

Notes:
v6:
- Track each individual queue sleep status (Konstantin)
- Fix segfault (Dave)

v5:
- Remove the "power save queue" API and replace it with mechanism suggested by Konstantin

v3:
- Move the list of supported NICs to NIC feature table

v2:
- Use a TAILQ for queues instead of a static array
- Address feedback from Konstantin
- Add additional checks for stopped queues

<snip>

Quoted 37 lines

....

Quoted 36 lines

lib/power/rte_power_pmd_mgmt.c +36 -0

+static inline void
+queue_reset(struct pmd_core_cfg *cfg, struct queue_list_entry *qcfg)
+{
+     const bool is_ready_to_sleep = qcfg->n_empty_polls > EMPTYPOLL_MAX;
+
+     /* reset empty poll counter for this queue */
+     qcfg->n_empty_polls = 0;
+     /* reset the queue sleep counter as well */
+     qcfg->n_sleeps = 0;
+     /* remove the queue from list of cores ready to sleep */
+     if (is_ready_to_sleep)
+             cfg->n_queues_ready_to_sleep--;
+     /*
+      * no need change the lcore sleep target counter because this lcore will
+      * reach the n_sleeps anyway, and the other cores are already counted so
+      * there's no need to do anything else.
+      */
+}
+
+static inline bool
+queue_can_sleep(struct pmd_core_cfg *cfg, struct queue_list_entry *qcfg)
+{
+     /* this function is called - that means we have an empty poll */
+     qcfg->n_empty_polls++;
+
+     /* if we haven't reached threshold for empty polls, we can't sleep */
+     if (qcfg->n_empty_polls <= EMPTYPOLL_MAX)
+             return false;
+
+     /*
+      * we've reached a point where we are able to sleep, but we still need
+      * to check if this queue has already been marked for sleeping.
+      */
+     if (qcfg->n_sleeps == cfg->sleep_target)
+             return true;
+

Quoted 8 lines

lib/power/rte_power_pmd_mgmt.c calc_tsc(void) · line 195

Quoted 3 lines

lib/power/rte_power_pmd_mgmt.c +3 -0

+     /* mark this queue as ready for sleep */
+     qcfg->n_sleeps = cfg->sleep_target;
+     cfg->n_queues_ready_to_sleep++;

So, assuming there is no incoming traffic, should it be:
1) poll_all_queues(times=EMPTYPOLL_MAX); sleep; poll_all_queues(times=1); sleep; poll_all_queues(times=1); sleep; ... OR
2) poll_all_queues(times=EMPTYPOLL_MAX); sleep; poll_all_queues(times= EMPTYPOLL_MAX); sleep; poll_all_queues(times=

Quoted 9 lines

EMPTYPOLL_MAX); sleep; ...

Quoted 3 lines

?

My initial thought was 2) but might be the intention is 1)?

The intent is 1), not 2). There's no need to wait for more empty polls once we pass the threshold - we keep sleeping until there's traffic.

Ok, then:

Acked-by Konstantin Ananyev <[email protected]>

Quoted 3 lines

Probably worth to put extra explanation here on in the doc, to help people avoid wrong assumptions😉

I don't see value in going into such details. What would be the point? Like, what difference would this information make to anyone?

Quoted 121 lines

-----Original Message-----
From: Burakov, Anatoly <[email protected]>
Sent: Wednesday, July 7, 2021 12:54 PM
To: Ananyev, Konstantin <[email protected]>; [email protected]; Hunt, David <[email protected]>
Cc: Loftus, Ciara <[email protected]>
Subject: Re: [PATCH v6 5/7] power: support callbacks for multiple Rx queues

Quoted 110 lines · Konstantin

Quoted 94 lines

Quoted 46 lines

Quoted 46 lines

Currently, there is a hard limitation on the PMD power management support that only allows it to support a single queue per lcore. This is not ideal as most DPDK use cases will poll multiple queues per core.

The PMD power management mechanism relies on ethdev Rx callbacks, so it is very difficult to implement such support because callbacks are effectively stateless and have no visibility into what the other ethdev devices are doing. This places limitations on what we can do within the framework of Rx callbacks, but the basics of this implementation are as follows:

- Replace per-queue structures with per-lcore ones, so that any device polled from the same lcore can share data
- Any queue that is going to be polled from a specific lcore has to be added to the list of queues to poll, so that the callback is aware of other queues being polled by the same lcore
- Both the empty poll counter and the actual power saving mechanism is shared between all queues polled on a particular lcore, and is only activated when all queues in the list were polled and were determined to have no traffic.
- The limitation on UMWAIT-based polling is not removed because UMWAIT is incapable of monitoring more than one address.

Also, while we're at it, update and improve the docs.

Signed-off-by Anatoly Burakov <[email protected]>

---

Notes:
v6:
- Track each individual queue sleep status (Konstantin)
- Fix segfault (Dave)

v5:
- Remove the "power save queue" API and replace it with mechanism suggested by Konstantin

v3:
- Move the list of supported NICs to NIC feature table

v2:
- Use a TAILQ for queues instead of a static array
- Address feedback from Konstantin
- Add additional checks for stopped queues

<snip>

Quoted 37 lines

....

Quoted 36 lines

lib/power/rte_power_pmd_mgmt.c +36 -0

+static inline void
+queue_reset(struct pmd_core_cfg *cfg, struct queue_list_entry *qcfg)
+{
+     const bool is_ready_to_sleep = qcfg->n_empty_polls > EMPTYPOLL_MAX;
+
+     /* reset empty poll counter for this queue */
+     qcfg->n_empty_polls = 0;
+     /* reset the queue sleep counter as well */
+     qcfg->n_sleeps = 0;
+     /* remove the queue from list of cores ready to sleep */
+     if (is_ready_to_sleep)
+             cfg->n_queues_ready_to_sleep--;
+     /*
+      * no need change the lcore sleep target counter because this lcore will
+      * reach the n_sleeps anyway, and the other cores are already counted so
+      * there's no need to do anything else.
+      */
+}
+
+static inline bool
+queue_can_sleep(struct pmd_core_cfg *cfg, struct queue_list_entry *qcfg)
+{
+     /* this function is called - that means we have an empty poll */
+     qcfg->n_empty_polls++;
+
+     /* if we haven't reached threshold for empty polls, we can't sleep */
+     if (qcfg->n_empty_polls <= EMPTYPOLL_MAX)
+             return false;
+
+     /*
+      * we've reached a point where we are able to sleep, but we still need
+      * to check if this queue has already been marked for sleeping.
+      */
+     if (qcfg->n_sleeps == cfg->sleep_target)
+             return true;
+

Quoted 8 lines

lib/power/rte_power_pmd_mgmt.c calc_tsc(void) · line 195

Quoted 3 lines

lib/power/rte_power_pmd_mgmt.c +3 -0

+     /* mark this queue as ready for sleep */
+     qcfg->n_sleeps = cfg->sleep_target;
+     cfg->n_queues_ready_to_sleep++;

So, assuming there is no incoming traffic, should it be:
1) poll_all_queues(times=EMPTYPOLL_MAX); sleep; poll_all_queues(times=1); sleep; poll_all_queues(times=1); sleep; ... OR
2) poll_all_queues(times=EMPTYPOLL_MAX); sleep; poll_all_queues(times= EMPTYPOLL_MAX); sleep; poll_all_queues(times=

Quoted 9 lines

EMPTYPOLL_MAX); sleep; ...

Quoted 3 lines

?

My initial thought was 2) but might be the intention is 1)?

The intent is 1), not 2). There's no need to wait for more empty polls once we pass the threshold - we keep sleeping until there's traffic.

Ok, then:

Acked-by Konstantin Ananyev <[email protected]>

Probably worth to put extra explanation here on in the doc, to help people avoid wrong assumptions😉

Quoted 2 lines

I don't see value in going into such details. What would be the point? Like, what difference would this information make to anyone?

I thought it is obvious: if you put extra explanation into the code, then it would be easier for anyone who reads it (reviewers/maintainers/users) to understand what it supposed to do.

Quoted 115 lines

Quoted 114 lines

Quoted 105 lines

Quoted 95 lines

Quoted 47 lines

Quoted 47 lines

Currently, there is a hard limitation on the PMD power management support that only allows it to support a single queue per lcore. This is not ideal as most DPDK use cases will poll multiple queues per core.

The PMD power management mechanism relies on ethdev Rx callbacks, so it is very difficult to implement such support because callbacks are effectively stateless and have no visibility into what the other ethdev devices are doing. This places limitations on what we can do within the framework of Rx callbacks, but the basics of this implementation are as follows:

- Replace per-queue structures with per-lcore ones, so that any device polled from the same lcore can share data
- Any queue that is going to be polled from a specific lcore has to be added to the list of queues to poll, so that the callback is aware of other queues being polled by the same lcore
- Both the empty poll counter and the actual power saving mechanism is shared between all queues polled on a particular lcore, and is only activated when all queues in the list were polled and were determined to have no traffic.
- The limitation on UMWAIT-based polling is not removed because UMWAIT is incapable of monitoring more than one address.

Also, while we're at it, update and improve the docs.

Signed-off-by Anatoly Burakov <[email protected]>

---

Notes:
v6:
- Track each individual queue sleep status (Konstantin)
- Fix segfault (Dave)

v5:
- Remove the "power save queue" API and replace it with mechanism suggested by Konstantin

v3:
- Move the list of supported NICs to NIC feature table

v2:
- Use a TAILQ for queues instead of a static array
- Address feedback from Konstantin
- Add additional checks for stopped queues

<snip>

Quoted 37 lines

....

Quoted 36 lines

(quoted diff) +36 -0

+static inline void
+queue_reset(struct pmd_core_cfg *cfg, struct queue_list_entry *qcfg)
+{
+     const bool is_ready_to_sleep = qcfg->n_empty_polls > EMPTYPOLL_MAX;
+
+     /* reset empty poll counter for this queue */
+     qcfg->n_empty_polls = 0;
+     /* reset the queue sleep counter as well */
+     qcfg->n_sleeps = 0;
+     /* remove the queue from list of cores ready to sleep */
+     if (is_ready_to_sleep)
+             cfg->n_queues_ready_to_sleep--;
+     /*
+      * no need change the lcore sleep target counter because this lcore will
+      * reach the n_sleeps anyway, and the other cores are already counted so
+      * there's no need to do anything else.
+      */
+}
+
+static inline bool
+queue_can_sleep(struct pmd_core_cfg *cfg, struct queue_list_entry *qcfg)
+{
+     /* this function is called - that means we have an empty poll */
+     qcfg->n_empty_polls++;
+
+     /* if we haven't reached threshold for empty polls, we can't sleep */
+     if (qcfg->n_empty_polls <= EMPTYPOLL_MAX)
+             return false;
+
+     /*
+      * we've reached a point where we are able to sleep, but we still need
+      * to check if this queue has already been marked for sleeping.
+      */
+     if (qcfg->n_sleeps == cfg->sleep_target)
+             return true;
+

Quoted 8 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+     /* mark this queue as ready for sleep */
+     qcfg->n_sleeps = cfg->sleep_target;
+     cfg->n_queues_ready_to_sleep++;

So, assuming there is no incoming traffic, should it be:
1) poll_all_queues(times=EMPTYPOLL_MAX); sleep; poll_all_queues(times=1); sleep; poll_all_queues(times=1); sleep; ... OR
2) poll_all_queues(times=EMPTYPOLL_MAX); sleep; poll_all_queues(times= EMPTYPOLL_MAX); sleep; poll_all_queues(times=

Quoted 9 lines

EMPTYPOLL_MAX); sleep; ...

Quoted 3 lines

?

My initial thought was 2) but might be the intention is 1)?

The intent is 1), not 2). There's no need to wait for more empty polls once we pass the threshold - we keep sleeping until there's traffic.

Quoted 6 lines

Ok, then:

Acked-by Konstantin Ananyev <[email protected]>

Probably worth to put extra explanation here on in the doc, to help people avoid wrong assumptions😉

I don't see value in going into such details. What would be the point? Like, what difference would this information make to anyone?

Quoted 4 lines

I thought it is obvious: if you put extra explanation into the code, then it would be easier for anyone who reads it (reviewers/maintainers/users) to understand what it supposed to do.

You're suggesting to put this *in the doc*, which implies that *the user* will find this information useful. I'm OK with adding this info as a comment somewhere perhaps, but why put it in the doc?

Currently, there is a hard limitation on the PMD power management support that only allows it to support a single queue per lcore. This is not ideal as most DPDK use cases will poll multiple queues per core.

The PMD power management mechanism relies on ethdev Rx callbacks, so it is very difficult to implement such support because callbacks are effectively stateless and have no visibility into what the other ethdev devices are doing. This places limitations on what we can do within the framework of Rx callbacks, but the basics of this implementation are as follows:

- Replace per-queue structures with per-lcore ones, so that any device polled from the same lcore can share data
- Any queue that is going to be polled from a specific lcore has to be added to the list of queues to poll, so that the callback is aware of other queues being polled by the same lcore
- Both the empty poll counter and the actual power saving mechanism is shared between all queues polled on a particular lcore, and is only activated when all queues in the list were polled and were determined to have no traffic.
- The limitation on UMWAIT-based polling is not removed because UMWAIT is incapable of monitoring more than one address.

Also, while we're at it, update and improve the docs.

Signed-off-by Anatoly Burakov <[email protected]>

---

Notes:
v6:
- Track each individual queue sleep status (Konstantin)
- Fix segfault (Dave)

v5:
- Remove the "power save queue" API and replace it with mechanism suggested by Konstantin

v3:
- Move the list of supported NICs to NIC feature table

v2:
- Use a TAILQ for queues instead of a static array
- Address feedback from Konstantin
- Add additional checks for stopped queues

  doc/guides/nics/features.rst           |  10 +
  doc/guides/prog_guide/power_man.rst    |  65 ++--
  doc/guides/rel_notes/release_21_08.rst |   3 +
  lib/power/rte_power_pmd_mgmt.c         | 452 +++++++++++++++++++------
  4 files changed, 394 insertions(+), 136 deletions(-)

Quoted 42 lines · Anatoly Burakov

Currently, there is a hard limitation on the PMD power management support that only allows it to support a single queue per lcore. This is not ideal as most DPDK use cases will poll multiple queues per core.

The PMD power management mechanism relies on ethdev Rx callbacks, so it is very difficult to implement such support because callbacks are effectively stateless and have no visibility into what the other ethdev devices are doing. This places limitations on what we can do within the framework of Rx callbacks, but the basics of this implementation are as follows:

- Replace per-queue structures with per-lcore ones, so that any device polled from the same lcore can share data
- Any queue that is going to be polled from a specific lcore has to be added to the list of queues to poll, so that the callback is aware of other queues being polled by the same lcore
- Both the empty poll counter and the actual power saving mechanism is shared between all queues polled on a particular lcore, and is only activated when all queues in the list were polled and were determined to have no traffic.
- The limitation on UMWAIT-based polling is not removed because UMWAIT is incapable of monitoring more than one address.

Also, while we're at it, update and improve the docs.

Signed-off-by Anatoly Burakov <[email protected]>

---

Notes:
     v6:
     - Track each individual queue sleep status (Konstantin)
     - Fix segfault (Dave)
     v5:
     - Remove the "power save queue" API and replace it with mechanism suggested by Konstantin
     v3:
     - Move the list of supported NICs to NIC feature table
     v2:
     - Use a TAILQ for queues instead of a static array
     - Address feedback from Konstantin
     - Add additional checks for stopped queues

Quoted 6 lines

  doc/guides/nics/features.rst           |  10 + doc/guides/prog_guide/power_man.rst    |  65 ++-- doc/guides/rel_notes/release_21_08.rst |   3 + lib/power/rte_power_pmd_mgmt.c         | 452 +++++++++++++++++++------

  4 files changed, 394 insertions(+), 136 deletions(-)

--snip--

lib/power/rte_power_pmd_mgmt.c calc_tsc(void) · line 168

Quoted 12 lines

lib/power/rte_power_pmd_mgmt.c +12 -0

+static inline void
+queue_reset(struct pmd_core_cfg *cfg, struct queue_list_entry *qcfg)
+{
+    const bool is_ready_to_sleep = qcfg->n_empty_polls > EMPTYPOLL_MAX;
+
+    /* reset empty poll counter for this queue */
+    qcfg->n_empty_polls = 0;
+    /* reset the queue sleep counter as well */
+    qcfg->n_sleeps = 0;
+    /* remove the queue from list of cores ready to sleep */
+    if (is_ready_to_sleep)
+        cfg->n_queues_ready_to_sleep--;

Hi Anatoly,

   I don't think the logic around this is bulletproof yet, in my
testing I'm seeing n_queues_ready_to_sleep wrap around (i.e. decremented while already zero).

Rgds,
Dave.

--snip--

Use the new multi-monitor intrinsic to allow monitoring multiple ethdev Rx queues while entering the energy efficient power state. The multi version will be used unconditionally if supported, and the UMWAIT one will only be used when multi-monitor is not supported by the hardware.

Signed-off-by Anatoly Burakov <[email protected]>

---

--
2.25.1

Previously, the semantics of power monitor were such that we were checking current value against the expected value, and if they matched, then the sleep was aborted. This is somewhat inflexible, because it only allowed us to check for a specific value in a specific way.

This commit replaces the comparison with a user callback mechanism, so that any PMD (or other code) using rte_power_monitor() can define their own comparison semantics and decision making on how to detect the need to abort the entering of power optimized state.

Existing implementations are adjusted to follow the new semantics.

Suggested-by Konstantin Ananyev <[email protected]>

Signed-off-by Anatoly Burakov <[email protected]>

Acked-by Konstantin Ananyev <[email protected]>

---

Notes:
v4:
- Return error if callback is set to NULL
- Replace raw number with a macro in monitor condition opaque data

v2:
- Use callback mechanism for more flexibility
- Address feedback from Konstantin

Use RTM and WAITPKG instructions to perform a wait-for-writes similar to what UMWAIT does, but without the limitation of having to listen for just one event. This works because the optimized power state used by the TPAUSE instruction will cause a wake up on RTM transaction abort, so if we add the addresses we're interested in to the read-set, any write to those addresses will wake us up.

Signed-off-by Konstantin Ananyev <[email protected]>

Signed-off-by Anatoly Burakov <[email protected]>

---

Notes:
v4:
- Fixed bugs in accessing the monitor condition
- Abort on any monitor condition not having a defined callback

v2:
- Adapt to callback mechanism

Currently, we expect that only one callback can be active at any given moment, for a particular queue configuration, which is relatively easy to implement in a thread-safe way. However, we're about to add support for multiple queues per lcore, which will greatly increase the possibility of various race conditions.

We could have used something like an RCU for this use case, but absent of a pressing need for thread safety we'll go the easy way and just mandate that the API's are to be called when all affected ports are stopped, and document this limitation. This greatly simplifies the rte_power_monitor-related code.

Signed-off-by Anatoly Burakov <[email protected]>

---

Notes:
v2:
- Add check for stopped queue
- Clarified doc message
- Added release notes

Currently, there is a hard limitation on the PMD power management support that only allows it to support a single queue per lcore. This is not ideal as most DPDK use cases will poll multiple queues per core.

The PMD power management mechanism relies on ethdev Rx callbacks, so it is very difficult to implement such support because callbacks are effectively stateless and have no visibility into what the other ethdev devices are doing. This places limitations on what we can do within the framework of Rx callbacks, but the basics of this implementation are as follows:

- Replace per-queue structures with per-lcore ones, so that any device polled from the same lcore can share data
- Any queue that is going to be polled from a specific lcore has to be added to the list of queues to poll, so that the callback is aware of other queues being polled by the same lcore
- Both the empty poll counter and the actual power saving mechanism is shared between all queues polled on a particular lcore, and is only activated when all queues in the list were polled and were determined to have no traffic.
- The limitation on UMWAIT-based polling is not removed because UMWAIT is incapable of monitoring more than one address.

Also, while we're at it, update and improve the docs.

Signed-off-by Anatoly Burakov <[email protected]>

---

Notes:
v7:
- Fix bug where initial sleep target was always set to zero
- Fix logic in handling of n_queues_ready_to_sleep counter
- Update documentation on hardware requirements

v6:
- Track each individual queue sleep status (Konstantin)
- Fix segfault (Dave)

v5:
- Remove the "power save queue" API and replace it with mechanism suggested by Konstantin

v3:
- Move the list of supported NICs to NIC feature table

v2:
- Use a TAILQ for queues instead of a static array
- Address feedback from Konstantin
- Add additional checks for stopped queues

  doc/guides/nics/features.rst           |  10 +
  doc/guides/prog_guide/power_man.rst    |  69 ++--
  doc/guides/rel_notes/release_21_08.rst |   3 +
  lib/power/rte_power_pmd_mgmt.c         | 456 +++++++++++++++++++------
  4 files changed, 402 insertions(+), 136 deletions(-)

Use the new multi-monitor intrinsic to allow monitoring multiple ethdev Rx queues while entering the energy efficient power state. The multi version will be used unconditionally if supported, and the UMWAIT one will only be used when multi-monitor is not supported by the hardware.

Signed-off-by Anatoly Burakov <[email protected]>

---

Notes:
v6:
- Fix the missed feedback from v5

v4:
- Fix possible out of bounds access
- Added missing index increment

Currently, l3fwd-power enforces the limitation of having one queue per lcore. This is no longer necessary, so remove the limitation.

Signed-off-by Anatoly Burakov <[email protected]>

---
  examples/l3fwd-power/main.c | 6 ------
  1 file changed, 6 deletions(-)

examples/l3fwd-power/main.c +0 -0

index f8dfed1634..52f56dc405 100644
@@ -2723,12 +2723,6 @@ main(int argc, char **argv)
  		printf("\nInitializing rx queues on lcore %u ... ", lcore_id );
  		fflush(stdout);

-----Original Message-----
From: Burakov, Anatoly <[email protected]>
Sent: Thursday, July 8, 2021 9:14 AM
To: [email protected]; McDaniel, Timothy <[email protected]>; Xing, Beilei <[email protected]>; Wu, Jingjing <[email protected]>; Yang, Qiming <[email protected]>; Zhang, Qi Z <[email protected]>; Wang, Haiyue <[email protected]>; Matan Azrad <[email protected]>; Shahaf Shuler <[email protected]>; Viacheslav Ovsiienko <[email protected]>; Richardson, Bruce <[email protected]>; Ananyev, Konstantin <[email protected]>
Cc: Loftus, Ciara <[email protected]>; Hunt, David <[email protected]>
Subject: [PATCH v8 1/7] power_intrinsics: use callbacks for comparison

Previously, the semantics of power monitor were such that we were checking current value against the expected value, and if they matched, then the sleep was aborted. This is somewhat inflexible, because it only allowed us to check for a specific value in a specific way.

This commit replaces the comparison with a user callback mechanism, so that any PMD (or other code) using rte_power_monitor() can define their own comparison semantics and decision making on how to detect the need to abort the entering of power optimized state.

Existing implementations are adjusted to follow the new semantics.

Suggested-by Konstantin Ananyev <[email protected]>

Signed-off-by Anatoly Burakov <[email protected]>

Acked-by Konstantin Ananyev <[email protected]>

---

Notes:
v4:
- Return error if callback is set to NULL
- Replace raw number with a macro in monitor condition opaque data

v2:
- Use callback mechanism for more flexibility
- Address feedback from Konstantin

 doc/guides/rel_notes/release_21_08.rst        |  2 ++
 drivers/event/dlb2/dlb2.c                     | 17 ++++++++--
 drivers/net/i40e/i40e_rxtx.c                  | 20 +++++++----
 drivers/net/iavf/iavf_rxtx.c                  | 20 +++++++----
 drivers/net/ice/ice_rxtx.c                    | 20 +++++++----
 drivers/net/ixgbe/ixgbe_rxtx.c                | 20 +++++++----
 drivers/net/mlx5/mlx5_rx.c                    | 17 ++++++++--
 .../include/generic/rte_power_intrinsics.h    | 33 +++++++++++++++----
 lib/eal/x86/rte_power_intrinsics.c            | 17 +++++-----
 9 files changed, 122 insertions(+), 44 deletions(-)

doc/guides/rel_notes/release_21_08.rst +2 -0

index c92e016783..65910de348 100644
@@ -135,6 +135,8 @@ API Changes
 * eal: ``rte_strscpy`` sets ``rte_errno`` to ``E2BIG`` in case of string
   truncation.
 
+* eal: the ``rte_power_intrinsics`` API changed to use a callback mechanism.
+
 
 ABI Changes
 -----------

drivers/event/dlb2/dlb2.c +10 -0

index eca183753f..252bbd8d5e 100644
@@ -3154,6 +3154,16 @@ dlb2_port_credits_inc(struct dlb2_port *qm_port,
 int num)
 	}
 }
 
+#define CLB_MASK_IDX 0
+#define CLB_VAL_IDX 1
+static int
+dlb2_monitor_callback(const uint64_t val,
+		const uint64_t opaque[RTE_POWER_MONITOR_OPAQUE_SZ])
+{
+	/* abort if the value matches */
+	return (val & opaque[CLB_MASK_IDX]) == opaque[CLB_VAL_IDX] ? -1 :
 0;
+}
+
 static inline int

dlb2_dequeue_wait(struct dlb2_eventdev *dlb2,

drivers/event/dlb2/dlb2.c +5 -2

 		  struct dlb2_eventdev_port *ev_port,

@@ -3194,8 +3204,11 @@ dlb2_dequeue_wait(struct dlb2_eventdev *dlb2,
 			expected_value = 0;
 
 		pmc.addr = monitor_addr;
-		pmc.val = expected_value;
-		pmc.mask = qe_mask.raw_qe[1];
+		/* store expected value and comparison mask in opaque data */
+		pmc.opaque[CLB_VAL_IDX] = expected_value;
+		pmc.opaque[CLB_MASK_IDX] = qe_mask.raw_qe[1];
+		/* set up callback */
+		pmc.fn = dlb2_monitor_callback;
 		pmc.size = sizeof(uint64_t);
 
 		rte_power_monitor(&pmc, timeout + start_ticks);

drivers/net/i40e/i40e_rxtx.c +12 -0

index 8d65f287f4..65f325ede1 100644
@@ -81,6 +81,18 @@
 #define I40E_TX_OFFLOAD_SIMPLE_NOTSUP_MASK \
 		(PKT_TX_OFFLOAD_MASK ^
 I40E_TX_OFFLOAD_SIMPLE_SUP_MASK)
 
+static int
+i40e_monitor_callback(const uint64_t value,
+		const uint64_t arg[RTE_POWER_MONITOR_OPAQUE_SZ]
 __rte_unused)
+{
+	const uint64_t m = rte_cpu_to_le_64(1 <<
 I40E_RX_DESC_STATUS_DD_SHIFT);
+	/*
+	 * we expect the DD bit to be set to 1 if this descriptor was already
+	 * written to.
+	 */
+	return (value & m) == m ? -1 : 0;
+}
+

int
i40e_get_monitor_addr(void *rx_queue, struct rte_power_monitor_cond
*pmc)
{

drivers/net/i40e/i40e_rxtx.c +2 -6

@@ -93,12 +105,8 @@ i40e_get_monitor_addr(void *rx_queue, struct
 rte_power_monitor_cond *pmc)
 	/* watch for changes in status bit */
 	pmc->addr = &rxdp->wb.qword1.status_error_len;
 
-	/*
-	 * we expect the DD bit to be set to 1 if this descriptor was already
-	 * written to.
-	 */
-	pmc->val = rte_cpu_to_le_64(1 << I40E_RX_DESC_STATUS_DD_SHIFT);
-	pmc->mask = rte_cpu_to_le_64(1 <<
 I40E_RX_DESC_STATUS_DD_SHIFT);
+	/* comparison callback */
+	pmc->fn = i40e_monitor_callback;
 

drivers/net/i40e/i40e_rxtx.c +0 -0

 	/* registers are 64-bit */
 	pmc->size = sizeof(uint64_t);

drivers/net/iavf/iavf_rxtx.c +12 -0

index f817fbc49b..d61b32fcee 100644
@@ -57,6 +57,18 @@ iavf_proto_xtr_type_to_rxdid(uint8_t flex_type)
 				rxdid_map[flex_type] :
 IAVF_RXDID_COMMS_OVS_1;
 }
 
+static int
+iavf_monitor_callback(const uint64_t value,
+		const uint64_t arg[RTE_POWER_MONITOR_OPAQUE_SZ]
 __rte_unused)
+{
+	const uint64_t m = rte_cpu_to_le_64(1 <<
 IAVF_RX_DESC_STATUS_DD_SHIFT);
+	/*
+	 * we expect the DD bit to be set to 1 if this descriptor was already
+	 * written to.
+	 */
+	return (value & m) == m ? -1 : 0;
+}
+

int
iavf_get_monitor_addr(void *rx_queue, struct rte_power_monitor_cond *pmc) {

drivers/net/iavf/iavf_rxtx.c +2 -6

@@ -69,12 +81,8 @@ iavf_get_monitor_addr(void *rx_queue, struct
 rte_power_monitor_cond *pmc)
 	/* watch for changes in status bit */
 	pmc->addr = &rxdp->wb.qword1.status_error_len;
 
-	/*
-	 * we expect the DD bit to be set to 1 if this descriptor was already
-	 * written to.
-	 */
-	pmc->val = rte_cpu_to_le_64(1 << IAVF_RX_DESC_STATUS_DD_SHIFT);
-	pmc->mask = rte_cpu_to_le_64(1 <<
 IAVF_RX_DESC_STATUS_DD_SHIFT);
+	/* comparison callback */
+	pmc->fn = iavf_monitor_callback;
 

drivers/net/iavf/iavf_rxtx.c +0 -0

 	/* registers are 64-bit */
 	pmc->size = sizeof(uint64_t);

drivers/net/ice/ice_rxtx.c +12 -0

index 3f6e735984..5d7ab4f047 100644
@@ -27,6 +27,18 @@ uint64_t
 rte_net_ice_dynflag_proto_xtr_ipv6_flow_mask;
 uint64_t rte_net_ice_dynflag_proto_xtr_tcp_mask;
 uint64_t rte_net_ice_dynflag_proto_xtr_ip_offset_mask;
 
+static int
+ice_monitor_callback(const uint64_t value,
+		const uint64_t arg[RTE_POWER_MONITOR_OPAQUE_SZ]
 __rte_unused)
+{
+	const uint64_t m = rte_cpu_to_le_16(1 <<
 ICE_RX_FLEX_DESC_STATUS0_DD_S);
+	/*
+	 * we expect the DD bit to be set to 1 if this descriptor was already
+	 * written to.
+	 */
+	return (value & m) == m ? -1 : 0;
+}
+

int
ice_get_monitor_addr(void *rx_queue, struct rte_power_monitor_cond *pmc) {

drivers/net/ice/ice_rxtx.c +2 -6

@@ -39,12 +51,8 @@ ice_get_monitor_addr(void *rx_queue, struct
 rte_power_monitor_cond *pmc)
 	/* watch for changes in status bit */
 	pmc->addr = &rxdp->wb.status_error0;
 
-	/*
-	 * we expect the DD bit to be set to 1 if this descriptor was already
-	 * written to.
-	 */
-	pmc->val = rte_cpu_to_le_16(1 << ICE_RX_FLEX_DESC_STATUS0_DD_S);
-	pmc->mask = rte_cpu_to_le_16(1 <<
 ICE_RX_FLEX_DESC_STATUS0_DD_S);
+	/* comparison callback */
+	pmc->fn = ice_monitor_callback;
 

drivers/net/ice/ice_rxtx.c +0 -0

 	/* register is 16-bit */
 	pmc->size = sizeof(uint16_t);

drivers/net/ixgbe/ixgbe_rxtx.c +12 -0

index d69f36e977..c814a28cb4 100644
@@ -1369,6 +1369,18 @@ const uint32_t
 		RTE_PTYPE_INNER_L3_IPV4_EXT |
 RTE_PTYPE_INNER_L4_UDP,
 };
 
+static int
+ixgbe_monitor_callback(const uint64_t value,
+		const uint64_t arg[RTE_POWER_MONITOR_OPAQUE_SZ]
 __rte_unused)
+{
+	const uint64_t m = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
+	/*
+	 * we expect the DD bit to be set to 1 if this descriptor was already
+	 * written to.
+	 */
+	return (value & m) == m ? -1 : 0;
+}
+
 int

ixgbe_get_monitor_addr(void *rx_queue, struct rte_power_monitor_cond
*pmc)
{

drivers/net/ixgbe/ixgbe_rxtx.c +2 -6

@@ -1381,12 +1393,8 @@ ixgbe_get_monitor_addr(void *rx_queue, struct
 rte_power_monitor_cond *pmc)
 	/* watch for changes in status bit */
 	pmc->addr = &rxdp->wb.upper.status_error;
 
-	/*
-	 * we expect the DD bit to be set to 1 if this descriptor was already
-	 * written to.
-	 */
-	pmc->val = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
-	pmc->mask = rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
+	/* comparison callback */
+	pmc->fn = ixgbe_monitor_callback;
 
 	/* the registers are 32-bit */

drivers/net/ixgbe/ixgbe_rxtx.c +0 -0

 	pmc->size = sizeof(uint32_t);

drivers/net/mlx5/mlx5_rx.c +12 -0

index 777a1d6e45..17370b77dc 100644
@@ -269,6 +269,18 @@ mlx5_rx_queue_count(struct rte_eth_dev *dev,
 uint16_t rx_queue_id)
 	return rx_queue_count(rxq);
 }
 
+#define CLB_VAL_IDX 0
+#define CLB_MSK_IDX 1
+static int
+mlx_monitor_callback(const uint64_t value,
+		const uint64_t opaque[RTE_POWER_MONITOR_OPAQUE_SZ])
+{
+	const uint64_t m = opaque[CLB_MSK_IDX];
+	const uint64_t v = opaque[CLB_VAL_IDX];
+
+	return (value & m) == v ? -1 : 0;
+}
+
 int mlx5_get_monitor_addr(void *rx_queue, struct rte_power_monitor_cond
 *pmc)

{

drivers/net/mlx5/mlx5_rx.c +3 -2

 	struct mlx5_rxq_data *rxq = rx_queue;

@@ -282,8 +294,9 @@ int mlx5_get_monitor_addr(void *rx_queue, struct
 rte_power_monitor_cond *pmc)
 		return -rte_errno;
 	}
 	pmc->addr = &cqe->op_own;
-	pmc->val =  !!idx;
-	pmc->mask = MLX5_CQE_OWNER_MASK;
+	pmc->opaque[CLB_VAL_IDX] = !!idx;
+	pmc->opaque[CLB_MSK_IDX] = MLX5_CQE_OWNER_MASK;
+	pmc->fn = mlx_monitor_callback;
 	pmc->size = sizeof(uint8_t);
 	return 0;

}

lib/eal/include/generic/rte_power_intrinsics.h +26 -7

index dddca3d41c..c9aa52a86d 100644
@@ -18,19 +18,38 @@
  * which are architecture-dependent.
  */
 
+/** Size of the opaque data in monitor condition */
+#define RTE_POWER_MONITOR_OPAQUE_SZ 4
+
+/**
+ * Callback definition for monitoring conditions. Callbacks with this signature
+ * will be used by `rte_power_monitor()` to check if the entering of power
+ * optimized state should be aborted.
+ *
+ * @param val
+ *   The value read from memory.
+ * @param opaque
+ *   Callback-specific data.
+ *
+ * @return
+ *   0 if entering of power optimized state should proceed
+ *   -1 if entering of power optimized state should be aborted
+ */
+typedef int (*rte_power_monitor_clb_t)(const uint64_t val,
+		const uint64_t opaque[RTE_POWER_MONITOR_OPAQUE_SZ]);
 struct rte_power_monitor_cond {
 	volatile void *addr;  /**< Address to monitor for changes */
-	uint64_t val;         /**< If the `mask` is non-zero, location pointed
-	                       *   to by `addr` will be read and compared
-	                       *   against this value.
-	                       */
-	uint64_t mask;   /**< 64-bit mask to extract value read from `addr` */
-	uint8_t size;    /**< Data size (in bytes) that will be used to compare
-	                  *   expected value (`val`) with data read from the
+	uint8_t size;    /**< Data size (in bytes) that will be read from the
 	                  *   monitored memory location (`addr`). Can be 1, 2,
 	                  *   4, or 8. Supplying any other value will result in
 	                  *   an error.
 	                  */
+	rte_power_monitor_clb_t fn; /**< Callback to be used to check if
+	                             *   entering power optimized state should
+	                             *   be aborted.
+	                             */
+	uint64_t opaque[RTE_POWER_MONITOR_OPAQUE_SZ];
+	/**< Callback-specific data */
 };
 
 /**

lib/eal/x86/rte_power_intrinsics.c +1 -0

index 39ea9fdecd..66fea28897 100644
@@ -76,6 +76,7 @@ rte_power_monitor(const struct
 rte_power_monitor_cond *pmc,
 	const uint32_t tsc_h = (uint32_t)(tsc_timestamp >> 32);
 	const unsigned int lcore_id = rte_lcore_id();
 	struct power_wait_status *s;
+	uint64_t cur_value;
 
 	/* prevent user from running this instruction if it's not supported */

lib/eal/x86/rte_power_intrinsics.c +3 -0

 	if (!wait_supported)

@@ -91,6 +92,9 @@ rte_power_monitor(const struct
 rte_power_monitor_cond *pmc,
 	if (__check_val_size(pmc->size) < 0)
 		return -EINVAL;
 
+	if (pmc->fn == NULL)
+		return -EINVAL;
+
 	s = &wait_status[lcore_id];
 

lib/eal/x86/rte_power_intrinsics.c +4 -9

 	/* update sleep address */

@@ -110,16 +114,11 @@ rte_power_monitor(const struct
 rte_power_monitor_cond *pmc,
 	/* now that we've put this address into monitor, we can unlock */
 	rte_spinlock_unlock(&s->lock);
 
-	/* if we have a comparison mask, we might not need to sleep at all */
-	if (pmc->mask) {
-		const uint64_t cur_value = __get_umwait_val(
-				pmc->addr, pmc->size);
-		const uint64_t masked = cur_value & pmc->mask;
+	cur_value = __get_umwait_val(pmc->addr, pmc->size);
 
-		/* if the masked value is already matching, abort */
-		if (masked == pmc->val)
-			goto end;
-	}
+	/* check if callback indicates we should abort */
+	if (pmc->fn(cur_value, pmc->opaque) != 0)
+		goto end;
 
 	/* execute UMWAIT */

asm volatile(".byte 0xf2, 0x0f, 0xae, 0xf7;"
--
2.25.1

 doc/guides/rel_notes/release_21_08.rst        |  2 ++
 drivers/event/dlb2/dlb2.c                     | 17 ++++++++--
 drivers/net/i40e/i40e_rxtx.c                  | 20 +++++++----
 drivers/net/iavf/iavf_rxtx.c                  | 20 +++++++----
 drivers/net/ice/ice_rxtx.c                    | 20 +++++++----
 drivers/net/ixgbe/ixgbe_rxtx.c                | 20 +++++++----
 drivers/net/mlx5/mlx5_rx.c                    | 17 ++++++++--
 .../include/generic/rte_power_intrinsics.h    | 33 +++++++++++++++----
 lib/eal/x86/rte_power_intrinsics.c            | 17 +++++-----
 9 files changed, 122 insertions(+), 44 deletions(-)

08/07/2021 16:13, Anatoly Burakov:

Quoted 10 lines

  doc/guides/rel_notes/release_21_08.rst        |  2 ++
  drivers/event/dlb2/dlb2.c                     | 17 ++++++++--
  drivers/net/i40e/i40e_rxtx.c                  | 20 +++++++----
  drivers/net/iavf/iavf_rxtx.c                  | 20 +++++++----
  drivers/net/ice/ice_rxtx.c                    | 20 +++++++----
  drivers/net/ixgbe/ixgbe_rxtx.c                | 20 +++++++----
  drivers/net/mlx5/mlx5_rx.c                    | 17 ++++++++--
  .../include/generic/rte_power_intrinsics.h    | 33 +++++++++++++++----
  lib/eal/x86/rte_power_intrinsics.c            | 17 +++++-----
  9 files changed, 122 insertions(+), 44 deletions(-)

About the title, it is introducing a new prefix "power_intrinsics:" with is not so much descriptive.
Probably better to formulate with "eal:" prefix.

Quoted 6 lines

drivers/net/mlx5/mlx5_rx.c +0 -0

@@ -269,6 +269,18 @@ mlx5_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id)
  	return rx_queue_count(rxq);
  }
 

drivers/net/mlx5/mlx5_rx.c mlx5_rx_queue_count(struct rte_eth_dev *dev, uint16_t rx_queue_id) · line 276

Quoted 5 lines

drivers/net/mlx5/mlx5_rx.c +5 -0

+#define CLB_VAL_IDX 0
+#define CLB_MSK_IDX 1
+static int
+mlx_monitor_callback(const uint64_t value,
+		const uint64_t opaque[RTE_POWER_MONITOR_OPAQUE_SZ])

Everything is prefixed with mlx5, let's be consistent. Please replace mlx_ with mlx5_

doc/guides/nics/features.rst +0 -0

index 403c2b03a3..a96e12d155 100644
@@ -912,6 +912,16 @@ Supports to get Rx/Tx packet burst mode information.
 * **[implements] eth_dev_ops**: ``rx_burst_mode_get``, ``tx_burst_mode_get``.
 * **[related] API**: ``rte_eth_rx_burst_mode_get()``, ``rte_eth_tx_burst_mode_get()``.
 

doc/guides/nics/features.rst +0 -0

 .. _nic_features_other:
 
 Other dev ops not represented by a Feature

Quoted 8 lines · Anatoly Burakov

doc/guides/nics/features.rst +0 -0

index 403c2b03a3..a96e12d155 100644
@@ -912,6 +912,16 @@ Supports to get Rx/Tx packet burst mode information.
  * **[implements] eth_dev_ops**: ``rx_burst_mode_get``, ``tx_burst_mode_get``.
  * **[related] API**: ``rte_eth_rx_burst_mode_get()``, ``rte_eth_tx_burst_mode_get()``.
 

doc/guides/nics/features.rst Supports to get Rx/Tx packet burst mode information. · line 924

Quoted 10 lines

doc/guides/nics/features.rst +10 -0

+.. _nic_features_get_monitor_addr:
+
+PMD power management using monitor addresses
+--------------------------------------------
+
+Supports getting a monitoring condition to use together with Ethernet PMD power
+management (see :doc:`../prog_guide/power_man` for more details).
+
+* **[implements] eth_dev_ops**: ``get_monitor_addr``
+

- This new ethdev feature deserves its own commit.

- Adding ethdev maintainers.

We are missing a doc/guides/nics/features/default.ini. The name of the features proposed here is rather long. As far as I can see, features should be shorter than:
doc/guides/conf.py:feature_str_len = 30
I am not really inspired.. "Power mgmt address monitor"?

- pmd supporting this feature must have their .ini updated.

Quoted 3 lines

doc/guides/nics/features.rst +0 -0

  .. _nic_features_other:
 
  Other dev ops not represented by a Feature

You'd have to walk me through/translate whatever it is that you just said as i have no idea what any of that means :D

Quoted 2 lines · Anatoly

You'd have to walk me through/translate whatever it is that you just said as i have no idea what any of that means :D

https://git.dpdk.org/dpdk/commit/?id=fa5dbd825a

Currently, l3fwd-power enforces the limitation of having one queue per lcore. This is no longer necessary, so remove the limitation.