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 7FDEAA04DE; Fri, 30 Oct 2020 16:45:04 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 24936C9BE; Fri, 30 Oct 2020 16:45:02 +0100 (CET) Received: from new2-smtp.messagingengine.com (new2-smtp.messagingengine.com [66.111.4.224]) by dpdk.org (Postfix) with ESMTP id 3C26EC9BE for ; Fri, 30 Oct 2020 16:45:00 +0100 (CET) Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailnew.nyi.internal (Postfix) with ESMTP id AA6E358066E; Fri, 30 Oct 2020 11:44:56 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute2.internal (MEProxy); Fri, 30 Oct 2020 11:44:56 -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= qB2uGQUNrwkNQTH9J2mMiW/EBzgzLkXV/BadoZdgbOs=; b=IRMTvbAUBMV4FMY3 pInchSuATjAM431cz+kJUoBXHGt8gScMkGFr96Nz0m1o8xkUzNbJLrKchHHXKbYF NLq4fX29LuuWrEDIos10mbhGxoi4hbfvrwJxQkn4y2B1NZHguDELFXCz4tU0nB77 +4F6AG9hZ+W/hGmtevEOZd9OQhp4yUqcJTT96gzhlTIooQnUA4c73P8U4B8nJNcR AnYhTqhrkTOI1QJ1rVb8a8J7ak31lFOpaizPN4spWauFJafgfVtcki39do5gT3Cd X35dzS1uh78onoaiank4rYMKY/9LvSNlmLEnyN0q9QpvHP9wH6zr/3788j+ebcIh Q9qz4A== 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=qB2uGQUNrwkNQTH9J2mMiW/EBzgzLkXV/BadoZdgb Os=; b=WVcxsaIYdR7RxfcAvmLTk8IUITM1PW7Q1INkl8uS4uJFzYgs+SiN86COs NYyWKtf2AW2PQ44Zcl5yRBWIKZToQRb6eflTKkFt44JEa1ndA/SG36nySISU87Nx gYrTFCSF2D9rt0DBYL4g58zQFdvjvgrnn7YTvOfT2hgvyMtp0sgGldYndflCUgfe dEU8+jJCUfoAvDJXK0GPAcXTuNealPGnB9pfpN+hGMM+SzVLVPgAQ8VeMsVDDdQ7 7LavzZpMnF/MlzkhfclLsdl4mFEyNeO8XSokFELL0UXNKZ2Wc5kDJtwjj6ypRZQO Zr8vW+QxZUJRspFjFw9ve8uJrgOMA== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrleehgdejhecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucggtf frrghtthgvrhhnpeffvdffjeeuteelfeeileduudeugfetjeelveefkeejfeeigeehteff vdekfeegudenucffohhmrghinhepughpughkrdhorhhgnecukfhppeejjedrudefgedrvd dtfedrudekgeenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhr ohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 1F2193064684; Fri, 30 Oct 2020 11:44:52 -0400 (EDT) From: Thomas Monjalon To: "Burakov, Anatoly" Cc: David Marchand , Liang Ma , dev , "Ruifeng Wang (Arm Technology China)" , "Wang, Haiyue" , Bruce Richardson , "Ananyev, Konstantin" , David Hunt , Jerin Jacob , Neil Horman , Timothy McDaniel , Gage Eads , Marcin Wojtas , Guy Tzalik , Ajit Khaparde , Harman Kalra , John Daley , "Wei Hu (Xavier)" , Ziyang Xuan , Matan Azrad , Yong Wang , Jerin Jacob , Jan Viktorin , David Christensen , Ray Kinsella Date: Fri, 30 Oct 2020 16:44:50 +0100 Message-ID: <6443192.ivzhVBn1yn@thomas> In-Reply-To: <69e9bc90-0de1-8853-7726-4f5e33610397@intel.com> References: <1603494392-7181-1-git-send-email-liang.j.ma@intel.com> <1722235.j7YojZp155@thomas> <69e9bc90-0de1-8853-7726-4f5e33610397@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v10 3/9] eal: add intrinsics support check infrastructure 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" 30/10/2020 16:27, Burakov, Anatoly: > On 30-Oct-20 2:09 PM, Thomas Monjalon wrote: > > 30/10/2020 14:37, Burakov, Anatoly: > >> On 30-Oct-20 10:14 AM, Thomas Monjalon wrote: > >>> 30/10/2020 11:09, Burakov, Anatoly: > >>>> On 29-Oct-20 9:27 PM, David Marchand wrote: > >>>>> On Tue, Oct 27, 2020 at 4:00 PM Liang Ma wrote: > >>>>>> > >>>>>> Currently, it is not possible to check support for intrinsics that > >>>>>> are platform-specific, cannot be abstracted in a generic way, or do not > >>>>>> have support on all architectures. The CPUID flags can be used to some > >>>>>> extent, but they are only defined for their platform, while intrinsics > >>>>>> will be available to all code as they are in generic headers. > >>>>>> > >>>>>> This patch introduces infrastructure to check support for certain > >>>>>> platform-specific intrinsics, and adds support for checking support for > >>>>>> IA power management-related intrinsics for UMWAIT/UMONITOR and TPAUSE. > >>>>>> > >>>>>> Signed-off-by: Anatoly Burakov > >>>>>> Signed-off-by: Liang Ma > >>>>>> Acked-by: David Christensen > >>>>>> Acked-by: Jerin Jacob > >>>>>> Acked-by: Ruifeng Wang > >>>>>> Acked-by: Ray Kinsella > >>>>> > >>>>> Coming late to the party, it seems crowded... > >>>>> > >>>>> > >>>>> > >>>>>> diff --git a/lib/librte_eal/include/generic/rte_cpuflags.h b/lib/librte_eal/include/generic/rte_cpuflags.h > >>>>>> index 872f0ebe3e..28a5aecde8 100644 > >>>>>> --- a/lib/librte_eal/include/generic/rte_cpuflags.h > >>>>>> +++ b/lib/librte_eal/include/generic/rte_cpuflags.h > >>>>>> @@ -13,6 +13,32 @@ > >>>>>> #include "rte_common.h" > >>>>>> #include > >>>>>> > >>>>>> +#include > >>>>>> + > >>>>>> +/** > >>>>>> + * Structure used to describe platform-specific intrinsics that may or may not > >>>>>> + * be supported at runtime. > >>>>>> + */ > >>>>>> +struct rte_cpu_intrinsics { > >>>>>> + uint32_t power_monitor : 1; > >>>>>> + /**< indicates support for rte_power_monitor function */ > >>>>>> + uint32_t power_pause : 1; > >>>>>> + /**< indicates support for rte_power_pause function */ > >>>>>> +}; > >>>>> > >>>>> - The rte_power library is supposed to be built on top of cpuflags. > >>>>> Not the other way around. > >>>>> Those capabilities should have been kept inside the rte_power_ API and > >>>>> not pollute the cpuflags API. > >>>>> > >>>>> - All of this should have come as a single patch as the previously > >>>>> introduced API is unusable before. > >>>>> > >>>>> > >>>>>> + > >>>>>> +/** > >>>>>> + * @warning > >>>>>> + * @b EXPERIMENTAL: this API may change without prior notice > >>>>>> + * > >>>>>> + * Check CPU support for various intrinsics at runtime. > >>>>>> + * > >>>>>> + * @param intrinsics > >>>>>> + * Pointer to a structure to be filled. > >>>>>> + */ > >>>>>> +__rte_experimental > >>>>>> +void > >>>>>> +rte_cpu_get_intrinsics_support(struct rte_cpu_intrinsics *intrinsics); > >>>>>> + > >>>>>> /** > >>>>>> * Enumeration of all CPU features supported > >>>>>> */ > >>>>>> diff --git a/lib/librte_eal/include/generic/rte_power_intrinsics.h b/lib/librte_eal/include/generic/rte_power_intrinsics.h > >>>>>> index fb897d9060..03a326f076 100644 > >>>>>> --- a/lib/librte_eal/include/generic/rte_power_intrinsics.h > >>>>>> +++ b/lib/librte_eal/include/generic/rte_power_intrinsics.h > >>>>>> @@ -32,6 +32,10 @@ > >>>>>> * checked against the expected value, and if they match, the entering of > >>>>>> * optimized power state may be aborted. > >>>>>> * > >>>>>> + * @warning It is responsibility of the user to check if this function is > >>>>>> + * supported at runtime using `rte_cpu_get_features()` API call. Failing to do > >>>>>> + * so may result in an illegal CPU instruction error. > >>>>>> + * > >>>>> > >>>>> - Reading this API description... what am I supposed to do in my > >>>>> application or driver who wants to use the new > >>>>> rte_power_monitor/rte_power_pause stuff? > >>>>> > >>>>> I should call rte_cpu_get_features(TOTO) ? > >>>>> This comment does not give a hint. > >>>>> > >>>>> I suppose the intent was to refer to the rte_cpu_get_intrinsics_support() thing. > >>>>> This must be fixed. > >>>>> > >>>>> > >>>>> - Again, I wonder why we are exposing all this stuff. > >>>>> This should be hidden in the rte_power API. > >>>>> > >>>> > >>>> We're exposing all of this here because the intrinsics are *not* part of > >>>> the power API but rather are generic headers within EAL. Therefore, any > >>>> infrastructure checking for their support can *not* reside in the power > >>>> library, but instead has to be in EAL. > >>>> > >>>> The intended usage here is to call this function before calling > >>>> rte_power_monitor(), such that: > >>>> > >>>> struct rte_cpu_intrinsics intrinsics; > >>>> > >>>> rte_cpu_get_intrinsics_support(&intrinsics); > >>>> > >>>> if (!intrinsics.power_monitor) { > >>>> // rte_power_monitor not supported and cannot be used > >>>> return; > >>>> } > >>> > >>> This check could be done inside the rte_power API. > >> > >> I'm not quite clear on exactly what you're asking here. > >> > >> Do you mean the example code above? If so, code like that is already > >> present in the power library, at the callback enable stage. > >> > >> If you mean to say, i should put this check into the rte_power_monitor > >> intrinsic, then no, i don't think it's a good idea to have this > >> expensive check every time you call rte_power_monitor. > > > > No but it can be done at initialization time. > > According to what you say above, it is alread done at callback enable stage. > > So the app does not need to do the check? > > Admittedly it's a bit confusing, but please bear with me. > > There are two separate issues at hand: the intrinsic itself, and the > calling code. We provide both. > > That means, the *calling code* should do the check. In our case, *our* > calling code is the callback. However, nothing stops someone else from > implementing their own scheme using our intrinsic - in that case, the > user will be responsible to check if the intrinsic is supported before > using it in their own code, because they won't be using our callback but > will be using our intrinsic. > > So, we have a check *in our calling code*. But if someone were to use > the *intrinsic* directly (like DLB), they would have to add their own > checks around the intrinsic usage. > > Our power intrinsic is a static inline function. Are you proposing to > add some sort of function pointer wrapper and make it an indirect call > instead of a static inline function? (or indeed a proper function) > > > > >> If you mean put this entire infrastructure into the power API - well, > >> that kinda defeats the purpose of both having these intrinsics in > >> generic headers and having a generic CPU feature check infrastructure > >> that was requested of us during the review. We of course can move the > >> intrinsic to the power library and outside of EAL, but then anything > >> that requires UMWAIT will have to depend on the librte_power. > > > > Yes the intrinsics can be in EAL if usable. > > But it seems DLB author cannot use what is in EAL. > > I'll let the DLB authors clarify that themselves, but as far as i'm > aware, it seems that this is not the case - while their current code > wouldn't be able to use these intrinsics by search-and-replace, they > will be able to use them with a couple of changes to their code that > basically amounted to reimplementation of our intrinsics. > > > > >> Please clarify exactly what changes you would like to see here, and what > >> is your objection. > >> > >>> > >>>> // proceed with rte_power_monitor usage > >>>> > >>>> Failing to do that will result in either -ENOTSUP on non-x86, or illegal > >>>> instruction crash on x86 that doesn't have that instruction (because we > >>>> encode raw opcode). > >>>> > >>>> I've *not* added this to the previous patches because i wanted to get > >>>> this part reviewed specifically, and not mix it with other IA-specific > >>>> stuff. It seems that i've succeeded in that goal, as this patch has 4 > >>>> likes^W acks :) > >>> > >>> You did not explain the need for rte_cpu_get_features() call. > >>> > >> > >> Did not explain *where*? Are you suggesting i put things about > >> rte_power_monitor into documentation for rte_cpu_get_intrinsics_support? > >> The documentation for rte_power_monitor already states that one should > >> use rte_cpu_get_intrinsics_support API to check if the rte_power_monitor > >> is supported on current machine. What else is missing? > > > > In your example above, you do not call rte_cpu_get_features() > > which is documented as required in the EAL doc. > > > > I'm not sure i follow. This is unrelated to rte_cpu_get_features call. > The rte_cpu_get_features is a CPUID check, and it was decided not to use > it because the WAITPKG CPUID flag is only defined for x86 and not for > other archs. This new call (rte_cpu_get_intrinsics_support) is non-arch > specific, but will have an arch-specific implementation (which happens > to use rte_cpu_get_features to detect support for WAITPKG). I have given > the example code of how to detect support for rte_power_monitor using > this new code, in the code example you just referred to. Please read the API again: http://git.dpdk.org/dpdk/tree/lib/librte_eal/include/generic/rte_power_intrinsics.h " * @warning It is responsibility of the user to check if this function is * supported at runtime using `rte_cpu_get_features()` API call. * Failing to do so may result in an illegal CPU instruction error. " Why is it referring to rte_cpu_get_features?