Re: [dpdk-dev] [PATCH v17 07/11] power: add PMD power management API and callback

[Thomas Monjalon <thomas@monjalon.net>]

14/01/2021 15:46, Anatoly Burakov:
> From: Liang Ma <liang.j.ma@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.