On 12/3/21 16:24, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: David Marchand <[email protected]>
>> Sent: Thursday, December 2, 2021 4:05 PM
>> To: Van Haaren, Harry <[email protected]>
>> Cc: [email protected]; [email protected]; Stokes, Ian
>> <[email protected]>; Amber, Kumar <[email protected]>
>> Subject: Re: [PATCH v3] dpif-netdev: Call cpuid for x86 isa availability.
>>
>> On Tue, Nov 30, 2021 at 5:53 PM Van Haaren, Harry
>> <[email protected]> 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
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
