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 218F1A04BC; Fri, 9 Oct 2020 11:55:19 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 75E691C194; Fri, 9 Oct 2020 11:55:16 +0200 (CEST) Received: from mail-io1-f68.google.com (mail-io1-f68.google.com [209.85.166.68]) by dpdk.org (Postfix) with ESMTP id D2A3E1C125 for ; Fri, 9 Oct 2020 11:55:13 +0200 (CEST) Received: by mail-io1-f68.google.com with SMTP id q202so178194iod.9 for ; Fri, 09 Oct 2020 02:55:13 -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=T8QBb/KIBvY1b2E1VDZVzns+Xht/KkV9T1hmo5UY5Us=; b=D6N4PdWNq6I8lvYd3DRmQG1VVh44Kvg4LGUiGTfAoM/CtimFc+dG1kAv1Ob7PnOJa4 4Dq8htt6Ubv3jeMXSB/+c+lN1Zz+fi1jMhaweaCdUaMU8u+2Pf+zzMBUtA8ETbg8eYzk cJUr6PFVEFys4G6Xz4hHkVxmwbsGeoAtWUwphD09hA/XXrkpUBGdTsOq965QpMnhQF97 /x1tvVBqkhSBDnD8O3+vyfPl1LAGLeo4iVtpe58dq3F4Tv2wvgmRMjxmzH32mDkEbNpB ji1Ml6drPgUAb4WQbKzid0NCxzq6elWO1C4kNmrHoGvkDNy5HHCvmyb1oRBoUK2DrBof K/pg== 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=T8QBb/KIBvY1b2E1VDZVzns+Xht/KkV9T1hmo5UY5Us=; b=Xngd0LZxdzvtaTnChXxmzdxf9n4XYuO1/aCmS2nX3ImVWVRYSSNNUefhae87vFjXGa 5O0ni8s0mavekJrxKeHgLHQKBuOumOI5ENyh7nA3+faPzmOdl6Mc4dcNI5bYU7hI2OLU euDzGmE0wjlOqAXZcT4F4LXAp8tOChbA6ujaL15GCMxv+Npxt/MwY+7AapdiOvExuYZX 6K1r6w0MaWBCIoGvi2oCjeEjGZE/KrcK7acmnq5c2KO/9fx/3HS4Yehl5EprpJtgSqK4 sxCp7ZTGHDVOkrQJeQO4cPkWXJ/38965Y/XGKaG6NK39SMSLIvtZ6m2XfJChtD8sgmG2 KmZA== X-Gm-Message-State: AOAM533Zn1vW5UZQgg7xOW3RWzH86tmLNg8M1pVDIafv9Wj5S1NSNVf+ HwhadMnW6/T2nTAqjupj20S5bQeWWyC26qNDhQw= X-Google-Smtp-Source: ABdhPJyg35zLavdR1OgTiaTFlZsRbBaE7UVfUgbnMrKh8ITWravD2UoXSV015TxZT28rY4Ly0MFZ2csMByQ4XZCUtr4= X-Received: by 2002:a02:a196:: with SMTP id n22mr10379495jah.104.1602237313151; Fri, 09 Oct 2020 02:55:13 -0700 (PDT) MIME-Version: 1.0 References: <1599214740-3927-1-git-send-email-liang.j.ma@intel.com> <3735900.a7ZSN3H2iV@thomas> <4d9bca84-a265-c87c-1200-e1981bb7e166@intel.com> In-Reply-To: <4d9bca84-a265-c87c-1200-e1981bb7e166@intel.com> From: Jerin Jacob Date: Fri, 9 Oct 2020 15:24:56 +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: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. + */ 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. > > 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. > > -- > Thanks, > Anatoly