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 D6745A04BC; Fri, 9 Oct 2020 12:19:44 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 9CF151C2A1; Fri, 9 Oct 2020 12:19:43 +0200 (CEST) Received: from mail-il1-f195.google.com (mail-il1-f195.google.com [209.85.166.195]) by dpdk.org (Postfix) with ESMTP id 0EB361C2A0 for ; Fri, 9 Oct 2020 12:19:41 +0200 (CEST) Received: by mail-il1-f195.google.com with SMTP id q1so8687930ilt.6 for ; Fri, 09 Oct 2020 03:19:40 -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=gRTZEl1fqrtocZ2DNiWDrh6Gj496VGlaQItBTI0sTBY=; b=G4vyTCUre+y2idAjvTKTO6dsCOxJ30ejuq8CPjs3Ave8La2G+5QljgIUBgyyh7WSz5 RxIl7h30o4tbsmrJldwHrsNjNfuEBjmApF1BHaFJINwx8s5KhPcfFVAZZ9scJdvycrXX UJ8E5SGIjx1hkQIhyJ3NGYryykyprgr8XM72VMvBi4xjAagdj2tu7mzHfcTVgpj8kmbt HCoQk4xoeGQOtdYOVcKh1j/sLraF/HpGokEXV/8q771MRaA6DM+57UPBHK0RDKtRqqeN 0Pij8kH7ZCfrM5HHi8LLzu50h2qbvORyX0XjzMHylg8btMCWd7GDR+lvqoth1IdKcErB T4Qg== 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=gRTZEl1fqrtocZ2DNiWDrh6Gj496VGlaQItBTI0sTBY=; b=HwKAfxx1RofwN9XdklZ2fqAVTtLORRqV6PmpdsQgcPOMsgsz+soSVILziroAf+XQpV YfZZpycnCRRvA9W57YmOjeaMdDmcL7b68bHnxMCP+0/sZN8twqqK0KpB2kK+b1LipVpF z41qRUKlPNC3ZmPa5Tk6WyiAFsMgL8sdC98cwhtdjSvH0A/ls7/iRfoRiHy9zCLE/VHQ YJgku5pHgGjwAnZv4KPOFBBij7KU3bpERHX2qdlJYGTt+qp5SwCoG51riBqYHy/ml7jV 1XoZMlsVxi7P2naon8FMVwZM85pP70NQfiOfUxy0mEfVofn/5bem0pVqrKViS027Iaw+ f2fQ== X-Gm-Message-State: AOAM530GbxpXx9dt+BM2ZZi22gVTjyennEueA7vA1sc6e8/1J1aqdMHe JOCVFhL2SUChWgxbd7EAg3pGA/aweVUjvuQ9+Jc= X-Google-Smtp-Source: ABdhPJyCU0mlVASB7pBhXElMSMQYjlt15OmSczSV/lm3EV75y8478FLZvozyIHq+oi1SDuMW4BriEvmCHgvHVCHrwyY= X-Received: by 2002:a92:770e:: with SMTP id s14mr10187055ilc.130.1602238780175; Fri, 09 Oct 2020 03:19:40 -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> <92b20fa1-f37a-9938-90a7-72956a206b62@intel.com> In-Reply-To: <92b20fa1-f37a-9938-90a7-72956a206b62@intel.com> From: Jerin Jacob Date: Fri, 9 Oct 2020 15:49:23 +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:33 PM Burakov, Anatoly wrote: > > 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). I guess we could remove > that and set return value to either 0 or -ENOTSUP if that would resolve > the issue? returning -ENOTSUP should be OK if we don't to take generic feature-get framework path. > > > 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. Just checked now, librte_power compiles at least for arm64. > > > > > > >> > >> 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 > > > -- > Thanks, > Anatoly