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 69AADA04DE; Fri, 30 Oct 2020 17:36:29 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id B53EF377E; Fri, 30 Oct 2020 17:36:27 +0100 (CET) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 0DD2E354D for ; Fri, 30 Oct 2020 17:36:25 +0100 (CET) IronPort-SDR: ZAS51Qi3x+Fkulwvc9sxGLqs/4jHOjDyfqJ85jE3VZ8F7t5kS1YC0SWH/w1BVF8yxI1A9ycnp6 U4F36ARQzUfg== X-IronPort-AV: E=McAfee;i="6000,8403,9790"; a="230268398" X-IronPort-AV: E=Sophos;i="5.77,434,1596524400"; d="scan'208";a="230268398" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Oct 2020 09:36:24 -0700 IronPort-SDR: bbx+vpAiMJyO4+YIrLX0iHsgoEnDwjp/ti14b3zXI7DIvlolM2hzzNTQyReST+lUjSJttwJzmY yhViE3rUisGA== X-IronPort-AV: E=Sophos;i="5.77,434,1596524400"; d="scan'208";a="537120746" Received: from aburakov-mobl.ger.corp.intel.com (HELO [10.213.250.146]) ([10.213.250.146]) by orsmga005-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Oct 2020 09:36:19 -0700 To: Thomas Monjalon 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 References: <1603494392-7181-1-git-send-email-liang.j.ma@intel.com> <1722235.j7YojZp155@thomas> <69e9bc90-0de1-8853-7726-4f5e33610397@intel.com> <6443192.ivzhVBn1yn@thomas> From: "Burakov, Anatoly" Message-ID: <04906abb-cb4e-ab18-cda5-ef0ea244db0f@intel.com> Date: Fri, 30 Oct 2020 16:36:17 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <6443192.ivzhVBn1yn@thomas> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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" On 30-Oct-20 3:44 PM, Thomas Monjalon wrote: > 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? > Aw, crap. You're right, it's an artifact of earlier implementation. We'll fix this. Thanks for letting us know! -- Thanks, Anatoly