From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; 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 <david.marchand@redhat.com>, Liang Ma <liang.j.ma@intel.com>
Cc: dev <dev@dpdk.org>,
"Ruifeng Wang (Arm Technology China)" <ruifeng.wang@arm.com>,
"Wang, Haiyue" <haiyue.wang@intel.com>,
Bruce Richardson <bruce.richardson@intel.com>,
"Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
David Hunt <david.hunt@intel.com>, Jerin Jacob <jerinjacobk@gmail.com>,
Neil Horman <nhorman@tuxdriver.com>, Thomas Monjalon <thomas@monjalon.net>,
Timothy McDaniel <timothy.mcdaniel@intel.com>,
Gage Eads <gage.eads@intel.com>, Marcin Wojtas <mw@semihalf.com>,
Guy Tzalik <gtzalik@amazon.com>, Ajit Khaparde <ajit.khaparde@broadcom.com>,
Harman Kalra <hkalra@marvell.com>, John Daley <johndale@cisco.com>,
"Wei Hu (Xavier)" <xavier.huwei@huawei.com>,
Ziyang Xuan <xuanziyang2@huawei.com>, Matan Azrad <matan@nvidia.com>,
Yong Wang <yongwang@vmware.com>, Jerin Jacob <jerinj@marvell.com>,
Jan Viktorin <viktorin@rehivetech.com>,
David Christensen <drc@linux.vnet.ibm.com>, Ray Kinsella <mdr@ashroe.eu>
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>
<CAJFAV8zxtPFtv2K1-0tm9Rtb-FsCiG334=RQpXVYqWhkS4kTkQ@mail.gmail.com>
From: "Burakov, Anatoly" <anatoly.burakov@intel.com>
Message-ID: <a760da2d-2552-2a7e-e2f9-7bd9d9cee99b@intel.com>
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: <CAJFAV8zxtPFtv2K1-0tm9Rtb-FsCiG334=RQpXVYqWhkS4kTkQ@mail.gmail.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
On 29-Oct-20 9:27 PM, David Marchand wrote:
> On Tue, Oct 27, 2020 at 4:00 PM Liang Ma <liang.j.ma@intel.com> 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 <anatoly.burakov@intel.com>
>> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
>> Acked-by: David Christensen <drc@linux.vnet.ibm.com>
>> Acked-by: Jerin Jacob <jerinj@marvell.com>
>> Acked-by: Ruifeng Wang <ruifeng.wang@arm.com>
>> Acked-by: Ray Kinsella <mdr@ashroe.eu>
>
> 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 <errno.h>
>>
>> +#include <rte_compat.h>
>> +
>> +/**
>> + * 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