From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 995F645B12; Fri, 11 Oct 2024 15:00:43 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4F790402A1; Fri, 11 Oct 2024 15:00:43 +0200 (CEST) Received: from frasgout.his.huawei.com (frasgout.his.huawei.com [185.176.79.56]) by mails.dpdk.org (Postfix) with ESMTP id B35274028B for ; Fri, 11 Oct 2024 15:00:41 +0200 (CEST) Received: from mail.maildlp.com (unknown [172.18.186.31]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4XQ6DK5rq7z6K61B; Fri, 11 Oct 2024 21:00:17 +0800 (CST) Received: from frapeml100007.china.huawei.com (unknown [7.182.85.133]) by mail.maildlp.com (Postfix) with ESMTPS id 3BA8714010C; Fri, 11 Oct 2024 21:00:41 +0800 (CST) Received: from frapeml500007.china.huawei.com (7.182.85.172) by frapeml100007.china.huawei.com (7.182.85.133) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Fri, 11 Oct 2024 15:00:41 +0200 Received: from frapeml500007.china.huawei.com ([7.182.85.172]) by frapeml500007.china.huawei.com ([7.182.85.172]) with mapi id 15.01.2507.039; Fri, 11 Oct 2024 15:00:41 +0200 From: Konstantin Ananyev To: Bruce Richardson CC: "dev@dpdk.org" , "david.marchand@redhat.com" Subject: RE: [PATCH] eal/x86: cache queried CPU flags Thread-Topic: [PATCH] eal/x86: cache queried CPU flags Thread-Index: AQHbGKkjn841gan5rUG8GrnCJvb5rrKBgtZQ///h74CAACTWwA== Date: Fri, 11 Oct 2024 13:00:40 +0000 Message-ID: References: <20241007110725.377550-1-bruce.richardson@intel.com> <94d414e76dad4378881efd1e6533c06e@huawei.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.206.138.42] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > -----Original Message----- > From: Bruce Richardson > Sent: Friday, October 11, 2024 1:48 PM > To: Konstantin Ananyev > Cc: dev@dpdk.org; david.marchand@redhat.com > Subject: Re: [PATCH] eal/x86: cache queried CPU flags >=20 > On Fri, Oct 11, 2024 at 12:42:01PM +0000, Konstantin Ananyev wrote: > > > > > > > Rather than re-querying the HW each time a CPU flag is requested, we = can > > > just save the return value in the flags array. This should speed up > > > repeated querying of CPU flags, and provides a workaround for a repor= ted > > > issue where errors are seen with constant querying of the AVX-512 CPU > > > flag from a non-AVX VM. > > > > > > Bugzilla Id: 1501 > > > > > > Signed-off-by: Bruce Richardson > > > --- > > > lib/eal/x86/rte_cpuflags.c | 20 +++++++++++++++----- > > > 1 file changed, 15 insertions(+), 5 deletions(-) > > > > > > diff --git a/lib/eal/x86/rte_cpuflags.c b/lib/eal/x86/rte_cpuflags.c > > > index 26163ab746..62e782fb4b 100644 > > > --- a/lib/eal/x86/rte_cpuflags.c > > > +++ b/lib/eal/x86/rte_cpuflags.c > > > @@ -8,6 +8,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > > > > #include "rte_cpuid.h" > > > > > > @@ -21,12 +22,14 @@ struct feature_entry { > > > uint32_t bit; /**< cpuid register bit */ > > > #define CPU_FLAG_NAME_MAX_LEN 64 > > > char name[CPU_FLAG_NAME_MAX_LEN]; /**< String for printing */ > > > + bool has_value; > > > + bool value; > > > }; > > > > > > #define FEAT_DEF(name, leaf, subleaf, reg, bit) \ > > > [RTE_CPUFLAG_##name] =3D {leaf, subleaf, reg, bit, #name }, > > > > > > -const struct feature_entry rte_cpu_feature_table[] =3D { > > > +struct feature_entry rte_cpu_feature_table[] =3D { > > > FEAT_DEF(SSE3, 0x00000001, 0, RTE_REG_ECX, 0) > > > FEAT_DEF(PCLMULQDQ, 0x00000001, 0, RTE_REG_ECX, 1) > > > FEAT_DEF(DTES64, 0x00000001, 0, RTE_REG_ECX, 2) > > > @@ -147,7 +150,7 @@ const struct feature_entry rte_cpu_feature_table[= ] =3D { > > > int > > > rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature) > > > { > > > - const struct feature_entry *feat; > > > + struct feature_entry *feat; > > > cpuid_registers_t regs; > > > unsigned int maxleaf; > > > > > > @@ -156,6 +159,8 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feat= ure) > > > return -ENOENT; > > > > > > feat =3D &rte_cpu_feature_table[feature]; > > > + if (feat->has_value) > > > + return feat->value; > > > > > > if (!feat->leaf) > > > /* This entry in the table wasn't filled out! */ > > > @@ -163,8 +168,10 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t fea= ture) > > > > > > maxleaf =3D __get_cpuid_max(feat->leaf & 0x80000000, NULL); > > > > > > - if (maxleaf < feat->leaf) > > > - return 0; > > > + if (maxleaf < feat->leaf) { > > > + feat->value =3D 0; > > > + goto out; > > > + } > > > > > > #ifdef RTE_TOOLCHAIN_MSVC > > > __cpuidex(regs, feat->leaf, feat->subleaf); > > > @@ -175,7 +182,10 @@ rte_cpu_get_flag_enabled(enum rte_cpu_flag_t fea= ture) > > > #endif > > > > > > /* check if the feature is enabled */ > > > - return (regs[feat->reg] >> feat->bit) & 1; > > > + feat->value =3D (regs[feat->reg] >> feat->bit) & 1; > > > +out: > > > + feat->has_value =3D true; > > > + return feat->value; > > > > If that function can be called by 2 (or more) threads simultaneously, > > then In theory 'feat->has_value =3D true;' can be reordered with > > ' feat->value =3D (regs[feat->reg] >> feat->bit) & 1;' (by cpu or comp= lier) > > and some thread(s) can get wrong feat->value. > > The probability of such collision is really low, but still seems not im= possible. > > >=20 > Well since this code is x86-specific the externally visible store orderin= g > will match the instruction store ordering. Therefore, I think a compiler > barrier is all that is necessary before feat->has_value assignment, > correct? Yep, seems so.