From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; 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" To: "Burakov, Anatoly" , "dev@dpdk.org" , "McDaniel, Timothy" , "Xing, Beilei" , "Wu, Jingjing" , "Yang, Qiming" , "Zhang, Qi Z" , "Wang, Haiyue" , Matan Azrad , Shahaf Shuler , Viacheslav Ovsiienko , "Richardson, Bruce" , "Ananyev, Konstantin" CC: "Loftus, Ciara" 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: dev On Behalf Of Anatoly Burakov > Sent: Tuesday, May 11, 2021 11:32 PM > To: dev@dpdk.org; McDaniel, Timothy ; Xing, > Beilei ; Wu, Jingjing ; Yan= g, > Qiming ; Zhang, Qi Z ; > Wang, Haiyue ; Matan Azrad > ; Shahaf Shuler ; Viacheslav > Ovsiienko ; Richardson, Bruce > ; Ananyev, Konstantin > > Cc: Loftus, Ciara > 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 > --- > 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