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

From: Liang Ma <redacted>

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

+
+API Overview for PMD Power Management
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Underlining should be shorter please.

+* **Queue Enable**: Enable specific power scheme for certain queue/port/core
+
+* **Queue Disable**: Disable power scheme for certain queue/port/core

Please terminate sentences with a dot.

Why the index page is mentionned here?

Please follow release notes guidelines (past tense and dot).

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

[...]

Wrapping ethdev looks very strange to me.

2010?

+ */
+
+#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"
+
+#define EMPTYPOLL_MAX  512
+
+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

+} 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?

blank line here?

+#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 */
+	RTE_POWER_MGMT_TYPE_MONITOR = 1,
+	/** Use power-optimized sleep to avoid busy polling */
+	RTE_POWER_MGMT_TYPE_PAUSE,
+	/** Use frequency scaling when traffic is low */
+	RTE_POWER_MGMT_TYPE_SCALE,
+};
+
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change, or be removed, without prior notice

Dot at the end please.

+ *
+ * Enable power management on a specified RX queue and lcore.
+ *
+ * @note This function is not thread-safe.
+ *
+ * @param lcore_id
+ *   lcore_id.

Interesting comment :)

+ * @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.
+
+ * @return
+ *   0 on success
+ *   <0 on error
+ */
+__rte_experimental
+int
+rte_power_pmd_mgmt_queue_enable(unsigned int lcore_id,
+		uint16_t port_id, uint16_t queue_id,
+		enum rte_power_pmd_mgmt_type mode);

In reality it is an API for ethdev Rx queue, not general PMD.
The function should be renamed accordingly.

[...]

Alpha sort please.