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 BB169A04DE; Fri, 30 Oct 2020 15:09:58 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 8378BC95E; Fri, 30 Oct 2020 15:09:56 +0100 (CET) Received: from new4-smtp.messagingengine.com (new4-smtp.messagingengine.com [66.111.4.230]) by dpdk.org (Postfix) with ESMTP id 7E1E9C94A for ; Fri, 30 Oct 2020 15:09:55 +0100 (CET) Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailnew.nyi.internal (Postfix) with ESMTP id C0D20580153; Fri, 30 Oct 2020 10:09:53 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute2.internal (MEProxy); Fri, 30 Oct 2020 10:09:53 -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= KVOUZkNsdozSw/dJ5AFXrRkC9iRAAM56arSunGEUbO8=; b=TSsY+6twEvMWo42s eKv6eQ49gdtyqySlT+bTP0JAkoePJzKbqWN3JuIZOiuS7Q0XWAQdPHwQXQFcgtly 7gmY1GMAGwjDfBpwOjGm49zQz60dmoIa7XQuxskZyCwsfa8+uyk7spXlu2bJAksw RnhD1BfFdAHihwSQf7ukt6S+/cX8/TPQNkBf4OhFxA+CPhw/5O36XzD3yvO/tyYr zjNq1lG+0UYQQ/vAQXrk7l1sUuL5CYIzN0uIRjGIvLWTX/QY7L6yKVGHx5e3DJ1p ugzd++F7qJiz9s1e/r3WTfxdP0AXeGh8FeHE232RZoayTgEjCik80gMd4ucNJ1LK b8105Q== 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=KVOUZkNsdozSw/dJ5AFXrRkC9iRAAM56arSunGEUb O8=; b=DEwAld750MUEYbLMU1b64p2TbCG8fXtO3yqk08YbC/Va+ZFHCEveqPX8x XWQI9ztrtrCJAf6aTIoJHT8Sr/zRHlvJ0l0XtclcAIp3mgpFXB4SesHvZ5rJR0PJ JT2wMTxZl/Iy++Qf1iFisqLxdgW82mb9D8gd3Yp7KYryQRylXFkQuisT+zMhe1Uo WwcMiAq16vF+TIMw+Ri9CY0hYjXckVwwtL+d4cIAEm9gqiJOCpzJFruGLvv7yBWe wtDlB0qQPC80Pi9f6pPAf44xAoNF49vP6p+OxH0hfeF7KRya7pT5R9dLK194RduJ LczMiCLjBtrO9Ku460crv70mjQmLg== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedrleehgdehiecutefuodetggdotefrodftvf 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 90A233064683; Fri, 30 Oct 2020 10:09:49 -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 15:09:47 +0100 Message-ID: <1722235.j7YojZp155@thomas> In-Reply-To: <87b44b76-1f44-f33f-b5b9-b4c250a88808@intel.com> References: <1603494392-7181-1-git-send-email-liang.j.ma@intel.com> <2254017.SMxEFvt3eC@thomas> <87b44b76-1f44-f33f-b5b9-b4c250a88808@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 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? > 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. > 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.