General comments; Thanks for reworking, indeed removing dependencies for CPU ISA checking is a good improvement, and allows CPU ISA optimized implementations to run more often.
> -----Original Message----- > From: David Marchand <[email protected]> > Sent: Tuesday, November 30, 2021 2:51 PM > To: [email protected] > Cc: [email protected]; Stokes, Ian <[email protected]>; Amber, Kumar > <[email protected]>; Van Haaren, Harry <[email protected]> > Subject: [PATCH v3] dpif-netdev: Call cpuid for x86 isa availability. > > DPIF AVX512 optimisations currently rely on DPDK availability while > they can be used without DPDK. > Besides, checking for availability of some isa only has to be done once > and won't change while a OVS process runs. > > Resolve isa availability in constructors by using a simplified query > based on cpuid API that comes from the compiler. Using constructors instead of an init() time call is interesting, but may not be what we always want. For "vswitchd" it is a useful startup feature, however other binaries/tools such as "ovs-vsctl" or "ovs-appctl" do not require CPUID-based ISA detection at all. As per this patch, every launch of "ovs-vsctl" (or other tooling/binaries) will cause the constructors to run. I would like to add some VLOG_* info/logging to the CPU ISA detection, it may be useful to understand the system if in future debug of CPU ISA implementations is required. (This is how the constructor-running was identified, lots of printf() at tooling startup!) > Note: this also fixes the check on BMI2 availability: DPDK had a bug > for this isa, see https://git.dpdk.org/dpdk/commit/?id=aae3037ab1e0. Yes, this has been resolved in DPDK (thanks to your linked patch), and will be consumed into the next OVS 2.17 release. > Suggested-by: Ilya Maximets <[email protected]> > Signed-off-by: David Marchand <[email protected]> Overall having OVS use its own CPUID checks is fine with me, I think some improvements are possible with regards to debug/logging, and potentially revisit the constructor-function vs cpu_isa_init() call, suggested improvements to implementation inline below. Regards, -Harry > Changes since v2: > - fixed compilation with AVX512, > > Changes since v1: > - fixed minor checkpatch issue, > > --- > lib/automake.mk | 2 + > lib/cpu.c | 68 ++++++++++++++++++++++++++ > lib/cpu.h | 34 +++++++++++++ > lib/dpdk-stub.c | 9 ---- > lib/dpdk.c | 52 -------------------- > lib/dpdk.h | 1 - > lib/dpif-netdev-avx512.c | 5 +- > lib/dpif-netdev-extract-avx512.c | 14 +++--- > lib/dpif-netdev-lookup-avx512-gather.c | 7 +-- > 9 files changed, 118 insertions(+), 74 deletions(-) > create mode 100644 lib/cpu.c > create mode 100644 lib/cpu.h > > diff --git a/lib/automake.mk b/lib/automake.mk > index 46f869a336..5224e08564 100644 > --- a/lib/automake.mk > +++ b/lib/automake.mk > @@ -38,6 +38,8 @@ lib_libopenvswitchavx512_la_CFLAGS = \ > -fPIC \ > $(AM_CFLAGS) > lib_libopenvswitchavx512_la_SOURCES = \ > + lib/cpu.c \ > + lib/cpu.h \ > lib/dpif-netdev-lookup-avx512-gather.c \ > lib/dpif-netdev-extract-avx512.c \ > lib/dpif-netdev-avx512.c > diff --git a/lib/cpu.c b/lib/cpu.c > new file mode 100644 > index 0000000000..ea1934d3ca > --- /dev/null > +++ b/lib/cpu.c > @@ -0,0 +1,68 @@ > +/* > + * Copyright (c) 2021, Red Hat, Inc. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#include <config.h> > + > +#include "cpu.h" > +#include "openvswitch/compiler.h" > + > +#ifdef __x86_64__ > +#include <cpuid.h> > +#include <inttypes.h> > + > +enum x86_reg { > + EAX, > + EBX, > + ECX, > + EDX, > +}; > +#define X86_LEAF_MASK 0x80000000 > +#define X86_EXT_FEATURES_LEAF 0x00000007 > +static bool x86_has_isa(uint32_t leaf, enum x86_reg reg, uint32_t bit) > +{ > + uint32_t maxleaf = __get_cpuid_max(leaf & X86_LEAF_MASK, NULL); > + uint32_t regs[4]; > + > + if (maxleaf < leaf) { > + return false; This is a programming error, not a runtime error correct? We're asking for a leaf that has not been supported in OVS. Presumably the programmer intended to ask for a feature that OVS has support for. So a unique/identifiable error return would be better than "false" which is currently ambiguous? (Currently the return value "false" means both "not supported" and "programming error") > + } > + __cpuid_count(leaf, 0, regs[EAX], regs[EBX], regs[ECX], regs[EDX]); > + return (regs[reg] & ((uint32_t) 1 << bit)) != 0; > +} > + > +static bool x86_isa[OVS_CPU_ISA_X86_LAST - OVS_CPU_ISA_X86_FIRST + 1]; > +#define X86_ISA(leaf, reg, bit, name) \ > +OVS_CONSTRUCTOR(cpu_isa_ ## name) { \ > + x86_isa[name - OVS_CPU_ISA_X86_FIRST] = x86_has_isa(leaf, reg, bit); \ > +} Instead of a constructor, can each "x86_has_isa" call just check if the init() function has been executed already? This avoids adding constructor functions, allows using VLOG, and defers doing CPU ISA checking in tools (ovs-appctl etc) which do not use the actual result. > +X86_ISA(X86_EXT_FEATURES_LEAF, EBX, 8, OVS_CPU_ISA_X86_BMI2) > +X86_ISA(X86_EXT_FEATURES_LEAF, EBX, 16, OVS_CPU_ISA_X86_AVX512F) > +X86_ISA(X86_EXT_FEATURES_LEAF, EBX, 30, OVS_CPU_ISA_X86_AVX512BW) > +X86_ISA(X86_EXT_FEATURES_LEAF, ECX, 1, OVS_CPU_ISA_X86_AVX512VBMI) > +X86_ISA(X86_EXT_FEATURES_LEAF, ECX, 14, OVS_CPU_ISA_X86_VPOPCNTDQ) > +#endif As noted above, a debug level VLOG would be helpful to identify the ISA detected by OVS when starting up. To display this clearly to the user, some form of string-ified name of the ISA would be helpful. This is currently achieved by adding a human-readable version of the ISA to the struct itself. A similar approach could work here? E.g.: X86_ISA(X86_EXT_FEATURES_LEAF, EBX, 8, OVS_CPU_ISA_X86_BMI2, "bmi2") or refactoring to add " OVS_CPU_ISA_X86_" at the MACRO level, and then use a macro to STRINGIFY() the flag if we don't want strings at all..? X86_ISA(X86_EXT_FEATURES_LEAF, EBX, 8, BMI2) > + > +bool > +cpu_has_isa(enum ovs_cpu_isa isa OVS_UNUSED) > +{ > +#ifdef __x86_64__ > + if (isa >= OVS_CPU_ISA_X86_FIRST && > + isa <= OVS_CPU_ISA_X86_LAST) { > + return x86_isa[isa - OVS_CPU_ISA_X86_FIRST]; > + } > > +#endif > + return false; > +} > diff --git a/lib/cpu.h b/lib/cpu.h > new file mode 100644 > index 0000000000..92897bb71c > --- /dev/null > +++ b/lib/cpu.h > @@ -0,0 +1,34 @@ > +/* > + * Copyright (c) 2021, Red Hat, Inc. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#ifndef CPU_H > +#define CPU_H 1 > + > +#include <stdbool.h> > + > +enum ovs_cpu_isa { > + OVS_CPU_ISA_X86_FIRST, > + OVS_CPU_ISA_X86_BMI2 = OVS_CPU_ISA_X86_FIRST, > + OVS_CPU_ISA_X86_AVX512F, > + OVS_CPU_ISA_X86_AVX512BW, > + OVS_CPU_ISA_X86_AVX512VBMI, > + OVS_CPU_ISA_X86_VPOPCNTDQ, > + OVS_CPU_ISA_X86_LAST = OVS_CPU_ISA_X86_VPOPCNTDQ, > +}; The enum counting tricks are kinda difficult to read, but it works fine. > + > +bool cpu_has_isa(enum ovs_cpu_isa); > + > +#endif /* CPU_H */ > diff --git a/lib/dpdk-stub.c b/lib/dpdk-stub.c > index fe24f9abdf..c332c217cb 100644 > --- a/lib/dpdk-stub.c > +++ b/lib/dpdk-stub.c > @@ -79,15 +79,6 @@ print_dpdk_version(void) > { > } > > -bool > -dpdk_get_cpu_has_isa(const char *arch OVS_UNUSED, > - const char *feature OVS_UNUSED) > -{ > - VLOG_DBG_ONCE("DPDK not supported in this version of Open vSwitch, " > - "cannot use CPU flag based optimizations"); > - return false; > -} Yes makes sense to remove DPDK based impl if OVS is going to carry & maintain its own. <snip removing rest of current implementation> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
