From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id D132EA04BC; Fri, 9 Oct 2020 12:17:56 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id B88201C2A1; Fri, 9 Oct 2020 12:17:54 +0200 (CEST) Received: from new3-smtp.messagingengine.com (new3-smtp.messagingengine.com [66.111.4.229]) by dpdk.org (Postfix) with ESMTP id B6AA01C28F for ; Fri, 9 Oct 2020 12:17:52 +0200 (CEST) Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailnew.nyi.internal (Postfix) with ESMTP id 4BA76580251; Fri, 9 Oct 2020 06:17:51 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute2.internal (MEProxy); Fri, 09 Oct 2020 06:17:51 -0400 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=fm2; bh= pg8aNmlXmzyn8kIxZFiFTb5lbKxenm2/6clcpZfdCJk=; b=GafHQSyfZ7FLpaGT GKrAfF+iNWM3MvhemGDnKb08YmxNf+UEgjTcfTXs+JhGILx4kHswlsf+b4XExkwM OA4HA/DMcBm0Bt0CdtyelPvQL3X/4p26iMCXWELQ94rg3bxOSJ6Hl2kvir2/H1GQ 2xXEh80+0Y83G2K5jje+BcNfjKksWwLVP7pqXefiWykaPebNFz/BXEokl7OmIx2V Z7CfrQgZiRJf39AepoymUYurFwS4hZWaWJsCU6QyGpfq0cXBHvTN4R6VdyRhBDeH AXd0iZCei+y16sRyVNVD4Cl1xTLMUBGsGKArE8WiS6Mpc7f9+D9j4/CoT4h91Gby DlGB8Q== 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=pg8aNmlXmzyn8kIxZFiFTb5lbKxenm2/6clcpZfdC Jk=; b=q5YJ0XP91CNO4KziqCTldZAK7SYrQ+DkHDv6494Dl9jIKW/VTnZ3fwS5l KnS6yZVQGSLBqJg9Ix6zKq43XDlIdj25gE2qwkH4jnWqAe9yVIAvuT3yvtpu0E3s WHbvKa3u9KE7El5et6bb//A+uVHOiE32Zhy8qjCiemK0X0spBTtN5yCAZGxb6I+0 r6PDMwt6MAY9PAarPNiN29sLTcoHRWl3dMao4yUZ72rJNsVmrCFSg6BaFMF35i/V AMaERKkNi66rY8JrmpuLhdP5zYbl6tF0DuHfMZbwJIpLLtLGy7m2jWQ6gbUqSOOB XpLRC9eAQKvWiqZqDOZfuRUiB6UUA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrhedugddvjecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucggtf frrghtthgvrhhnpeffvdffjeeuteelfeeileduudeugfetjeelveefkeejfeeigeehteff vdekfeegudenucffohhmrghinhepughpughkrdhorhhgnecukfhppeejjedrvddthedrge ejrdeileenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhm pehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Received: from xps.localnet (69.47.205.77.rev.sfr.net [77.205.47.69]) by mail.messagingengine.com (Postfix) with ESMTPA id 147A3328005E; Fri, 9 Oct 2020 06:17:46 -0400 (EDT) From: Thomas Monjalon To: Jerin Jacob , "Burakov, Anatoly" Cc: "Ananyev, Konstantin" , David Marchand , "Ma, Liang J" , dpdk-dev , "Hunt, David" , Stephen Hemminger , Honnappa Nagarahalli , "Ruifeng Wang (Arm Technology China)" , David Christensen , Jerin Jacob Date: Fri, 09 Oct 2020 12:17:44 +0200 Message-ID: <1744543.fjzj2HbIQ4@thomas> In-Reply-To: <92b20fa1-f37a-9938-90a7-72956a206b62@intel.com> References: <1599214740-3927-1-git-send-email-liang.j.ma@intel.com> <92b20fa1-f37a-9938-90a7-72956a206b62@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v4 02/10] eal: add power management intrinsics X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" 09/10/2020 12:03, Burakov, Anatoly: > On 09-Oct-20 10:54 AM, Jerin Jacob wrote: > > On Fri, Oct 9, 2020 at 3:10 PM Burakov, Anatoly > > wrote: > >> On 09-Oct-20 10:29 AM, Thomas Monjalon wrote: > >>> 09/10/2020 11:25, Burakov, Anatoly: > >>>> On 09-Oct-20 6:42 AM, Jerin Jacob wrote: > >>>>> On Thu, Oct 8, 2020 at 10:38 PM Ananyev, Konstantin > >>>>> wrote: > >>>>>>> On Thu, Oct 8, 2020 at 6:57 PM Burakov, Anatoly > >>>>>>> wrote: > >>>>>>>> On 08-Oct-20 9:44 AM, Jerin Jacob wrote: > >>>>>>>>> On Thu, Oct 8, 2020 at 2:04 PM Thomas Monjalon wrote: > >>>>>>>>>> > >>>>>>>>>>> Add two new power management intrinsics, and provide an implementation > >>>>>>>>>>> in eal/x86 based on UMONITOR/UMWAIT instructions. The instructions > >>>>>>>>>>> are implemented as raw byte opcodes because there is not yet widespread > >>>>>>>>>>> compiler support for these instructions. > >>>>>>>>>>> > >>>>>>>>>>> The power management instructions provide an architecture-specific > >>>>>>>>>>> function to either wait until a specified TSC timestamp is reached, or > >>>>>>>>>>> optionally wait until either a TSC timestamp is reached or a memory > >>>>>>>>>>> location is written to. The monitor function also provides an optional > >>>>>>>>>>> comparison, to avoid sleeping when the expected write has already > >>>>>>>>>>> happened, and no more writes are expected. > >>>>>>>>>>> > >>>>>>>>>>> For more details, Please reference Intel SDM Volume 2. > >>>>>>>>>> > >>>>>>>>>> I really would like to see feedbacks from other arch maintainers. > >>>>>>>>>> Unfortunately they were not Cc'ed. > >>>>>>>>> > >>>>>>>>> Shared the feedback from the arm64 perspective here. Yet to get a reply on this. > >>>>>>>>> http://mails.dpdk.org/archives/dev/2020-September/181646.html > >>>>>>>>> > >>>>>>>>>> Also please mark the new functions as experimental. > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>> > >>>>>>>> Hi Jerin, > >>>>>>> > >>>>>>> Hi Anatoly, > >>>>>>> > >>>>>>>> > >>>>>>>> > IMO, We must introduce some arch feature-capability _get_ scheme to tell > >>>>>>>> > the consumer of this API is only supported on x86. Probably as > >>>>>>>> functions[1] > >>>>>>>> > or macro flags scheme and have a stub for the other architectures as the > >>>>>>>> > API marked as generic ie rte_power_* not rte_x86_.. > >>>>>>>> > > >>>>>>>> > This will help the consumer to create workers based on the > >>>>>>>> instruction features > >>>>>>>> > which can NOT be abstracted as a generic feature across the > >>>>>>>> architectures. > >>>>>>>> > >>>>>>>> I'm not entirely sure what you mean by that. > >>>>>>>> > >>>>>>>> I mean, yes, we should have added stubs for other architectures, and we > >>>>>>>> will add those in future revisions, but what does your proposed runtime > >>>>>>>> check accomplish that cannot currently be done with CPUID flags? > >>>>>>> > >>>>>>> > >>>>>>> RTE_CPUFLAG_WAITPKG flag definition is not available in other architectures. > >>>>>>> i.e RTE_CPUFLAG_WAITPKG defined in lib/librte_eal/x86/include/rte_cpuflags.h > >>>>>>> and it is used in http://patches.dpdk.org/patch/79540/ as generic API. > >>>>>>> I doubt http://patches.dpdk.org/patch/79540/ would compile on non-x86. > >>>>>> > >>>>>> > >>>>>> I am agree with Jerin, that we need some generic way to > >>>>>> figure-out does platform supports power_monitor() or not. > >>>>>> Though not sure do we need to create a new feature-get framework here... > >>>>> > >>>>> That's works too. Some means of generic probing is fine. Following > >>>>> schemed needs > >>>>> more documentation on that usage, as, it is not straight forward compare to > >>>>> feature-get framework. Also, on the other thread, we are adding the > >>>>> new instructions like > >>>>> demote cacheline etc, maybe if the user wants to KNOW if the arch > >>>>> supports it then > >>>>> the feature-get framework is good. > >>>>> If we think, there is no other usecase for generic arch feature-get > >>>>> framework then > >>>>> we can keep the below scheme else generic arch feature is better for > >>>>> more forward > >>>>> looking use cases. > >>>>> > >>>>>> Might be just something like: > >>>>>> rte_power_monitor(...) == -ENOTSUP > >>>>>> be enough indication for that? > >>>>>> So user can just do: > >>>>>> if (rte_power_monitor(NULL, 0, 0, 0, 0) == -ENOTSUP) { > >>>>>> /* not supported path */ > >>>>>> } > >>>>>> > >>>>>> To check is that feature supported or not. > >>>>> > >>>>> > >>>> > >>>> Looking at CLDEMOTE patches, CLDEMOTE is a noop on other archs. I think > >>>> we can safely make this intrinsic as a noop on other archs as well, as > >>>> it's functionally identical to waking up immediately. > >>>> > >>>> If we're not creating this for CLDEMOTE, we don't need it here as well. > >>>> If we do need it for this, then we arguably need it for CLDEMOTE too. > >>> > >>> Sorry I don't understand what you mean, too many "it" and "this" :) > >>> > >> > >> Sorry, i meant "the generic feature-get framework". CLDEMOTE doesn't > >> exist on other archs, this doesn't too, so it's a fairly similar > >> situation. Stubbing UMWAIT with a noop is a valid approach because it's > >> equivalent to sleeping and then immediately waking up (which can happen > >> for a host of reasons unrelated to the code itself). > > > > If we are keeping the following return in the public API then it can not be NOP > > + * @return > > + * - 1 if wakeup was due to TSC timeout expiration. > > + * - 0 if wakeup was due to memory write or other reasons. > > + */ > > > > In the generic header, it is specified that return value is > implementation-defined (i.e. arch-specific). Obviously an API definition should *never* be "implementation-defined". > I guess we could remove > that and set return value to either 0 or -ENOTSUP if that would resolve > the issue? > > > Also, we need to fix compilation issue if any with > > http://patches.dpdk.org/patch/79540/ > > as it has direct reference to if > > (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_WAITPKG)) { > > Either we need to add -ENOTSUP return or generic feature-get framework. > > IIRC power library isn't compiled on anything other than x86, so this > code wouldn't get compiled. It is not call "power-x86", so we must assume it could work on any architecture. > >> I'm not against a generic feature-get framework, i'm just pointing out > >> that if this is what's preventing the merge, it should prevent the merge > >> of CLDEMOTE as well, yet Jerin has acked that one and has explicitly > >> stated that he's OK with leaving CLDEMOTE as a noop on other architectures. CLDEMOTE is used for optimization, while UMWAIT can be used in a logic, that's why the expectations may be different.