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 589D0A04BC; Fri, 9 Oct 2020 12:46:08 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E63EA1D16E; Fri, 9 Oct 2020 12:46:06 +0200 (CEST) Received: from mail-il1-f196.google.com (mail-il1-f196.google.com [209.85.166.196]) by dpdk.org (Postfix) with ESMTP id E671F1D15A for ; Fri, 9 Oct 2020 12:46:03 +0200 (CEST) Received: by mail-il1-f196.google.com with SMTP id q1so8744641ilt.6 for ; Fri, 09 Oct 2020 03:46:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=wm1rcFwVKIoIRwsYzGvN1fKF+iw6j32Lf2A1rfVTCMk=; b=br7luKd1AmPk9PBXaJeJuiHnm4FKSz+rnxUoKms5juFaNdPJAsqi+pqjPkklmuQQgI uUrpET29c8noKdxYBZ9RVzsemJadDC6GaZul7x7Safs0R9j4AYMI+0qvhj1Ppw+gugX+ gt5iHJeylNndZEm6+cIMv2E1poAumoTIuiDP26T7kjw6Qodqtea6QLQ1U9lghgxY84P7 oNTQqjGJuexL0XP8ZZ1bVez29dx+KrWg2I76Wsb9pWuvzr9ysInizFjaDEwDK0iZTd3J VjNvE1V6KjAMA9aCKrn0YBfk5Vz0P7xta0b9eLEG7T87AEo0eAAGqeb/jFvk5GqzsYf/ 2pyQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=wm1rcFwVKIoIRwsYzGvN1fKF+iw6j32Lf2A1rfVTCMk=; b=mwRKnk4TvDsDFCmAfaIed6WTmEjdaSpytlLn6SL4A6Cv1QPcaKL//0PuTEPQUmY9Qu IIDtlNxXbrvbuX1DUtCKvdYPf9FmnOlNUYntdRxYvIPDsp4k+bMbloboF/mi0ea/naEq M5E1oGP7vwWjqKPdaDna0TMk3bI4b12egUnfSiX/Dod7/Cgc3s9qH/LxRXM2MpOzdjdL H3FfKBK4E7+LDiLgYpQ/uVJQhQf7FJkH9niNg9knem1qGQgyHzzVX6ZAdF674FgL9h/z XuQ/EF+1CHeWubEfPIH/IeY43NZ/1uDCGjcSE2/+Z/UoCiAPLKejiq6SjdjwSul/KyQL oAcA== X-Gm-Message-State: AOAM531+oRVbL2OzgM1N9Iiv57v3JmhF2F7G3uy6h855514f2XT9tEOg OK0nM2ze6VhsDVYrj4M93kS0uLx51/a4E4fHHYY= X-Google-Smtp-Source: ABdhPJwCBqiZbZeYquK7h05QXFrIKXy595QvpWV0iEhtX7qWEBXmomwOMUAgzVgUF2nvQnEXcytlcbjMWZ0fcbOlEe8= X-Received: by 2002:a92:1e02:: with SMTP id e2mr8352186ile.294.1602240362133; Fri, 09 Oct 2020 03:46:02 -0700 (PDT) MIME-Version: 1.0 References: <1599214740-3927-1-git-send-email-liang.j.ma@intel.com> <92b20fa1-f37a-9938-90a7-72956a206b62@intel.com> <1744543.fjzj2HbIQ4@thomas> In-Reply-To: From: Jerin Jacob Date: Fri, 9 Oct 2020 16:15:45 +0530 Message-ID: To: "Burakov, Anatoly" Cc: Thomas Monjalon , "Ananyev, Konstantin" , David Marchand , "Ma, Liang J" , dpdk-dev , "Hunt, David" , Stephen Hemminger , Honnappa Nagarahalli , "Ruifeng Wang (Arm Technology China)" , David Christensen , Jerin Jacob Content-Type: text/plain; charset="UTF-8" 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" On Fri, Oct 9, 2020 at 3:53 PM Burakov, Anatoly wrote: > > On 09-Oct-20 11:17 AM, Thomas Monjalon wrote: > > 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". > > If there isn't a meaningful return value, we could either make it a > void, or return 0/-ENOTSUP so. I'm OK with either as nop is a valid > result for a UMWAIT, and there are no side-effects to the intrinsic > itself (it's basically a fancy rte_pause). > > > > > > >> 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. > > #ifdef it is! > > > > > > >>>> 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. > > > > UMWAIT is a best-effort mechanism with no side-effects. It's perfectly > legal for a UMWAIT to not sleep at all, thus rendering it effectively a > noop. So i don't think it's all that different. If a platform does not support UMWAIT in ALL case IMO, no consumer takes this the path for power saving. So IMO, t is different than CLDEMOTE > > -- > Thanks, > Anatoly