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 679B6A0546;
Tue, 25 May 2021 11:15:08 +0200 (CEST)
Received: from [217.70.189.124] (localhost [127.0.0.1])
by mails.dpdk.org (Postfix) with ESMTP id 2852540150;
Tue, 25 May 2021 11:15:08 +0200 (CEST)
Received: from mga07.intel.com (mga07.intel.com [134.134.136.100])
by mails.dpdk.org (Postfix) with ESMTP id 085694003F
for <dev@dpdk.org>; Tue, 25 May 2021 11:15:05 +0200 (CEST)
IronPort-SDR: kihEKTmNebVSRH6M6XVWpcBLXi2eR19K3lbDdyhlpAtobKTCr81tunfqxgVCVS7IBtcQVA+MTZ
0DwroHTIAwBg==
X-IronPort-AV: E=McAfee;i="6200,9189,9994"; a="266041487"
X-IronPort-AV: E=Sophos;i="5.82,328,1613462400"; d="scan'208";a="266041487"
Received: from fmsmga004.fm.intel.com ([10.253.24.48])
by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;
25 May 2021 02:15:03 -0700
IronPort-SDR: 81+zbTDRLW9eMp0vhtxRLltOYgyV1ypra+cp0gS8sgWOEz5XjCP8HIPOqTilKevPfpLrn9WJd1
jBhBr5ce8FIA==
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.82,328,1613462400"; d="scan'208";a="464131561"
Received: from fmsmsx604.amr.corp.intel.com ([10.18.126.84])
by fmsmga004.fm.intel.com with ESMTP; 25 May 2021 02:15:03 -0700
Received: from shsmsx606.ccr.corp.intel.com (10.109.6.216) by
fmsmsx604.amr.corp.intel.com (10.18.126.84) with Microsoft SMTP Server
(version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
15.1.2242.4; Tue, 25 May 2021 02:15:02 -0700
Received: from shsmsx601.ccr.corp.intel.com (10.109.6.141) by
SHSMSX606.ccr.corp.intel.com (10.109.6.216) with Microsoft SMTP Server
(version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id
15.1.2242.4; Tue, 25 May 2021 17:15:00 +0800
Received: from shsmsx601.ccr.corp.intel.com ([10.109.6.141]) by
SHSMSX601.ccr.corp.intel.com ([10.109.6.141]) with mapi id 15.01.2242.008;
Tue, 25 May 2021 17:15:00 +0800
From: "Liu, Yong" <yong.liu@intel.com>
To: "Burakov, Anatoly" <anatoly.burakov@intel.com>, "dev@dpdk.org"
<dev@dpdk.org>, "McDaniel, Timothy" <timothy.mcdaniel@intel.com>, "Xing,
Beilei" <beilei.xing@intel.com>, "Wu, Jingjing" <jingjing.wu@intel.com>,
"Yang, Qiming" <qiming.yang@intel.com>, "Zhang, Qi Z" <qi.z.zhang@intel.com>,
"Wang, Haiyue" <haiyue.wang@intel.com>, Matan Azrad <matan@nvidia.com>,
Shahaf Shuler <shahafs@nvidia.com>, Viacheslav Ovsiienko
<viacheslavo@nvidia.com>, "Richardson, Bruce" <bruce.richardson@intel.com>,
"Ananyev, Konstantin" <konstantin.ananyev@intel.com>
CC: "Loftus, Ciara" <ciara.loftus@intel.com>
Thread-Topic: [dpdk-dev] [21.08 PATCH v1 1/2] power: invert the monitor check
Thread-Index: AQHXRnr4qj89uFd6hEWiFDG5S/0lVarz+9Tg
Date: Tue, 25 May 2021 09:15:00 +0000
Message-ID: <4b05f3b389e04e098ab298f49b97d6a2@intel.com>
References: <819ef1ace187365a615d3383e54579e3d9fb216e.1620747068.git.anatoly.burakov@intel.com>
In-Reply-To: <819ef1ace187365a615d3383e54579e3d9fb216e.1620747068.git.anatoly.burakov@intel.com>
Accept-Language: zh-CN, en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
dlp-reaction: no-action
dlp-version: 11.5.1.3
dlp-product: dlpe-windows
x-originating-ip: [10.239.127.36]
Content-Type: text/plain; charset="Windows-1252"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Subject: Re: [dpdk-dev] [21.08 PATCH v1 1/2] power: invert the monitor check
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>
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Anatoly Burakov
> Sent: Tuesday, May 11, 2021 11:32 PM
> To: dev@dpdk.org; McDaniel, Timothy <timothy.mcdaniel@intel.com>; Xing,
> Beilei <beilei.xing@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Yan=
g,
> Qiming <qiming.yang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> Wang, Haiyue <haiyue.wang@intel.com>; Matan Azrad
> <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>; Viacheslav
> Ovsiienko <viacheslavo@nvidia.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>
> Cc: Loftus, Ciara <ciara.loftus@intel.com>
> Subject: [dpdk-dev] [21.08 PATCH v1 1/2] power: invert the monitor check
>=20
> Previously, the semantics of power monitor were such that we were
> checking current value against the expected value, and if they matched,
> then the sleep was aborted. This is somewhat inflexible, because it only
> allowed us to check for a specific value.
>=20
> We can reverse the check, and instead have monitor sleep to be aborted
> if the expected value *doesn't* match what's in memory. This allows us
> to both implement all currently implemented driver code, as well as
> support more use cases which don't easily map to previous semantics
> (such as waiting on writes to AF_XDP counter value).
>=20
Hi Anatoly,
In virtio spec, packed formatted descriptor utilizes two bits for represent=
ing the status. One bit for available status, one bit for used status.=20
For checking the status more precisely, it is need to check value against t=
he expected value.=20
The monitor function in virtio datapath still can work with new semantics, =
but it may lead to some useless io call.=20
Base on that, I'd like to keep previous semantics.
Regards,
Marvin
> This commit also adjusts all current driver implementations to match the
> new semantics.
>=20
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> drivers/event/dlb2/dlb2.c | 2 +-
> drivers/net/i40e/i40e_rxtx.c | 2 +-
> drivers/net/iavf/iavf_rxtx.c | 2 +-
> drivers/net/ice/ice_rxtx.c | 2 +-
> drivers/net/ixgbe/ixgbe_rxtx.c | 2 +-
> drivers/net/mlx5/mlx5_rx.c | 2 +-
> lib/eal/include/generic/rte_power_intrinsics.h | 8 ++++----
> lib/eal/x86/rte_power_intrinsics.c | 4 ++--
> 8 files changed, 12 insertions(+), 12 deletions(-)
>=20
> diff --git a/drivers/event/dlb2/dlb2.c b/drivers/event/dlb2/dlb2.c
> index 3570678b9e..5701bbb8ab 100644
> --- a/drivers/event/dlb2/dlb2.c
> +++ b/drivers/event/dlb2/dlb2.c
> @@ -3188,7 +3188,7 @@ dlb2_dequeue_wait(struct dlb2_eventdev *dlb2,
> &cq_base[qm_port->cq_idx];
> monitor_addr++; /* cq_gen bit is in second 64bit location */
>=20
> - if (qm_port->gen_bit)
> + if (!qm_port->gen_bit)
> expected_value =3D qe_mask.raw_qe[1];
> else
> expected_value =3D 0;
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> index 02cf5e787c..4617ae914a 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -88,7 +88,7 @@ i40e_get_monitor_addr(void *rx_queue, struct
> rte_power_monitor_cond *pmc)
> * we expect the DD bit to be set to 1 if this descriptor was already
> * written to.
> */
> - pmc->val =3D rte_cpu_to_le_64(1 << I40E_RX_DESC_STATUS_DD_SHIFT);
> + pmc->val =3D 0;
> pmc->mask =3D rte_cpu_to_le_64(1 <<
> I40E_RX_DESC_STATUS_DD_SHIFT);
>=20
> /* registers are 64-bit */
> diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
> index 87f7eebc65..d8d9cc860c 100644
> --- a/drivers/net/iavf/iavf_rxtx.c
> +++ b/drivers/net/iavf/iavf_rxtx.c
> @@ -73,7 +73,7 @@ iavf_get_monitor_addr(void *rx_queue, struct
> rte_power_monitor_cond *pmc)
> * we expect the DD bit to be set to 1 if this descriptor was already
> * written to.
> */
> - pmc->val =3D rte_cpu_to_le_64(1 <<
> IAVF_RX_DESC_STATUS_DD_SHIFT);
> + pmc->val =3D 0;
> pmc->mask =3D rte_cpu_to_le_64(1 <<
> IAVF_RX_DESC_STATUS_DD_SHIFT);
>=20
> /* registers are 64-bit */
> diff --git a/drivers/net/ice/ice_rxtx.c b/drivers/net/ice/ice_rxtx.c
> index 92fbbc18da..4e349bfa3f 100644
> --- a/drivers/net/ice/ice_rxtx.c
> +++ b/drivers/net/ice/ice_rxtx.c
> @@ -43,7 +43,7 @@ ice_get_monitor_addr(void *rx_queue, struct
> rte_power_monitor_cond *pmc)
> * we expect the DD bit to be set to 1 if this descriptor was already
> * written to.
> */
> - pmc->val =3D rte_cpu_to_le_16(1 <<
> ICE_RX_FLEX_DESC_STATUS0_DD_S);
> + pmc->val =3D 0;
> pmc->mask =3D rte_cpu_to_le_16(1 <<
> ICE_RX_FLEX_DESC_STATUS0_DD_S);
>=20
> /* register is 16-bit */
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxt=
x.c
> index d69f36e977..2793718171 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -1385,7 +1385,7 @@ ixgbe_get_monitor_addr(void *rx_queue, struct
> rte_power_monitor_cond *pmc)
> * we expect the DD bit to be set to 1 if this descriptor was already
> * written to.
> */
> - pmc->val =3D rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
> + pmc->val =3D 0;
> pmc->mask =3D rte_cpu_to_le_32(IXGBE_RXDADV_STAT_DD);
>=20
> /* the registers are 32-bit */
> diff --git a/drivers/net/mlx5/mlx5_rx.c b/drivers/net/mlx5/mlx5_rx.c
> index 6cd71a44eb..3cbbe5bf59 100644
> --- a/drivers/net/mlx5/mlx5_rx.c
> +++ b/drivers/net/mlx5/mlx5_rx.c
> @@ -282,7 +282,7 @@ int mlx5_get_monitor_addr(void *rx_queue, struct
> rte_power_monitor_cond *pmc)
> return -rte_errno;
> }
> pmc->addr =3D &cqe->op_own;
> - pmc->val =3D !!idx;
> + pmc->val =3D !idx;
> pmc->mask =3D MLX5_CQE_OWNER_MASK;
> pmc->size =3D sizeof(uint8_t);
> return 0;
> diff --git a/lib/eal/include/generic/rte_power_intrinsics.h
> b/lib/eal/include/generic/rte_power_intrinsics.h
> index dddca3d41c..28c481a8d2 100644
> --- a/lib/eal/include/generic/rte_power_intrinsics.h
> +++ b/lib/eal/include/generic/rte_power_intrinsics.h
> @@ -45,10 +45,10 @@ struct rte_power_monitor_cond {
> * Additionally, an expected value (`pmc->val`), mask (`pmc->mask`), and
> data
> * size (`pmc->size`) are provided in the `pmc` power monitoring conditi=
on. If
> * the mask is non-zero, the current value pointed to by the `pmc->addr`
> pointer
> - * will be read and compared against the expected value, and if they mat=
ch,
> the
> - * entering of optimized power state will be aborted. This is intended t=
o
> - * prevent the CPU from entering optimized power state and waiting on a
> write
> - * that has already happened by the time this API is called.
> + * will be read and compared against the expected value, and if they do =
not
> + * match, the entering of optimized power state will be aborted. This is
> + * intended to prevent the CPU from entering optimized power state and
> waiting
> + * on a write that has already happened by the time this API is called.
> *
> * @warning It is responsibility of the user to check if this function i=
s
> * supported at runtime using `rte_cpu_get_intrinsics_support()` API c=
all.
> diff --git a/lib/eal/x86/rte_power_intrinsics.c
> b/lib/eal/x86/rte_power_intrinsics.c
> index 39ea9fdecd..7f0588d70e 100644
> --- a/lib/eal/x86/rte_power_intrinsics.c
> +++ b/lib/eal/x86/rte_power_intrinsics.c
> @@ -116,8 +116,8 @@ rte_power_monitor(const struct
> rte_power_monitor_cond *pmc,
> pmc->addr, pmc->size);
> const uint64_t masked =3D cur_value & pmc->mask;
>=20
> - /* if the masked value is already matching, abort */
> - if (masked =3D=3D pmc->val)
> + /* if the masked value is not matching, abort */
> + if (masked !=3D pmc->val)
> goto end;
> }
>=20
> --
> 2.25.1