From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
by inbox.dpdk.org (Postfix) with ESMTP id 9BBBCA0A03;
Mon, 18 Jan 2021 23:48:12 +0100 (CET)
Received: from [217.70.189.124] (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id 584D1140D2B;
Mon, 18 Jan 2021 23:48:12 +0100 (CET)
Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com
[66.111.4.29]) by mails.dpdk.org (Postfix) with ESMTP id 76863140D28
for <dev@dpdk.org>; Mon, 18 Jan 2021 23:48:10 +0100 (CET)
Received: from compute2.internal (compute2.nyi.internal [10.202.2.42])
by mailout.nyi.internal (Postfix) with ESMTP id 941105C01C7;
Mon, 18 Jan 2021 17:48:09 -0500 (EST)
Received: from mailfrontend2 ([10.202.2.163])
by compute2.internal (MEProxy); Mon, 18 Jan 2021 17:48:09 -0500
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h=
from:to:cc:subject:date:message-id:in-reply-to:references
:mime-version:content-transfer-encoding:content-type; s=fm3; bh=
6OZCvLY32skuGKSx2DOvC8m2tvcDq3mcVLCvz4pBREU=; b=DTJEaUacMAZN6f+y
w90MBVm2YiZXeHmioz+BFV7peND11afeVS19Ssf2CMhDMgdOlKIFiHxhkc+5C/yY
TCVnL3JEaRQJkYHRVDgeFqnpw+61SfcLV6D28m0PQyIGPTM9gdcGBvOpdFObivIf
iPhlEW/sW/7KhswKlil1V6vGfQMGwQBTaVZLEd3ZcIDESo+WLg9h8e4i+zzZ8HKi
dBDbwUoehSA4raJyRCaLzRdcW5vfN641AGJQoD+yCGiC6AEnWZ3lMi67YsrjAlX6
c+gbJKTJFt9mSHzVcVLZlEyxhPufmBOJwppc1D4ypm+CLfWasaBbWBgG1eDzjW0H
U4QWjA==
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=
messagingengine.com; h=cc:content-transfer-encoding:content-type
:date:from:in-reply-to:message-id:mime-version:references
:subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender
:x-sasl-enc; s=fm1; bh=6OZCvLY32skuGKSx2DOvC8m2tvcDq3mcVLCvz4pBR
EU=; b=MP6W7I9Ql/iygZ3pmOi/DZBQ2GVQoPfqYV4SnHSeIs0zmTe6GJpJApAn1
YhvXWGKnjjTMMTJsE0TR1DUizvFTur2wmjd/6Gz4LmmVFqvJrO5MByKC57vQ7sVZ
R0MPHSrlGXDH8jLb8jCOUX41eGnMP2nHIN0c1nyoQvWBsWUaxLh9ttnsKIjbOL9P
HBELac7kCIHO5LweFvOtfdKo3YicI8c5GfnGSZxoh461uStT9JQdRu4n+7osz/tl
J4OC4lHpYvwF8vNyoOiPuM6U81gmkr8vGHDN6MaKXzpHxu7Bvuc+SfptCyRgfrwo
b6l3rA7EQhCx772Tgocbe6827tm3A==
X-ME-Sender: <xms:JxAGYL2DXRZE1T5VtR8nWhF3J_YS5wic3qtjUGZapEdYUF6sscBDpA>
<xme:JxAGYKEBXyr0n8tj5Fqtlkh0GOMoyBoaW50Una-9jbMJHAa0PhB_lGUQW2zqHcwS3
N-E9kQ1iPNNgrRPbA>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrtdelucetufdoteggodetrfdotffvucfrrh
hofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgenuceurghi
lhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujfgurh
ephffvufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgrshcuofho
nhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecuggftrfgrth
htvghrnhepudeggfdvfeduffdtfeeglefghfeukefgfffhueejtdetuedtjeeuieeivdff
geehnecukfhppeejjedrudefgedrvddtfedrudekgeenucevlhhushhtvghrufhiiigvpe
dtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhn
vght
X-ME-Proxy: <xmx:JxAGYL6vtnvRPratRkFdwYTwc8yujKbJcV4GfiizNBaftNuuBCNoJg>
<xmx:JxAGYA30uNogIOR-kAwaUHR0SJrSvXV4uc5lOS4S_bxzJdihRAZqsg>
<xmx:JxAGYOFbQ8oEWtpS5nLK6g3j0xQdEH2jowhKx16Ki7QclmagEcee0Q>
<xmx:KRAGYB02gI86tF4gvDwlwijzV-DnrHVqN1J7jjtpiDcRB3MJV4sOMg>
Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184])
by mail.messagingengine.com (Postfix) with ESMTPA id E4A681080057;
Mon, 18 Jan 2021 17:48:06 -0500 (EST)
From: Thomas Monjalon <thomas@monjalon.net>
To: Anatoly Burakov <anatoly.burakov@intel.com>
Cc: dev@dpdk.org, Liang Ma <liang.j.ma@intel.com>,
David Hunt <david.hunt@intel.com>, Ray Kinsella <mdr@ashroe.eu>,
Neil Horman <nhorman@tuxdriver.com>, konstantin.ananyev@intel.com,
timothy.mcdaniel@intel.com, bruce.richardson@intel.com,
chris.macnamara@intel.com
Date: Mon, 18 Jan 2021 23:48:05 +0100
Message-ID: <6905373.rh0UmGlLeQ@thomas>
In-Reply-To: <a0fd1329af4d912a03a8988e174a78fba27cc723.1610635488.git.anatoly.burakov@intel.com>
References: <cover.1610473000.git.anatoly.burakov@intel.com>
<cover.1610635488.git.anatoly.burakov@intel.com>
<a0fd1329af4d912a03a8988e174a78fba27cc723.1610635488.git.anatoly.burakov@intel.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Subject: Re: [dpdk-dev] [PATCH v17 07/11] power: add PMD power management
API and callback
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
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.