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 35F20A0C41; Wed, 23 Jun 2021 11:43:30 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id AD91B4003F; Wed, 23 Jun 2021 11:43:29 +0200 (CEST) Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by mails.dpdk.org (Postfix) with ESMTP id EE3654003E for ; Wed, 23 Jun 2021 11:43:27 +0200 (CEST) IronPort-SDR: aWhOfwgrUcwAYU6mGcAioS3xG2h+3X3Xrv9Ric3hnI8ck0qnyi+G2hQHhomX8ZJbUPxI4Pcv8B +GeRT4DSxURw== X-IronPort-AV: E=McAfee;i="6200,9189,10023"; a="205402697" X-IronPort-AV: E=Sophos;i="5.83,293,1616482800"; d="scan'208";a="205402697" Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga104.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Jun 2021 02:43:26 -0700 IronPort-SDR: y9w+Lv9+qdSbaXTAvdbRT44pZvYrmWyYUQdGES+9Es/wZbE9BAE3FjZthjGtuvORnyLCj1o1m+ SNP8ty0McrdA== X-IronPort-AV: E=Sophos;i="5.83,293,1616482800"; d="scan'208";a="556090110" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.213.203.112]) ([10.213.203.112]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 23 Jun 2021 02:43:24 -0700 To: "Ananyev, Konstantin" , "dev@dpdk.org" , "Richardson, Bruce" Cc: "Loftus, Ciara" , "Hunt, David" References: <8007029ea9e5129ea43f0c11708169406a16727f.1622548381.git.anatoly.burakov@intel.com> From: "Burakov, Anatoly" Message-ID: <53ac7ee0-e4bc-099a-2ddd-0d74949eea9d@intel.com> Date: Wed, 23 Jun 2021 10:43:21 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v1 1/7] power_intrinsics: allow monitor checks inversion 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" On 21-Jun-21 1:56 PM, Ananyev, Konstantin wrote: > > Hi Anatoly, > >> 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. >> >> This commit adds an option to reverse the check, so that we can have >> monitor sleep 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). >> >> Since the old behavior is the default, no need to adjust existing >> implementations. >> >> Signed-off-by: Anatoly Burakov >> --- >> lib/eal/include/generic/rte_power_intrinsics.h | 4 ++++ >> lib/eal/x86/rte_power_intrinsics.c | 5 ++++- >> 2 files changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/lib/eal/include/generic/rte_power_intrinsics.h b/lib/eal/include/generic/rte_power_intrinsics.h >> index dddca3d41c..1006c2edfc 100644 >> --- a/lib/eal/include/generic/rte_power_intrinsics.h >> +++ b/lib/eal/include/generic/rte_power_intrinsics.h >> @@ -31,6 +31,10 @@ struct rte_power_monitor_cond { >> * 4, or 8. Supplying any other value will result in >> * an error. >> */ >> + uint8_t invert; /**< Invert check for expected value (e.g. instead of >> + * checking if `val` matches something, check if >> + * `val` *doesn't* match a particular value) >> + */ >> }; >> >> /** >> diff --git a/lib/eal/x86/rte_power_intrinsics.c b/lib/eal/x86/rte_power_intrinsics.c >> index 39ea9fdecd..5d944e9aa4 100644 >> --- a/lib/eal/x86/rte_power_intrinsics.c >> +++ b/lib/eal/x86/rte_power_intrinsics.c >> @@ -117,7 +117,10 @@ rte_power_monitor(const struct rte_power_monitor_cond *pmc, >> const uint64_t masked = cur_value & pmc->mask; >> >> /* if the masked value is already matching, abort */ >> - if (masked == pmc->val) >> + if (!pmc->invert && masked == pmc->val) >> + goto end; >> + /* same, but for inverse check */ >> + if (pmc->invert && masked != pmc->val) >> goto end; >> } >> > > Hmm..., such approach looks too 'patchy'... > Can we at least replace 'inver' with something like: > enum rte_power_monitor_cond_op { > _EQ, NEQ,... > }; > Then at least new comparions ops can be added in future. > Even better I think would be to just leave to PMD to provide a comparison callback. > Will make things really simple and generic: > struct rte_power_monitor_cond { > volatile void *addr; > int (*cmp)(uint64_t val); > uint8_t size; > }; > And then in rte_power_monitor(...): > .... > const uint64_t cur_value = __get_umwait_val(pmc->addr, pmc->size); > if (pmc->cmp(cur_value) != 0) > goto end; > .... > I like the idea of a callback, but these are supposed to be intrinsic-like functions, so putting too much into them is contrary to their goal, and it's going to make the API hard to use in simpler cases (e.g. when we're explicitly calling rte_power_monitor as opposed to letting the RX callback do it for us). For example, event/dlb code calls rte_power_monitor explicitly. It's going to be especially "fun" to do these indirect function calls from inside transactional region on call to multi-monitor. I'm not opposed to having a callback here, but maybe others have more thoughts on this? -- Thanks, Anatoly