| Message ID | 20211130145047.5000-1-david.marchand@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v3] dpif-netdev: Call cpuid for x86 isa availability. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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 <david.marchand@redhat.com> > Sent: Tuesday, November 30, 2021 2:51 PM > To: dev@openvswitch.org > Cc: i.maximets@ovn.org; Stokes, Ian <ian.stokes@intel.com>; Amber, Kumar > <kumar.amber@intel.com>; Van Haaren, Harry <harry.van.haaren@intel.com> > 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 <i.maximets@ovn.org> > Signed-off-by: David Marchand <david.marchand@redhat.com> 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>
On Tue, Nov 30, 2021 at 5:53 PM Van Haaren, Harry <harry.van.haaren@intel.com> wrote: > > 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!) I can look at adding logs in dpif as a preparation patch. The current situation where we have logs at init is not user friendly: for a user that wants to use feature X and enters the right ovs-*ctl commands, it is hard to make the relation with "cpu has feature Y" in OVS logs that could be days old. Moving this code in some init function will add an init order dependency. This detection code is really small, stateless and can be done as early as possible. Otoh, with constructors, components in OVS can get them without caring if init was run (let's say some day we need to know about those features in utils). > > + > > +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 A ovs_assert() is better in this case.
> -----Original Message----- > From: David Marchand <david.marchand@redhat.com> > Sent: Thursday, December 2, 2021 4:05 PM > To: Van Haaren, Harry <harry.van.haaren@intel.com> > Cc: dev@openvswitch.org; i.maximets@ovn.org; Stokes, Ian > <ian.stokes@intel.com>; Amber, Kumar <kumar.amber@intel.com> > Subject: Re: [PATCH v3] dpif-netdev: Call cpuid for x86 isa availability. > > On Tue, Nov 30, 2021 at 5:53 PM Van Haaren, Harry > <harry.van.haaren@intel.com> wrote: > > > 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!) > > I can look at adding logs in dpif as a preparation patch. > The current situation where we have logs at init is not user friendly: > for a user that wants to use feature X and enters the right ovs-*ctl > commands, it is hard to make the relation with "cpu has feature Y" in > OVS logs that could be days old. Good point. The previously proposed solution to only run CPUID instruction and decode available ISA handles that very nicely. Suggestion in pseudo-code below: static int cpuid_check_done; void do_cpuid_check() { /* execute cpuid, run through all OVS-known ISA, VLOG() and store results. */ cpuid_check_done = 1; } int check_isa( isa_type ) { if (OVS_UNLIKELY( ! cpuid_check_done )) { do_cpuid_check() } return isa_available[isa_type]; } Benefits; 1) When user executes command to use ISA, ISA checks are executed, VLOG executes too 2) OVS-appctl (etc) which do not call "check_isa()" will not run CPUID, and not VLOG either. 3) Lack of constructor functions eases portability (DPDK & Windows has issues with constructors IIUC) > Moving this code in some init function will add an init order dependency. No order dependency required, see the above. First to call will do the required work. > This detection code is really small, stateless and can be done as > early as possible. > Otoh, with constructors, components in OVS can get them without caring > if init was run (let's say some day we need to know about those > features in utils). > > > > > + > > > +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 > > A ovs_assert() is better in this case. Yes agree. > -- > David Marchand Thanks, -Harry
On 12/3/21 16:24, Van Haaren, Harry wrote: >> -----Original Message----- >> From: David Marchand <david.marchand@redhat.com> >> Sent: Thursday, December 2, 2021 4:05 PM >> To: Van Haaren, Harry <harry.van.haaren@intel.com> >> Cc: dev@openvswitch.org; i.maximets@ovn.org; Stokes, Ian >> <ian.stokes@intel.com>; Amber, Kumar <kumar.amber@intel.com> >> Subject: Re: [PATCH v3] dpif-netdev: Call cpuid for x86 isa availability. >> >> On Tue, Nov 30, 2021 at 5:53 PM Van Haaren, Harry >> <harry.van.haaren@intel.com> wrote: >>>> 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!) >> >> I can look at adding logs in dpif as a preparation patch. >> The current situation where we have logs at init is not user friendly: >> for a user that wants to use feature X and enters the right ovs-*ctl >> commands, it is hard to make the relation with "cpu has feature Y" in >> OVS logs that could be days old. > > Good point. The previously proposed solution to only run CPUID instruction and decode > available ISA handles that very nicely. Suggestion in pseudo-code below: > > static int cpuid_check_done; > > void do_cpuid_check() { > /* execute cpuid, run through all OVS-known ISA, VLOG() and store results. */ > cpuid_check_done = 1; > } > > int check_isa( isa_type ) { > if (OVS_UNLIKELY( ! cpuid_check_done )) { > do_cpuid_check() > } > > return isa_available[isa_type]; > } > > Benefits; > 1) When user executes command to use ISA, ISA checks are executed, VLOG executes too I don't understand the point of debugging the CPUID itself. If you want to know the capabilities of the current CPU, just look at /proc/cpuinfo. CPUID values are rock-solid and will, likely, never change. So, once written and verified during development this code is very unlikely to be changed. So, why do we need to complicate the implementation with dynamic initialization? In the future we may want to check some things dynamically on a hot path and extra logic in these functions will not be welcome in that case. _From a user's standpoint it's almost impossible to know which CPU capabilities are required for a certain feature to work (bmi2 who?). It's not documented. If you really want users to understand why something is not available, the better way would be to replace these fairly painful to read 'available: not available' messages with some meaningful 'not available, because ...', showing which CPU capability is missing. Comparing these to the /proc/cpuinfo will clearly say if there is a bug in CPUID handling without need to print anything from the cpu.c. > 2) OVS-appctl (etc) which do not call "check_isa()" will not run CPUID, and not VLOG either. This is not a problem if there are no logs. :) > 3) Lack of constructor functions eases portability (DPDK & Windows has issues with constructors IIUC) OVS uses constructors for coverage counters, logs, and for some sockets. The last one is actually exclusively on Windows. And that never was a problem, AFAIK. Best regards, Ilya Maximets. > > >> Moving this code in some init function will add an init order dependency. > > No order dependency required, see the above. First to call will do the required work. > >> This detection code is really small, stateless and can be done as >> early as possible. >> Otoh, with constructors, components in OVS can get them without caring >> if init was run (let's say some day we need to know about those >> features in utils). >> >> >>>> + >>>> +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 >> >> A ovs_assert() is better in this case. > > Yes agree. > >> -- >> David Marchand > > Thanks, -Harry >
On Tue, Dec 7, 2021 at 12:30 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > 3) Lack of constructor functions eases portability (DPDK & Windows has issues with constructors IIUC) > OVS uses constructors for coverage counters, logs, and for > some sockets. The last one is actually exclusively on > Windows. And that never was a problem, AFAIK. +1 I'll keep the current implementation, but respin with an ovs_assert() where needed as agreed previously. More high level logs might be added in dpif in the future. Thanks.
> -----Original Message----- > From: David Marchand <david.marchand@redhat.com> > Sent: Wednesday, December 15, 2021 3:55 PM > To: Ilya Maximets <i.maximets@ovn.org> > Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@openvswitch.org; > Stokes, Ian <ian.stokes@intel.com>; Amber, Kumar <kumar.amber@intel.com> > Subject: Re: [PATCH v3] dpif-netdev: Call cpuid for x86 isa availability. > > On Tue, Dec 7, 2021 at 12:30 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > > 3) Lack of constructor functions eases portability (DPDK & Windows has issues > with constructors IIUC) > > OVS uses constructors for coverage counters, logs, and for > > some sockets. The last one is actually exclusively on > > Windows. And that never was a problem, AFAIK. > > +1 > > > I'll keep the current implementation, but respin with an ovs_assert() > where needed as agreed previously. > More high level logs might be added in dpif in the future. Hi Ilya & David, Sorry, I'm not following what we are trying to achieve here. My understanding as follows; - David sends patch to change the CPU detection to be in OVS, using CPUID impl (~ like DPDK) - I raised concerns, suggesting an alternative implementation still using CPUID but better logging - Ilya raises concerns around using CPUID "as a whole", and notes that constructors on Windows are OK - [About a week goes by, I was waiting for technical input/response from David] - David suggests "OK, will keep current implementation" (with minor modifications for assert) What about the outstanding reviews & community input? I took the time to review, propose an alternative impl, write the pseudo-code to assist, and listed benefits of that implementation. Why is this technical input not being taken into account? (pasted again below the line for reference) Equally, we should decide on what to do about using /proc/cpuinfo, as per Ilya's suggestion. Personally, I think it a different approach, with perhaps other caveats that I don't know. I like tried-and-tested solutions, so am comfortable with moving in "small steps" and changing from DPDK based implementation to OVS-with-CPUID implementation. Changing to totally different approach would require more effort, and I don't have much time for that at the moment. Regards, -Harry
On Wed, Dec 15, 2021 at 5:37 PM Van Haaren, Harry <harry.van.haaren@intel.com> wrote: > > I'll keep the current implementation, but respin with an ovs_assert() > > where needed as agreed previously. > > More high level logs might be added in dpif in the future. > > Hi Ilya & David, > > Sorry, I'm not following what we are trying to achieve here. My understanding as follows; > - David sends patch to change the CPU detection to be in OVS, using CPUID impl (~ like DPDK) > - I raised concerns, suggesting an alternative implementation still using CPUID but better logging > - Ilya raises concerns around using CPUID "as a whole", and notes that constructors on Windows are OK > - [About a week goes by, I was waiting for technical input/response from David] > - David suggests "OK, will keep current implementation" (with minor modifications for assert) Ilya comments made sense, I was expeecting replies from your side but seen none. So I understood this as you were ok with Ilya comments, so I only updated the parts we agreed on previously.
On 12/15/21 17:33, Van Haaren, Harry wrote: >> -----Original Message----- >> From: David Marchand <david.marchand@redhat.com> >> Sent: Wednesday, December 15, 2021 3:55 PM >> To: Ilya Maximets <i.maximets@ovn.org> >> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; dev@openvswitch.org; >> Stokes, Ian <ian.stokes@intel.com>; Amber, Kumar <kumar.amber@intel.com> >> Subject: Re: [PATCH v3] dpif-netdev: Call cpuid for x86 isa availability. >> >> On Tue, Dec 7, 2021 at 12:30 AM Ilya Maximets <i.maximets@ovn.org> wrote: >>>> 3) Lack of constructor functions eases portability (DPDK & Windows has issues >> with constructors IIUC) >>> OVS uses constructors for coverage counters, logs, and for >>> some sockets. The last one is actually exclusively on >>> Windows. And that never was a problem, AFAIK. >> >> +1 >> >> >> I'll keep the current implementation, but respin with an ovs_assert() >> where needed as agreed previously. >> More high level logs might be added in dpif in the future. > > Hi Ilya & David, > > Sorry, I'm not following what we are trying to achieve here. My understanding as follows; > - David sends patch to change the CPU detection to be in OVS, using CPUID impl (~ like DPDK) > - I raised concerns, suggesting an alternative implementation still using CPUID but better logging > - Ilya raises concerns around using CPUID "as a whole", and notes that constructors on Windows are OK I don't think I raised any concerns about the current approach in this patch. My goal was to defend it instead. My key points was: 1. 'I don't understand the point of debugging the CPUID itself.' This means, I don't think we need logs, as there are other ways to get this information. 2. There is no clear relation between CPU capabilities and optimized implementations, so user may not understand how these are connected. Again, having logs is a low value thing. 3. /proc/cpuinfo wasn't about checking from the code, but about checking by user's hands to verify what capabilities are there. That will be identical to checking the logs in most cases, hence logs are not needed again. 4. OVS uses constructors everywhere without any problems. And constructors are used on Windows even more than on Linux. So, that should not be a problem. 6. regarding your solution I had pointed out a few downsides of it, e.g.. slower work if the check will ever be needed on a hot path in the future. Best regards, Ilya Maximets.
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; + } + __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); \ +} +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 + +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, +}; + +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; -} - void dpdk_status(const struct ovsrec_open_vswitch *cfg) { diff --git a/lib/dpdk.c b/lib/dpdk.c index b2ef31cd20..fe201bf29c 100644 --- a/lib/dpdk.c +++ b/lib/dpdk.c @@ -585,58 +585,6 @@ print_dpdk_version(void) puts(rte_version()); } -/* Avoid calling rte_cpu_get_flag_enabled() excessively, by caching the - * result of the call for each CPU flag in a static variable. To avoid - * allocating large numbers of static variables, use a uint8 as a bitfield. - * Note the macro must only return if the ISA check is done and available. - */ -#define ISA_CHECK_DONE_BIT (1 << 0) -#define ISA_AVAILABLE_BIT (1 << 1) - -#define CHECK_CPU_FEATURE(feature, name_str, RTE_CPUFLAG) \ - do { \ - if (strncmp(feature, name_str, strlen(name_str)) == 0) { \ - static uint8_t isa_check_##RTE_CPUFLAG; \ - int check = isa_check_##RTE_CPUFLAG & ISA_CHECK_DONE_BIT; \ - if (OVS_UNLIKELY(!check)) { \ - int has_isa = rte_cpu_get_flag_enabled(RTE_CPUFLAG); \ - VLOG_DBG("CPU flag %s, available %s\n", \ - name_str, has_isa ? "yes" : "no"); \ - isa_check_##RTE_CPUFLAG = ISA_CHECK_DONE_BIT; \ - if (has_isa) { \ - isa_check_##RTE_CPUFLAG |= ISA_AVAILABLE_BIT; \ - } \ - } \ - if (isa_check_##RTE_CPUFLAG & ISA_AVAILABLE_BIT) { \ - return true; \ - } else { \ - return false; \ - } \ - } \ - } while (0) - -bool -dpdk_get_cpu_has_isa(const char *arch, const char *feature) -{ - /* Ensure Arch is x86_64. */ - if (strncmp(arch, "x86_64", 6) != 0) { - return false; - } - -#if __x86_64__ - /* CPU flags only defined for the architecture that support it. */ - CHECK_CPU_FEATURE(feature, "avx512f", RTE_CPUFLAG_AVX512F); - CHECK_CPU_FEATURE(feature, "avx512bw", RTE_CPUFLAG_AVX512BW); - CHECK_CPU_FEATURE(feature, "avx512vbmi", RTE_CPUFLAG_AVX512VBMI); - CHECK_CPU_FEATURE(feature, "avx512vpopcntdq", RTE_CPUFLAG_AVX512VPOPCNTDQ); - CHECK_CPU_FEATURE(feature, "bmi2", RTE_CPUFLAG_BMI2); -#endif - - VLOG_WARN("Unknown CPU arch,feature: %s,%s. Returning not supported.\n", - arch, feature); - return false; -} - void dpdk_status(const struct ovsrec_open_vswitch *cfg) { diff --git a/lib/dpdk.h b/lib/dpdk.h index 445a51d065..2eb1aedbb0 100644 --- a/lib/dpdk.h +++ b/lib/dpdk.h @@ -44,6 +44,5 @@ bool dpdk_per_port_memory(void); bool dpdk_available(void); void print_dpdk_version(void); void dpdk_status(const struct ovsrec_open_vswitch *); -bool dpdk_get_cpu_has_isa(const char *arch, const char *feature); #endif /* dpdk.h */ diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c index 544d36903e..3f1e623f36 100644 --- a/lib/dpif-netdev-avx512.c +++ b/lib/dpif-netdev-avx512.c @@ -20,6 +20,7 @@ #include <config.h> +#include "cpu.h" #include "dpif-netdev.h" #include "dpif-netdev-perf.h" #include "dpif-netdev-private.h" @@ -61,8 +62,8 @@ struct dpif_userdata { int32_t dp_netdev_input_outer_avx512_probe(void) { - bool avx512f_available = dpdk_get_cpu_has_isa("x86_64", "avx512f"); - bool bmi2_available = dpdk_get_cpu_has_isa("x86_64", "bmi2"); + bool avx512f_available = cpu_has_isa(OVS_CPU_ISA_X86_AVX512F); + bool bmi2_available = cpu_has_isa(OVS_CPU_ISA_X86_BMI2); if (!avx512f_available || !bmi2_available) { return -ENOTSUP; diff --git a/lib/dpif-netdev-extract-avx512.c b/lib/dpif-netdev-extract-avx512.c index ec64419e38..feafba0c32 100644 --- a/lib/dpif-netdev-extract-avx512.c +++ b/lib/dpif-netdev-extract-avx512.c @@ -42,8 +42,8 @@ #include <stdint.h> #include <string.h> +#include "cpu.h" #include "flow.h" -#include "dpdk.h" #include "dpif-netdev-private-dpcls.h" #include "dpif-netdev-private-extract.h" @@ -589,21 +589,21 @@ DECLARE_MFEX_FUNC(dot1q_ip_tcp, PROFILE_ETH_VLAN_IPV4_TCP) static int32_t avx512_isa_probe(uint32_t needs_vbmi) { - static const char *isa_required[] = { - "avx512f", - "avx512bw", - "bmi2", + static enum ovs_cpu_isa isa_required[] = { + OVS_CPU_ISA_X86_AVX512F, + OVS_CPU_ISA_X86_AVX512BW, + OVS_CPU_ISA_X86_BMI2, }; int32_t ret = 0; for (uint32_t i = 0; i < ARRAY_SIZE(isa_required); i++) { - if (!dpdk_get_cpu_has_isa("x86_64", isa_required[i])) { + if (!cpu_has_isa(isa_required[i])) { ret = -ENOTSUP; } } if (needs_vbmi) { - if (!dpdk_get_cpu_has_isa("x86_64", "avx512vbmi")) { + if (!cpu_has_isa(OVS_CPU_ISA_X86_AVX512VBMI)) { ret = -ENOTSUP; } } diff --git a/lib/dpif-netdev-lookup-avx512-gather.c b/lib/dpif-netdev-lookup-avx512-gather.c index 072831e96a..914c9e2ede 100644 --- a/lib/dpif-netdev-lookup-avx512-gather.c +++ b/lib/dpif-netdev-lookup-avx512-gather.c @@ -19,6 +19,7 @@ #include <config.h> +#include "cpu.h" #include "dpif-netdev.h" #include "dpif-netdev-lookup.h" #include "cmap.h" @@ -398,13 +399,13 @@ dpcls_subtable_avx512_gather_probe(uint32_t u0_bits, uint32_t u1_bits) { dpcls_subtable_lookup_func f = NULL; - int avx512f_available = dpdk_get_cpu_has_isa("x86_64", "avx512f"); - int bmi2_available = dpdk_get_cpu_has_isa("x86_64", "bmi2"); + int avx512f_available = cpu_has_isa(OVS_CPU_ISA_X86_AVX512F); + int bmi2_available = cpu_has_isa(OVS_CPU_ISA_X86_BMI2); if (!avx512f_available || !bmi2_available) { return NULL; } - int use_vpop = dpdk_get_cpu_has_isa("x86_64", "avx512vpopcntdq"); + int use_vpop = cpu_has_isa(OVS_CPU_ISA_X86_VPOPCNTDQ); CHECK_LOOKUP_FUNCTION(9, 4, use_vpop); CHECK_LOOKUP_FUNCTION(9, 1, use_vpop);
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. 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. Suggested-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: David Marchand <david.marchand@redhat.com> --- 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