-----Original Message-----
From: Burakov, Anatoly <redacted>
Sent: Tuesday, January 12, 2021 5:37 PM
To: dev@dpdk.org
Cc: Ma, Liang J <redacted>; Hunt, David <redacted>; Ray Kinsella <redacted>; Neil Horman
[off-list ref]; thomas@monjalon.net; Ananyev, Konstantin [off-list ref]; McDaniel, Timothy
[off-list ref]; Richardson, Bruce [off-list ref]; Macnamara, Chris [off-list ref]
Subject: [PATCH v16 07/11] power: add PMD power management API and callback

From: Liang Ma <redacted>

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 <redacted>
Signed-off-by: Anatoly Burakov <redacted>
---

Notes:
    v15:
    - Fix check in UMWAIT callback

    v13:
    - Rework the synchronization mechanism to not require locking
    - Add more parameter checking
    - Rework n_rx_queues access to not go through internal PMD structures and use
      public API instead

    v13:
    - Rework the synchronization mechanism to not require locking
    - Add more parameter checking
    - Rework n_rx_queues access to not go through internal PMD structures and use
      public API instead

 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  | 359 +++++++++++++++++++++++++
 lib/librte_power/rte_power_pmd_mgmt.h  |  90 +++++++
 lib/librte_power/version.map           |   5 +
 6 files changed, 511 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

...

+
+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)
+{
+
+	struct pmd_queue_cfg *q_conf;
+
+	q_conf = &port_cfg[port_id][qidx];
+
+	if (unlikely(nb_rx == 0)) {
+		q_conf->empty_poll_stats++;
+		if (unlikely(q_conf->empty_poll_stats > EMPTYPOLL_MAX)) {
+			struct rte_power_monitor_cond pmc;
+			uint16_t ret;
+
+			/*
+			 * we might get a cancellation request while being
+			 * inside the callback, in which case the wakeup
+			 * wouldn't work because it would've arrived too early.
+			 *
+			 * to get around this, we notify the other thread that
+			 * we're sleeping, so that it can spin until we're done.
+			 * unsolicited wakeups are perfectly safe.
+			 */
+			q_conf->umwait_in_progress = true;

This write and subsequent read can be reordered by the cpu.
I think you need rte_atomic_thread_fence(__ATOMIC_SEQ_CST) here and
in disable() code-path below.

+
+			/* check if we need to cancel sleep */
+			if (q_conf->pwr_mgmt_state == PMD_MGMT_ENABLED) {
+				/* use monitoring condition to sleep */
+				ret = rte_eth_get_monitor_addr(port_id, qidx,
+						&pmc);
+				if (ret == 0)
+					rte_power_monitor(&pmc, -1ULL);
+			}
+			q_conf->umwait_in_progress = false;
+		}
+	} else
+		q_conf->empty_poll_stats = 0;
+
+	return nb_rx;
+}
+

...

+
+int
+rte_power_pmd_mgmt_queue_disable(unsigned int lcore_id,
+		uint16_t port_id, uint16_t queue_id)
+{
+	struct pmd_queue_cfg *queue_cfg;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
+
+	if (lcore_id >= RTE_MAX_LCORE || queue_id >= RTE_MAX_QUEUES_PER_PORT)
+		return -EINVAL;
+
+	/* no need to check queue id as wrong queue id would not be enabled */
+	queue_cfg = &port_cfg[port_id][queue_id];
+
+	if (queue_cfg->pwr_mgmt_state != PMD_MGMT_ENABLED)
+		return -EINVAL;
+
+	/* let the callback know we're shutting down */
+	queue_cfg->pwr_mgmt_state = PMD_MGMT_BUSY;

Same as above - write to pwr_mgmt_state and read from umwait_in_progress
could be reordered by cpu.
Need to insert rte_atomic_thread_fence(__ATOMIC_SEQ_CST) between them.

BTW, out of curiosity - why do you need this intermediate
state (PMD_MGMT_BUSY) at all?
Why not directly:
queue_cfg->pwr_mgmt_state = PMD_MGMT_DISABLED;
?