Power-optimized RX for Ethernet devices

This patchset proposes a simple API for Ethernet drivers to cause the CPU to enter a power-optimized state while waiting for packets to arrive. There are multiple proposed mechanisms to achieve said power
savings: simple frequency scaling, idle loop, and monitoring the Rx queue for incoming packages. The latter is achieved through cooperation with the NIC driver that will allow us to know address of wake up event, and wait for writes on that address.

On IA, this is achieved through using UMONITOR/UMWAIT instructions. They are used in their raw opcode form because there is no widespread compiler support for them yet. Still, the API is made generic enough to hopefully support other architectures, if they happen to implement similar instructions.

To achieve power savings, there is a very simple mechanism used: we're counting empty polls, and if a certain threshold is reached, we employ one of the suggested power management schemes automatically, from within a Rx callback inside the PMD. Once there's traffic again, the empty poll counter is reset.

This patchset also introduces a few changes into existing power management-related intrinsics, namely to provide a native way of waking up a sleeping core without application being responsible for it, as well as general robustness improvements. There's quite a bit of locking going on, but these locks are per-thread and very little (if any) contention is expected, so the performance impact shouldn't be that bad (and in any case the locking happens when we're about to sleep anyway).

Why are we putting it into ethdev as opposed to leaving this up to the application? Our customers specifically requested a way to do it with minimal changes to the application code. The current approach allows to just flip a switch and automatically have power savings.

Things of note:

- Only 1:1 core to queue mapping is supported, meaning that each lcore must at most handle RX on a single queue

- Support 3 type policies. Monitor/Pause/Frequency Scaling
- Power management is enabled per-queue
- The API doesn't extend to other device types

v16:
- Implemented Konstantin's suggestions and comments
- Added return values to the API

Quoted 34 lines · Anatoly Burakov

This patchset proposes a simple API for Ethernet drivers to cause the CPU to enter a power-optimized state while waiting for packets to arrive. There are multiple proposed mechanisms to achieve said power
savings: simple frequency scaling, idle loop, and monitoring the Rx queue for incoming packages. The latter is achieved through cooperation with the NIC driver that will allow us to know address of wake up event, and wait for writes on that address.

On IA, this is achieved through using UMONITOR/UMWAIT instructions. They are used in their raw opcode form because there is no widespread compiler support for them yet. Still, the API is made generic enough to hopefully support other architectures, if they happen to implement similar instructions.

To achieve power savings, there is a very simple mechanism used: we're counting empty polls, and if a certain threshold is reached, we employ one of the suggested power management schemes automatically, from within a Rx callback inside the PMD. Once there's traffic again, the empty poll counter is reset.

This patchset also introduces a few changes into existing power management-related intrinsics, namely to provide a native way of waking up a sleeping core without application being responsible for it, as well as general robustness improvements. There's quite a bit of locking going on, but these locks are per-thread and very little (if any) contention is expected, so the performance impact shouldn't be that bad (and in any case the locking happens when we're about to sleep anyway).

Why are we putting it into ethdev as opposed to leaving this up to the application? Our customers specifically requested a way to do it with minimal changes to the application code. The current approach allows to just flip a switch and automatically have power savings.

Quoted 4 lines

Things of note:

- Only 1:1 core to queue mapping is supported, meaning that each lcore must at most handle RX on a single queue

If we want to save power, it is likely we would poll more rxqs on a thread.

Quoted 9 lines

- Support 3 type policies. Monitor/Pause/Frequency Scaling
- Power management is enabled per-queue
- The API doesn't extend to other device types

v16:
- Implemented Konstantin's suggestions and comments
- Added return values to the API

- This revision breaks SPDK build (reported by UNH):
http://mails.dpdk.org/archives/test-report/2021-January/174069.html

- Build is broken for ARM and PPC at patch:
86491d5bd4 - (HEAD) eal: add monitor wakeup function (25 minutes ago) <Anatoly Burakov>

Only pasting the ARM failure:
ninja: Entering directory `/home/dmarchan/builds/build-arm64-host-clang' [1/297] Compiling C object 'lib/76b5a35@@rte_eal@sta/librte_eal_arm_rte_power_intrinsics.c.o'.
FAILED: lib/76b5a35@@rte_eal@sta/librte_eal_arm_rte_power_intrinsics.c.o aarch64-linux-gnu-gcc -Ilib/76b5a35@@rte_eal@sta -Ilib

(quoted diff) +0 -19

-I../../dpdk/lib -I. -I../../dpdk/ -Iconfig -I../../dpdk/config
-Ilib/librte_eal/include -I../../dpdk/lib/librte_eal/include
-Ilib/librte_eal/linux/include
-I../../dpdk/lib/librte_eal/linux/include -Ilib/librte_eal/arm/include
-I../../dpdk/lib/librte_eal/arm/include -Ilib/librte_eal/common
-I../../dpdk/lib/librte_eal/common -Ilib/librte_eal
-I../../dpdk/lib/librte_eal -Ilib/librte_kvargs
-I../../dpdk/lib/librte_kvargs
-Ilib/librte_telemetry/../librte_metrics
-I../../dpdk/lib/librte_telemetry/../librte_metrics
-Ilib/librte_telemetry -I../../dpdk/lib/librte_telemetry
-fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall
-Winvalid-pch -Werror -O2 -g -include rte_config.h -Wextra -Wcast-qual
-Wdeprecated -Wformat -Wformat-nonliteral -Wformat-security
-Wmissing-declarations -Wmissing-prototypes -Wnested-externs
-Wold-style-definition -Wpointer-arith -Wsign-compare
-Wstrict-prototypes -Wundef -Wwrite-strings -Wno-packed-not-aligned
-Wno-missing-field-initializers -D_GNU_SOURCE -fPIC -march=armv8-a+crc
-DALLOW_EXPERIMENTAL_API -DALLOW_INTERNAL_API -Wno-format-truncation

'-DABI_VERSION="21.1"' -DRTE_LIBEAL_USE_GETENTROPY -MD -MQ 'lib/76b5a35@@rte_eal@sta/librte_eal_arm_rte_power_intrinsics.c.o' -MF 'lib/76b5a35@@rte_eal@sta/librte_eal_arm_rte_power_intrinsics.c.o.d'

(quoted diff) +0 -2

-o 'lib/76b5a35@@rte_eal@sta/librte_eal_arm_rte_power_intrinsics.c.o'
-c ../../dpdk/lib/librte_eal/arm/rte_power_intrinsics.c

../../dpdk/lib/librte_eal/arm/rte_power_intrinsics.c:35:1: error:
conflicting types for ‘rte_power_monitor_wakeup’
rte_power_monitor_wakeup(const unsigned int lcore_id) ^~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../../dpdk/lib/librte_eal/arm/include/rte_power_intrinsics.h:14,

                  from ../../dpdk/lib/librte_eal/arm/rte_power_intrinsics.c:5:
../../dpdk/lib/librte_eal/include/generic/rte_power_intrinsics.h:79:5:
note: previous declaration of ‘rte_power_monitor_wakeup’ was here
  int rte_power_monitor_wakeup(const unsigned int lcore_id);
      ^~~~~~~~~~~~~~~~~~~~~~~~

ninja: build stopped: subcommand failed.

- The ABI check is still not happy as I reported earlier. Reproduced on v16 (GHA had a hiccup on this revision, but previous ones had the failure too):

1 Changed variable:

[C] 'rte_eth_dev rte_eth_devices[]' was changed at rte_ethdev_core.h:196:1:
type of variable changed:
array element type 'struct rte_eth_dev' changed:
type size hasn't changed
1 data member change:
type of 'const eth_dev_ops* rte_eth_dev::dev_ops' changed:
in pointed to type 'const eth_dev_ops':
in unqualified underlying type 'struct eth_dev_ops' at
rte_ethdev_driver.h:789:1:
type size changed from 6208 to 6272 (in bits)
1 data member insertion:
'eth_get_monitor_addr_t
eth_dev_ops::get_monitor_addr', at offset 6208 (in bits) at rte_ethdev_driver.h:940:1
no data member changes (94 filtered);
type size hasn't changed

Error: ABI issue reported for 'abidiff --suppr /home/dmarchan/dpdk/devtools/../devtools/libabigail.abignore --no-added-syms --headers-dir1 /home/dmarchan/abi/v20.11/build-gcc-static/usr/local/include --headers-dir2 /home/dmarchan/builds/build-gcc-static/install/usr/local/include /home/dmarchan/abi/v20.11/build-gcc-static/dump/librte_ethdev.dump /home/dmarchan/builds/build-gcc-static/install/dump/librte_ethdev.dump'

ABIDIFF_ABI_CHANGE, this change requires a review (abidiff flagged this as a potential issue).

One solution is to add an exception on the eth_dev_ops structure.

+};

14/01/2021 15:46, Anatoly Burakov:

lib/librte_eal/include/generic/rte_power_intrinsics.h line 24

Quoted 4 lines

lib/librte_eal/include/generic/rte_power_intrinsics.h +4 -0

+struct rte_power_monitor_cond {
+	volatile void *addr;  /**< Address to monitor for changes */
+	uint64_t val;         /**< Before attempting the monitoring, the address
+	                       *   may be read and compared against this value.

"may" be read and compared?
Is there a case where there is no read and compare?

lib/librte_eal/include/generic/rte_power_intrinsics.h line 30

Quoted 7 lines

lib/librte_eal/include/generic/rte_power_intrinsics.h +6 -0

+	                       **/
+	uint64_t mask;   /**< 64-bit mask to extract current value from addr */
+	uint8_t data_sz; /**< Data size (in bytes) that will be used to compare
+	                  *   expected value with the memory address. Can be 1,
+	                  *   2, 4, or 8. Supplying any other value will lead to
+	                  *   undefined result. */

Other parameters are not prefixed with "data_", so I think this field could be simply named "size".

Quoted 2 lines

+};

I understand this struct is a direct translation of what existed in 20.11 as function parameters and comments.
If you agree, these comments could be addressed in a separate patch.

Quoted 8 lines

14/01/2021 15:46, Anatoly Burakov:

lib/librte_eal/include/generic/rte_power_intrinsics.h line 24

Quoted 4 lines

lib/librte_eal/include/generic/rte_power_intrinsics.h +4 -0

+struct rte_power_monitor_cond {
+	volatile void *addr;  /**< Address to monitor for changes */
+	uint64_t val;         /**< Before attempting the monitoring, the address
+	                       *   may be read and compared against this value.

"may" be read and compared?
Is there a case where there is no read and compare?

Yes, if the mask is not set.

19/01/2021 11:29, Burakov, Anatoly:

Quoted 10 lines · Thomas Monjalon

Quoted 8 lines

14/01/2021 15:46, Anatoly Burakov:

lib/librte_eal/include/generic/rte_power_intrinsics.h line 24

Quoted 4 lines

lib/librte_eal/include/generic/rte_power_intrinsics.h +4 -0

+struct rte_power_monitor_cond {
+	volatile void *addr;  /**< Address to monitor for changes */
+	uint64_t val;         /**< Before attempting the monitoring, the address
+	                       *   may be read and compared against this value.

"may" be read and compared?
Is there a case where there is no read and compare?

Quoted 1 line

Yes, if the mask is not set.

If the mask is not set, the address is "read" anyway or it is only "watched" for any change?

Sorry the mechanism is really not clear to me.

Quoted 16 lines

19/01/2021 11:29, Burakov, Anatoly:

Quoted 11 lines · Thomas Monjalon

Quoted 8 lines

14/01/2021 15:46, Anatoly Burakov:

lib/librte_eal/include/generic/rte_power_intrinsics.h line 24

Quoted 4 lines

lib/librte_eal/include/generic/rte_power_intrinsics.h +4 -0

+struct rte_power_monitor_cond {
+	volatile void *addr;  /**< Address to monitor for changes */
+	uint64_t val;         /**< Before attempting the monitoring, the address
+	                       *   may be read and compared against this value.

"may" be read and compared?
Is there a case where there is no read and compare?

Yes, if the mask is not set.

If the mask is not set, the address is "read" anyway or it is only "watched" for any change?

Quoted 2 lines

Sorry the mechanism is really not clear to me.

The "value" is only used to avoid the sleep, i.e. to check if the write has already happened. We're waiting on *a write* rather than *a value*, so it's not equivalent to "wait until equal" call. It's more of a "sleep until something happens".

19/01/2021 12:23, Burakov, Anatoly:

Quoted 20 lines · Thomas Monjalon

Quoted 18 lines

19/01/2021 11:29, Burakov, Anatoly:

Quoted 11 lines · Thomas Monjalon

Quoted 8 lines

14/01/2021 15:46, Anatoly Burakov:

lib/librte_eal/include/generic/rte_power_intrinsics.h line 24

Quoted 4 lines

lib/librte_eal/include/generic/rte_power_intrinsics.h +4 -0

+struct rte_power_monitor_cond {
+	volatile void *addr;  /**< Address to monitor for changes */
+	uint64_t val;         /**< Before attempting the monitoring, the address
+	                       *   may be read and compared against this value.

"may" be read and compared?
Is there a case where there is no read and compare?

Yes, if the mask is not set.

If the mask is not set, the address is "read" anyway or it is only "watched" for any change?

Sorry the mechanism is really not clear to me.

Quoted 4 lines

The "value" is only used to avoid the sleep, i.e. to check if the write has already happened. We're waiting on *a write* rather than *a value*, so it's not equivalent to "wait until equal" call. It's more of a "sleep until something happens".

Please make things explicit in doxygen.
The behaviour of each case should be explained crystal clear. Thanks

Quoted 26 lines

19/01/2021 12:23, Burakov, Anatoly:

Quoted 24 lines · Thomas Monjalon

Quoted 11 lines

19/01/2021 11:29, Burakov, Anatoly:

Quoted 10 lines · Thomas Monjalon

Quoted 8 lines

14/01/2021 15:46, Anatoly Burakov:

Review

Quoted 4 lines

(quoted diff) +4 -0

+struct rte_power_monitor_cond {
+	volatile void *addr;  /**< Address to monitor for changes */
+	uint64_t val;         /**< Before attempting the monitoring, the address
+	                       *   may be read and compared against this value.

"may" be read and compared?
Is there a case where there is no read and compare?

Quoted 7 lines

Quoted 1 line

Yes, if the mask is not set.

If the mask is not set, the address is "read" anyway or it is only "watched" for any change?

Sorry the mechanism is really not clear to me.

The "value" is only used to avoid the sleep, i.e. to check if the write has already happened. We're waiting on *a write* rather than *a value*, so it's not equivalent to "wait until equal" call. It's more of a "sleep until something happens".

Quoted 3 lines

Please make things explicit in doxygen.
The behaviour of each case should be explained crystal clear. Thanks

It is explained in the comments to rte_power_monitor() call. But OK, i'll add more clarification for the struct too.

20/01/2021 11:32, Burakov, Anatoly:

Quoted 27 lines · Thomas Monjalon

Quoted 26 lines

19/01/2021 12:23, Burakov, Anatoly:

Quoted 24 lines · Thomas Monjalon

Quoted 11 lines

19/01/2021 11:29, Burakov, Anatoly:

Quoted 10 lines · Thomas Monjalon

Quoted 8 lines

14/01/2021 15:46, Anatoly Burakov:

Review

Quoted 4 lines

(quoted diff) +4 -0

+struct rte_power_monitor_cond {
+	volatile void *addr;  /**< Address to monitor for changes */
+	uint64_t val;         /**< Before attempting the monitoring, the address
+	                       *   may be read and compared against this value.

"may" be read and compared?
Is there a case where there is no read and compare?

Quoted 7 lines

Quoted 1 line

Yes, if the mask is not set.

If the mask is not set, the address is "read" anyway or it is only "watched" for any change?

Sorry the mechanism is really not clear to me.

The "value" is only used to avoid the sleep, i.e. to check if the write has already happened. We're waiting on *a write* rather than *a value*, so it's not equivalent to "wait until equal" call. It's more of a "sleep until something happens".

Quoted 6 lines

Quoted 3 lines

Please make things explicit in doxygen.
The behaviour of each case should be explained crystal clear. Thanks

It is explained in the comments to rte_power_monitor() call. But OK, i'll add more clarification for the struct too.

Please avoid the word "may" in API description.

This is what is explained in rte_power_monitor:
"
* Additionally, an expected 64-bit value and 64-bit mask are provided. If
* mask is non-zero, the current value pointed to by the p pointer will be
* checked against the expected value, and if they match, the entering of
* optimized power state may be aborted.
"

Can we replace "may" by "will"?

Quoted 45 lines

20/01/2021 11:32, Burakov, Anatoly:

Quoted 27 lines · Thomas Monjalon

Quoted 26 lines

19/01/2021 12:23, Burakov, Anatoly:

Quoted 24 lines · Thomas Monjalon

Quoted 11 lines

19/01/2021 11:29, Burakov, Anatoly:

Quoted 10 lines · Thomas Monjalon

Quoted 8 lines

14/01/2021 15:46, Anatoly Burakov:

Review

Quoted 4 lines

(quoted diff) +4 -0

+struct rte_power_monitor_cond {
+	volatile void *addr;  /**< Address to monitor for changes */
+	uint64_t val;         /**< Before attempting the monitoring, the address
+	                       *   may be read and compared against this value.

"may" be read and compared?
Is there a case where there is no read and compare?

Quoted 7 lines

Quoted 1 line

Yes, if the mask is not set.

If the mask is not set, the address is "read" anyway or it is only "watched" for any change?

Sorry the mechanism is really not clear to me.

The "value" is only used to avoid the sleep, i.e. to check if the write has already happened. We're waiting on *a write* rather than *a value*, so it's not equivalent to "wait until equal" call. It's more of a "sleep until something happens".

Quoted 6 lines

Quoted 3 lines

Please make things explicit in doxygen.
The behaviour of each case should be explained crystal clear. Thanks

It is explained in the comments to rte_power_monitor() call. But OK, i'll add more clarification for the struct too.

Please avoid the word "may" in API description.

This is what is explained in rte_power_monitor:
"
* Additionally, an expected 64-bit value and 64-bit mask are provided. If
* mask is non-zero, the current value pointed to by the p pointer will be
* checked against the expected value, and if they match, the entering of
* optimized power state may be aborted.
"

Quoted 2 lines

Can we replace "may" by "will"?

Yep, we can. However, the "may" part was intended to leave some wiggle room for a different implementation, should the need arise, and i find "will" to be needlessly prescriptive. Frankly, i do not see the need for such a detailed description of what the API does under the hood, as long as it's clear what its effects are. The main purpose is waiting for a write. The mask is only used to check whether the expected write has already happened by the time we're calling the API. Whether the CPU then does or does not go to sleep is not really relevant IMO.

20/01/2021 12:05, Burakov, Anatoly:

Quoted 49 lines · Thomas Monjalon

Quoted 45 lines

20/01/2021 11:32, Burakov, Anatoly:

Quoted 27 lines · Thomas Monjalon

Quoted 26 lines

19/01/2021 12:23, Burakov, Anatoly:

Quoted 24 lines · Thomas Monjalon

Quoted 11 lines

19/01/2021 11:29, Burakov, Anatoly:

Quoted 10 lines · Thomas Monjalon

Quoted 8 lines

14/01/2021 15:46, Anatoly Burakov:

Review

Quoted 4 lines

(quoted diff) +4 -0

+struct rte_power_monitor_cond {
+	volatile void *addr;  /**< Address to monitor for changes */
+	uint64_t val;         /**< Before attempting the monitoring, the address
+	                       *   may be read and compared against this value.

"may" be read and compared?
Is there a case where there is no read and compare?

Quoted 7 lines

Quoted 1 line

Yes, if the mask is not set.

If the mask is not set, the address is "read" anyway or it is only "watched" for any change?

Sorry the mechanism is really not clear to me.

The "value" is only used to avoid the sleep, i.e. to check if the write has already happened. We're waiting on *a write* rather than *a value*, so it's not equivalent to "wait until equal" call. It's more of a "sleep until something happens".

Quoted 6 lines

Quoted 3 lines

Please make things explicit in doxygen.
The behaviour of each case should be explained crystal clear. Thanks

It is explained in the comments to rte_power_monitor() call. But OK, i'll add more clarification for the struct too.

Please avoid the word "may" in API description.

This is what is explained in rte_power_monitor:
"
* Additionally, an expected 64-bit value and 64-bit mask are provided. If
* mask is non-zero, the current value pointed to by the p pointer will be
* checked against the expected value, and if they match, the entering of
* optimized power state may be aborted.
"

Quoted 2 lines

Can we replace "may" by "will"?

Quoted 8 lines

Yep, we can. However, the "may" part was intended to leave some wiggle room for a different implementation, should the need arise, and i find "will" to be needlessly prescriptive. Frankly, i do not see the need for such a detailed description of what the API does under the hood, as long as it's clear what its effects are. The main purpose is waiting for a write. The mask is only used to check whether the expected write has already happened by the time we're calling the API. Whether the CPU then does or does not go to sleep is not really relevant IMO.

I think it is relevant but I may be wrong.
Any other opinions?

From: Liang Ma <[email protected]>

Add a simple on/off switch that will enable saving power when no packets are arriving. It is based on counting the number of empty polls and, when the number reaches a certain threshold, entering an architecture-defined optimized power state that will either wait until a TSC timestamp expires, or when packets arrive.

This API mandates a core-to-single-queue mapping (that is, multiple queued per device are supported, but they have to be polled on different cores).

This design is using PMD RX callbacks.

1. UMWAIT/UMONITOR:

When a certain threshold of empty polls is reached, the core will go into a power optimized sleep while waiting on an address of next RX descriptor to be written to.

2. TPAUSE/Pause instruction

This method uses the pause (or TPAUSE, if available) instruction to avoid busy polling.

3. Frequency scaling
Reuse existing DPDK power library to scale up/down core frequency depending on traffic volume.

Signed-off-by Liang Ma <[email protected]>

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

---

Notes:
v17:
- Added memory barriers suggested by Konstantin
- Removed the BUSY state

  doc/guides/prog_guide/power_man.rst    |  44 +++
  doc/guides/rel_notes/release_21_02.rst |  10 +
  lib/librte_power/meson.build           |   5 +-
  lib/librte_power/rte_power_pmd_mgmt.c  | 364 +++++++++++++++++++++++++
  lib/librte_power/rte_power_pmd_mgmt.h  |  90 ++++++
  lib/librte_power/version.map           |   5 +
  6 files changed, 516 insertions(+), 2 deletions(-)

create mode 100644 lib/librte_power/rte_power_pmd_mgmt.c create mode 100644 lib/librte_power/rte_power_pmd_mgmt.h

The data_sz name is fine, but it looks out of place because nothing else has "data" prefix in that structure. Rename it to "size", as well as add more clarity to the comments around each struct member.

Fixes 6a17919b0e2a ("eal: change power intrinsics API")

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

Currently, the API documentation is ambiguous as to what happens when certain conditions are met. Document the behavior explicitly, as well as fix some typos and outdated comments.

Fixes 6a17919b0e2a ("eal: change power intrinsics API")

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

---
* Monitor specific address for changes. This will cause the CPU to enter an
* architecture-defined optimized power state until either the specified
* memory address is written to, a certain TSC timestamp is reached, or other
* reasons cause the CPU to wake up.
*
- * Additionally, an expected 64-bit value and 64-bit mask are provided. If
- * mask is non-zero, the current value pointed to by the p pointer will be
- * checked against the expected value, and if they match, the entering of

- * optimized power state may be aborted.
+ * Additionally, an expected value (pmc->val), mask (pmc->mask), and data
+ * size (pmc->size) are provided in the pmc power monitoring condition. If
+ * the mask is non-zero, the current value pointed to by the pmc->addr pointer
+ * will be read and compared against the expected value, and if they match, the
+ * entering of optimized power state will be aborted. This is intended to
+ * prevent the CPU from entering optimized power state and waiting on a write
+ * that has already happened by the time this API is called.

From: Liang Ma <[email protected]>

Add PMD power management feature support to l3fwd-power sample app.

Signed-off-by Liang Ma <[email protected]>

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

Acked-by David Hunt <[email protected]>

---

+PMD Power Management Mode
+-------------------------

Things of note:

- Only 1:1 core to queue mapping is supported, meaning that each lcore must at most handle RX on a single queue

- Support 3 type policies. Monitor/Pause/Frequency Scaling
- Power management is enabled per-queue
- The API doesn't extend to other device types

Anatoly Burakov (2):
eal: rename power monitor condition member
eal: improve comments around power monitoring API

Liang Ma (2):
power: add PMD power management API and callback examples/l3fwd-power: enable PMD power mgmt

22/01/2021 18:12, Anatoly Burakov:

Quoted 4 lines

Things of note:

- Only 1:1 core to queue mapping is supported, meaning that each lcore must at most handle RX on a single queue

Is there a way to have a more generic solution?
I think it may deserve some comments in the API.

Quoted 4 lines

- Support 3 type policies. Monitor/Pause/Frequency Scaling
- Power management is enabled per-queue
- The API doesn't extend to other device types

Could it be extended to more device types?
Otherwise it should be called specifically ethdev power management.

Quoted 5 lines

Anatoly Burakov (2):
eal: rename power monitor condition member
eal: improve comments around power monitoring API

Quoted 3 lines

Liang Ma (2):
power: add PMD power management API and callback examples/l3fwd-power: enable PMD power mgmt

Applied with few formatting improvements, thanks

doc/guides/rel_notes/release_21_02.rst +8 -0

+
+  A new helper API has been added to make using Ethernet PMD power management
+  easier for the user: ``rte_power_pmd_mgmt_queue_enable()``. Three power
+  management schemes are supported initially:
+
+  * Power saving based on UMWAIT instruction (x86 only)
+  * Power saving based on ``rte_pause()`` (generic) or TPAUSE instruction (x86 only)
+  * Power saving based on frequency scaling through the ``librte_power`` library

lib/librte_power/rte_power_pmd_mgmt.c new file +10 -0

+ */
+
+#include <rte_lcore.h>
+#include <rte_cycles.h>
+#include <rte_cpuflags.h>
+#include <rte_malloc.h>
+#include <rte_ethdev.h>
+#include <rte_power_intrinsics.h>
+
+#include "rte_power_pmd_mgmt.h"

lib/librte_power/rte_power_pmd_mgmt.h new file +1 -0

@@ -0,0 +1,90 @@
+/* SPDX-License-Identifier: BSD-3-Clause

lib/librte_power/rte_power_pmd_mgmt.h new file +18 -0

+#include <stdint.h>
+#include <stdbool.h>
+
+#include <rte_common.h>
+#include <rte_byteorder.h>
+#include <rte_log.h>
+#include <rte_power.h>
+#include <rte_atomic.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * PMD Power Management Type
+ */
+enum rte_power_pmd_mgmt_type {
+	/** Use power-optimized monitoring to wait for incoming traffic */

lib/librte_power/rte_power_pmd_mgmt.h new file +6 -0

+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ * @param queue_id
+ *   The queue identifier of the Ethernet device.
+ * @param mode
+ *   The power management callback function type.

14/01/2021 15:46, Anatoly Burakov:

doc/guides/prog_guide/power_man.rst User Cases · line 228

Quoted 5 lines

From: Liang Ma <[email protected]>

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

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

This is quite limited.
Not sure librte_power is the right place for a flexible ethdev management.

lib/librte_power/rte_power_pmd_mgmt.c line 22

Quoted 7 lines

lib/librte_power/rte_power_pmd_mgmt.c new file +7 -0

+static struct pmd_conf_data {
+	struct rte_cpu_intrinsics intrinsics_support;
+	/**< what do we support? */
+	uint64_t tsc_per_us;
+	/**< pre-calculated tsc diff for 1us */
+	uint64_t pause_per_us;
+	/**< how many rte_pause can we fit in a microisecond? */

Vim typo spotted: microisecond

Quoted 1 line

+} global_data;

Not sure about the need for a struct.
Please insert comment before the field if not on the same line. BTW, why doxygen syntax in a .c file?

From: Liang Ma <[email protected]>

Add PMD power management feature support to l3fwd-power sample app.

Signed-off-by Liang Ma <[email protected]>

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

---

Notes:
v12:
- Allow selecting PMD power management scheme from command-line
- Enforce 1 core 1 queue rule

This patchset proposes a simple API for Ethernet drivers to cause the CPU to enter a power-optimized state while waiting for packets to arrive. There are multiple proposed mechanisms to achieve said power
savings: simple frequency scaling, idle loop, and monitoring the Rx queue for incoming packages. The latter is achieved through cooperation with the NIC driver that will allow us to know address of wake up event, and wait for writes on that address.

On IA, this is achieved through using UMONITOR/UMWAIT instructions. They are used in their raw opcode form because there is no widespread compiler support for them yet. Still, the API is made generic enough to hopefully support other architectures, if they happen to implement similar instructions.

To achieve power savings, there is a very simple mechanism used: we're counting empty polls, and if a certain threshold is reached, we employ one of the suggested power management schemes automatically, from within a Rx callback inside the PMD. Once there's traffic again, the empty poll counter is reset.

This patchset also introduces a few changes into existing power management-related intrinsics, namely to provide a native way of waking up a sleeping core without application being responsible for it, as well as general robustness improvements. There's quite a bit of locking going on, but these locks are per-thread and very little (if any) contention is expected, so the performance impact shouldn't be that bad (and in any case the locking happens when we're about to sleep anyway).

Why are we putting it into ethdev as opposed to leaving this up to the application? Our customers specifically requested a way to do it with minimal changes to the application code. The current approach allows to just flip a switch and automatically have power savings.

Things of note:

- Only 1:1 core to queue mapping is supported, meaning that each lcore must at most handle RX on a single queue
- Support 3 type policies. Monitor/Pause/Frequency Scaling
- Power management is enabled per-queue
- The API doesn't extend to other device types

v17:
- Added exception for ethdev driver-only ABI
- Added memory barriers for monitor/wakeup (Konstantin)
- Fixed compiled issues on non-x86 platforms (hopefully!)

Quoted 42 lines · Anatoly Burakov

This patchset proposes a simple API for Ethernet drivers to cause the CPU to enter a power-optimized state while waiting for packets to arrive. There are multiple proposed mechanisms to achieve said power
savings: simple frequency scaling, idle loop, and monitoring the Rx queue for incoming packages. The latter is achieved through cooperation with the NIC driver that will allow us to know address of wake up event, and wait for writes on that address.

On IA, this is achieved through using UMONITOR/UMWAIT instructions. They are used in their raw opcode form because there is no widespread compiler support for them yet. Still, the API is made generic enough to hopefully support other architectures, if they happen to implement similar instructions.

To achieve power savings, there is a very simple mechanism used: we're counting empty polls, and if a certain threshold is reached, we employ one of the suggested power management schemes automatically, from within a Rx callback inside the PMD. Once there's traffic again, the empty poll counter is reset.

This patchset also introduces a few changes into existing power management-related intrinsics, namely to provide a native way of waking up a sleeping core without application being responsible for it, as well as general robustness improvements. There's quite a bit of locking going on, but these locks are per-thread and very little (if any) contention is expected, so the performance impact shouldn't be that bad (and in any case the locking happens when we're about to sleep anyway).

Why are we putting it into ethdev as opposed to leaving this up to the application? Our customers specifically requested a way to do it with minimal changes to the application code. The current approach allows to just flip a switch and automatically have power savings.

Things of note:

- Only 1:1 core to queue mapping is supported, meaning that each lcore must at most handle RX on a single queue
- Support 3 type policies. Monitor/Pause/Frequency Scaling
- Power management is enabled per-queue
- The API doesn't extend to other device types

Quoted 4 lines

v17:
- Added exception for ethdev driver-only ABI
- Added memory barriers for monitor/wakeup (Konstantin)
- Fixed compiled issues on non-x86 platforms (hopefully!)

SPDK build is still broken.
http://mails.dpdk.org/archives/test-report/2021-January/174840.html

==== 20 line log output for Ubuntu 18.04 (dpdk_compile_spdk): ==== rte_power_pmd_mgmt.c:(.text.experimental+0x1cc): undefined reference to rte_eth_add_rx_callback' rte_power_pmd_mgmt.c:(.text.experimental+0x1f8): undefined reference to rte_eth_get_monitor_addr' rte_power_pmd_mgmt.c:(.text.experimental+0x37f): undefined reference to `rte_eth_dev_logtype' /dpdk/build/lib/librte_power.a(librte_power_rte_power_pmd_mgmt.c.o):
In function `rte_power_pmd_mgmt_queue_disable':
rte_power_pmd_mgmt.c:(.text.experimental+0x42a): undefined reference to rte_eth_dev_is_valid_port' rte_power_pmd_mgmt.c:(.text.experimental+0x4e7): undefined reference to rte_eth_remove_rx_callback' rte_power_pmd_mgmt.c:(.text.experimental+0x536): undefined reference to rte_eth_remove_rx_callback' rte_power_pmd_mgmt.c:(.text.experimental+0x54d): undefined reference to rte_eth_dev_logtype'
collect2: error: ld returned 1 exit status /spdk/mk/spdk.app.mk:65: recipe for target 'iscsi_fuzz' failed /spdk/mk/spdk.subdirs.mk:44: recipe for target 'iscsi_fuzz' failed /spdk/mk/spdk.subdirs.mk:44: recipe for target 'fuzz' failed make[4]: *** [iscsi_fuzz] Error 1
make[3]: *** [iscsi_fuzz] Error 2
make[2]: *** [fuzz] Error 2
/spdk/mk/spdk.subdirs.mk:44: recipe for target 'app' failed make[1]: *** [app] Error 2
/spdk/mk/spdk.subdirs.mk:44: recipe for target 'test' failed
make: *** [test] Error 2
[2] Error running command.

I guess this is because of the added dependency of rte_ethdev to rte_power. Afaics, SPDK does not use pkg-config:
https://github.com/spdk/spdk/blob/master/lib/env_dpdk/env.mk#L53

Quoted 10 lines

Quoted 8 lines · Anatoly Burakov

This patchset proposes a simple API for Ethernet drivers to cause the CPU to enter a power-optimized state while waiting for packets to arrive. There are multiple proposed mechanisms to achieve said power
savings: simple frequency scaling, idle loop, and monitoring the Rx queue for incoming packages. The latter is achieved through cooperation with the NIC driver that will allow us to know address of wake up event, and wait for writes on that address.

Quoted 4 lines

Quoted 4 lines

Why are we putting it into ethdev as opposed to leaving this up to the application? Our customers specifically requested a way to do it with minimal changes to the application code. The current approach allows to just flip a switch and automatically have power savings.

Quoted 2 lines

SPDK build is still broken.
http://mails.dpdk.org/archives/test-report/2021-January/174840.html

Quoted 3 lines

I guess this is because of the added dependency of rte_ethdev to rte_power. Afaics, SPDK does not use pkg-config:
https://github.com/spdk/spdk/blob/master/lib/env_dpdk/env.mk#L53

Sooo... this is an SPDK issue then? Because i can't see any way of fixing the issue on DPDK side.

18/01/2021 16:45, Burakov, Anatoly:

Quoted 11 lines · David Marchand

Quoted 10 lines

Quoted 8 lines · Anatoly Burakov

This patchset proposes a simple API for Ethernet drivers to cause the CPU to enter a power-optimized state while waiting for packets to arrive. There are multiple proposed mechanisms to achieve said power
savings: simple frequency scaling, idle loop, and monitoring the Rx queue for incoming packages. The latter is achieved through cooperation with the NIC driver that will allow us to know address of wake up event, and wait for writes on that address.

[...]

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Why are we putting it into ethdev as opposed to leaving this up to the application? Our customers specifically requested a way to do it with minimal changes to the application code. The current approach allows to just flip a switch and automatically have power savings.

The customer laziness is usually a bad justification :) I think we could achieve the same with not too much code on application side.

And I'm not sure hiding queue management is sane. Remember this rule: application must remain in control.

[...]

Quoted 2 lines

Quoted 2 lines

SPDK build is still broken.
http://mails.dpdk.org/archives/test-report/2021-January/174840.html

[...]

Quoted 4 lines

Quoted 3 lines

I guess this is because of the added dependency of rte_ethdev to rte_power. Afaics, SPDK does not use pkg-config:
https://github.com/spdk/spdk/blob/master/lib/env_dpdk/env.mk#L53

Quoted 2 lines

Sooo... this is an SPDK issue then? Because i can't see any way of fixing the issue on DPDK side.

Yes SPDK should not skip pkg-config.
But it raises 2 question:
- are we breaking ABI compatibility?

- is ethdev management expected for librte_power?

It makes me wonder whether we should host the few functions mixing librte_ethdev and librte_power somewhere else.
The question is where?

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

SPDK build is still broken.
http://mails.dpdk.org/archives/test-report/2021-January/174840.html

[...]

Quoted 4 lines

Quoted 3 lines

I guess this is because of the added dependency of rte_ethdev to rte_power. Afaics, SPDK does not use pkg-config:
https://github.com/spdk/spdk/blob/master/lib/env_dpdk/env.mk#L53

Quoted 6 lines

Quoted 2 lines

Sooo... this is an SPDK issue then? Because i can't see any way of fixing the issue on DPDK side.

Yes SPDK should not skip pkg-config.
But it raises 2 question:
- are we breaking ABI compatibility?

Good question. Does including an extra intra-DPDK dependency count as ABI break? I was under impression that we didn't want DPDK to be distributed as individual libraries but rather would like it to be used as a whole, so if internal dependencies between components change, it's not a big deal (unless a third-party build system is used that explicitly specifies dependencies rather than using pkg-config).

Anatoly Burakov (5):
eal: uninline power intrinsics
eal: avoid invalid API usage in power intrinsics
eal: change API of power intrinsics
eal: remove sync version of power monitor
eal: add monitor wakeup function

Liang Ma (6):
ethdev: add simple power management API
power: add PMD power management API and callback net/ixgbe: implement power management API net/i40e: implement power management API net/ice: implement power management API examples/l3fwd-power: enable PMD power mgmt

14/01/2021 15:46, Anatoly Burakov:

Quoted 7 lines

Anatoly Burakov (5):
eal: uninline power intrinsics
eal: avoid invalid API usage in power intrinsics
eal: change API of power intrinsics
eal: remove sync version of power monitor
eal: add monitor wakeup function

Quoted 7 lines

Liang Ma (6):
ethdev: add simple power management API
power: add PMD power management API and callback net/ixgbe: implement power management API net/i40e: implement power management API net/ice: implement power management API examples/l3fwd-power: enable PMD power mgmt

The librte_power part may deserve another iteration. I suggest to give it a chance for a better version in -rc2.

For 21.02-rc1, the EAL and ethdev (including PMDs) parts are merged, thanks.

Add a simple on/off switch that will enable saving power when no packets are arriving. It is based on counting the number of empty polls and, when the number reaches a certain threshold, entering an architecture-defined optimized power state that will either wait until a TSC timestamp expires, or when packets arrive.

This API is limited to 1 core 1 queue use case as there is no coordination between queues/cores in ethdev.

The TSC timestamp is automatically calculated using current link speed and RX descriptor ring size, such that the sleep time is not longer than it would take for a NIC to fill its entire RX descriptor ring.

Signed-off-by Liang J. Ma <[email protected]>

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

---
 lib/librte_ethdev/rte_ethdev.c           | 39 +++++++++++++
 lib/librte_ethdev/rte_ethdev.h           | 70 ++++++++++++++++++++++++
 lib/librte_ethdev/rte_ethdev_core.h      | 41 +++++++++++++-
 lib/librte_ethdev/rte_ethdev_version.map |  4 ++
 4 files changed, 152 insertions(+), 2 deletions(-)

lib/librte_ethdev/rte_ethdev.c +39 -0

index 8e10a6fc36..0be5ecfc11 100644
@@ -16,6 +16,7 @@
 #include <netinet/in.h>
 
 #include <rte_byteorder.h>
+#include <rte_cpuflags.h>
 #include <rte_log.h>
 #include <rte_debug.h>
 #include <rte_interrupts.h>

@@ -5053,6 +5054,44 @@ rte_eth_dev_pool_ops_supported(uint16_t port_id, const char *pool)
 	return (*dev->dev_ops->pool_ops_supported)(dev, pool);
 }
 
+int
+rte_eth_dev_power_mgmt_enable(uint16_t port_id)
+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	dev = &rte_eth_devices[port_id];
+
+	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_WAITPKG))
+		return -ENOTSUP;
+
+	/* allocate memory for empty poll stats */
+	dev->empty_poll_stats = rte_malloc_socket(NULL,
+		sizeof(struct rte_eth_ep_stat) * RTE_MAX_QUEUES_PER_PORT,
+		0, dev->data->numa_node);
+
+	if (dev->empty_poll_stats == NULL)
+		return -ENOMEM;
+
+	dev->pwr_mgmt_state = RTE_ETH_DEV_POWER_MGMT_ENABLED;
+	return 0;
+}
+
+int
+rte_eth_dev_power_mgmt_disable(uint16_t port_id)
+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+	dev = &rte_eth_devices[port_id];
+
+	/* rte_free ignores NULL so safe to call without checks */
+	rte_free(dev->empty_poll_stats);
+
+	dev->pwr_mgmt_state = RTE_ETH_DEV_POWER_MGMT_DISABLED;
+	return 0;
+}
+
 /**
  * A set of values to describe the possible states of a switch domain.
  */

lib/librte_ethdev/rte_ethdev.h +51 -0

index a49242bcd2..b8318f7e91 100644
@@ -157,6 +157,7 @@ extern "C" {
 #include <rte_common.h>
 #include <rte_config.h>
 #include <rte_ether.h>
+#include <rte_power_intrinsics.h>
 
 #include "rte_ethdev_trace_fp.h"
 #include "rte_dev_info.h"

@@ -666,6 +667,7 @@ rte_eth_rss_hf_refine(uint64_t rss_hf)
 /** Maximum nb. of vlan per mirror rule */
 #define ETH_MIRROR_MAX_VLANS       64
 
+#define ETH_EMPTYPOLL_MAX          512 /**< Empty poll number threshlold */
 #define ETH_MIRROR_VIRTUAL_POOL_UP     0x01  /**< Virtual Pool uplink Mirroring. */
 #define ETH_MIRROR_UPLINK_PORT         0x02  /**< Uplink Port Mirroring. */
 #define ETH_MIRROR_DOWNLINK_PORT       0x04  /**< Downlink Port Mirroring. */

@@ -1490,6 +1492,16 @@ enum rte_eth_dev_state {
 	RTE_ETH_DEV_REMOVED,
 };
 
+/**
+ * Possible power managment states of an ethdev port.
+ */
+enum rte_eth_dev_power_mgmt_state {
+	/** Device power management is disabled. */
+	RTE_ETH_DEV_POWER_MGMT_DISABLED = 0,
+	/** Device power management is enabled. */
+	RTE_ETH_DEV_POWER_MGMT_ENABLED
+};
+
 struct rte_eth_dev_sriov {
 	uint8_t active;               /**< SRIOV is active with 16, 32 or 64 pools */
 	uint8_t nb_q_per_pool;        /**< rx queue number per pool */

@@ -4302,6 +4314,38 @@ __rte_experimental
 int rte_eth_dev_hairpin_capability_get(uint16_t port_id,
 				       struct rte_eth_hairpin_cap *cap);
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Enable device power management.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ *
+ * @return
+ *   0 on success
+ *   <0 on error
+ */
+__rte_experimental
+int rte_eth_dev_power_mgmt_enable(uint16_t port_id);
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice
+ *
+ * Disable device power management.
+ *
+ * @param port_id
+ *   The port identifier of the Ethernet device.
+ *
+ * @return
+ *   0 on success
+ *   <0 on error
+ */
+__rte_experimental
+int rte_eth_dev_power_mgmt_disable(uint16_t port_id);
+
 #include <rte_ethdev_core.h>
 
 /**

@@ -4417,6 +4461,32 @@ rte_eth_rx_burst(uint16_t port_id, uint16_t queue_id,
 		} while (cb != NULL);
 	}
 #endif
+	if (dev->pwr_mgmt_state == RTE_ETH_DEV_POWER_MGMT_ENABLED) {
+		if (unlikely(nb_rx == 0)) {
+			dev->empty_poll_stats[queue_id].num++;
+			if (unlikely(dev->empty_poll_stats[queue_id].num >
+					ETH_EMPTYPOLL_MAX)) {
+				volatile void *target_addr;
+				uint64_t expected, mask;