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 7C76BA04DF; Fri, 30 Oct 2020 11:14:58 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 5D69AC8CE; Fri, 30 Oct 2020 11:14:56 +0100 (CET) Received: from new3-smtp.messagingengine.com (new3-smtp.messagingengine.com [66.111.4.229]) by dpdk.org (Postfix) with ESMTP id 25A6372F0 for ; Fri, 30 Oct 2020 11:14:54 +0100 (CET) Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailnew.nyi.internal (Postfix) with ESMTP id B60D35804D2; Fri, 30 Oct 2020 06:14:52 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute2.internal (MEProxy); Fri, 30 Oct 2020 06:14:52 -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= /qPu8/bMk2GwdMT/895ql2JMGDSQp/BbnYos38SNbOQ=; b=RNZbm6/sDYIB3i2w 2f7zFYOL18O7nsLe2Xrb+2EIv7cvufnaPLLNTqHxy1nwjcmXyMDDjC8JaMbsHEM8 OY0Ig0LRzqaXXJcH6nE1PQs01Hkyimw+uo5Uhmsjd4NaeGL/2dHUMZuXycc9rDZ2 P7fVBCTk60y87rTjPbtYafkKx0pzS0zqGCOzbbF2yKj0APdcL36hMhvhDA/TtBuW 32GWpEdOF2AYK5ThsR5ZC8on+QHHvFWny7RPyOg0DoiTMI2IklcCpm5ESYEK+q+3 CZBdMcxixtv/CA6BAoRuMp1YKW/vm0G0/k3de0gbYHlAlNjfw8ngYN6tk/adORKS g/ygBg== 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=/qPu8/bMk2GwdMT/895ql2JMGDSQp/BbnYos38SNb OQ=; b=MUKOHOnP5KGYwGa4ZGsNyVKvKUqq4nBc2oah/BREVwru8NccF/7xly4Ab yNUtmivvAPuw4qDQ1JEviCtcxJTk/B87SkvoqGcApIenH+nDCWV+ijYgAI7nynTx iPGQV7M3w1TT3EszW/NzJFSe47NT36nNVpX9o5LOPDPNuvwY9H2IXjUP1Lg2fmgY JOP7yjUzxKOUcsSOB55ud7v9Fo75WS1zCIt0UeGBxQh4Z3jgYQObsS4qshtP75Fd 6v1AjChfnuXJsrdureYzAZ2IFoSq8W/REB9UKk7k8qR3z8qSWYm9aaohhdaWWf0g O5nETFtEaVGrY4ax4EM303eHj9+2g== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrleehgdduvdcutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhmrghs ucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenucggtf frrghtthgvrhhnpedugefgvdefudfftdefgeelgffhueekgfffhfeujedtteeutdejueei iedvffegheenucfkphepjeejrddufeegrddvtdefrddukeegnecuvehluhhsthgvrhfuih iivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepthhhohhmrghssehmohhnjhgrlhho nhdrnhgvth 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 0262E3064685; Fri, 30 Oct 2020 06:14:48 -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 11:14:45 +0100 Message-ID: <2254017.SMxEFvt3eC@thomas> In-Reply-To: References: <1603494392-7181-1-git-send-email-liang.j.ma@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 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. > // 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.