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 DA333A04DF; Fri, 30 Oct 2020 11:09:51 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 11CE5C87A; Fri, 30 Oct 2020 11:09:49 +0100 (CET) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id A0932C85E for ; Fri, 30 Oct 2020 11:09:46 +0100 (CET) IronPort-SDR: AuijvIsfLhQMWxsKQiDEqkBJScfayb0yh6bD+0p3baQEzPk9ndBlaQ/QhH5QX3UX3+cB9zQ5Ne /JVa7Il99fdA== X-IronPort-AV: E=McAfee;i="6000,8403,9789"; a="168683074" X-IronPort-AV: E=Sophos;i="5.77,432,1596524400"; d="scan'208";a="168683074" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 30 Oct 2020 03:09:44 -0700 IronPort-SDR: clRWbx4Y5xkSvKt+dO+f4Lia+cVplNTm1tmHSZiWpGX16aYK0+LV1B0YE1WS7LFul24OXNmcOa /BghmkRcoptg== X-IronPort-AV: E=Sophos;i="5.77,432,1596524400"; d="scan'208";a="537016004" Received: from cflatley-mobl1.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 03:09:39 -0700 To: David Marchand , Liang Ma Cc: dev , "Ruifeng Wang (Arm Technology China)" , "Wang, Haiyue" , Bruce Richardson , "Ananyev, Konstantin" , David Hunt , Jerin Jacob , Neil Horman , Thomas Monjalon , 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> <1603810749-22285-1-git-send-email-liang.j.ma@intel.com> <1603810749-22285-4-git-send-email-liang.j.ma@intel.com> From: "Burakov, Anatoly" Message-ID: Date: Fri, 30 Oct 2020 10:09:37 +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: 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 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; } // 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 :) -- Thanks, Anatoly