14/01/2021 15:46, Anatoly Burakov:
> From: Liang Ma <liang.j...@intel.com>
>
> + 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.
> +
> References
> ----------
>
> @@ -200,3 +241,6 @@ References
>
> * The :doc:`../sample_app_ug/vm_power_management`
> chapter in the :doc:`../sample_app_ug/index` section.
> +
> +* The :doc:`../sample_app_ug/rxtx_callbacks`
> + chapter in the :doc:`../sample_app_ug/index` section.
Why the index page is mentionned here?
> --- a/doc/guides/rel_notes/release_21_02.rst
> +++ b/doc/guides/rel_notes/release_21_02.rst
> +* **Add PMD power management helper API**
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
[...]
> --- a/lib/librte_power/meson.build
> +++ b/lib/librte_power/meson.build
> -deps += ['timer']
> +deps += ['timer' ,'ethdev']
Wrapping ethdev looks very strange to me.
> --- /dev/null
> +++ b/lib/librte_power/rte_power_pmd_mgmt.c
> @@ -0,0 +1,364 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2020 Intel Corporation
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?
> --- /dev/null
> +++ b/lib/librte_power/rte_power_pmd_mgmt.h
> @@ -0,0 +1,90 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2020 Intel Corporation
> + */
> +
> +#ifndef _RTE_POWER_PMD_MGMT_H
> +#define _RTE_POWER_PMD_MGMT_H
> +
> +/**
> + * @file
> + * RTE PMD Power Management
> + */
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.
[...]
> --- a/lib/librte_power/version.map
> +++ b/lib/librte_power/version.map
> + # added in 21.02
> + rte_power_pmd_mgmt_queue_enable;
> + rte_power_pmd_mgmt_queue_disable;
Alpha sort please.
