On 8 Sep 2021, at 17:28, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Eelco Chaudron <[email protected]>
>> Sent: Wednesday, September 8, 2021 9:16 AM
>> To: Amber, Kumar <[email protected]>
>> Cc: [email protected]; [email protected]; [email protected];
>> Stokes, Ian <[email protected]>; [email protected]; Van Haaren, Harry
>> <[email protected]>
>> Subject: Re: [PATCH v1] configure: Allow opt-in to CPU ISA opts at compile
>> time
>>
>> Not a real review of the patch, but just some comment/questions glancing over
>> the patch.
>
> Sure, thanks for input.
>
>> On 3 Sep 2021, at 15:53, Kumar Amber wrote:
>>
>>> This commit allows "opt-in" to CPU ISA optimized implementations of
>>> OVS SW datapath components at compile time. This can be useful in some
>>> deployments where the CPU ISA optimized implementation is to be chosen
>>> by default.
>
> <snip some contents>
>
>>> +Enabling all AVX512 options
>>> +---------------------------
>>> +
>>> +A user can enable all the three DPIF, Miniflow Extract and DPLCS optimized
>>> +AVX512 options at build time, if the CPU supports the required AVX512 ISA
>>> +by using the following command ::
>>> +
>>> + ./configure --enable-cpu-isa
>>
>> If we have different ISA architectures, i.e., i86 vs ARM, we are ok with a
>> single
>> option. Have you thought about AMD adding its own AMDXXX instructions in
>> addition to AVX512? How would this configuration option work? Maybe an
>> optional option to prioritize one over the other.
>
> The ISA enabling efforts have been generic so far, any reference to specific
> ISA (e.g. AVX512)
> has been solely in the implementation choice - never in a general component.
> Intention here
> is to stay in line with that - and "enable CPU ISA" seemed a logical string
> to achieve that to me..
>
> It is of course possible to provide multiple configure command lines, but I
> was hoping to avoid
> creating too many compile time flags. Typically I think projects attempt to
> avoid due to expanding
> testing & validation. A single flag would limit overhead to the minimum...
>
> Typically ISA sets have a "good - better - best" type relationship - which
> could lead to a general
> acceptance of what ISA is best. We have runtime functions to switch
> implementation - so today
> the code already enables a log of runtime/dynamic updating of implementation.
> If there's a
> need to expose that at compile time too, then that's easy to add - but comes
> with a burden in
> testing & validation...
The main reason to mention this is the inconsistent behavior across
builds/releases. With this flag being as general as it is, if someone decides
to add AVX1024, it now gets selected as the default isa function (assuming the
target was already supporting this). This is a change in behavior that happens
without any configure option change. The difference to any other general change
is that this is not a global change, but something that changes based on your
target.
Anyone else has an opinion on this? I think an alternative to this, is a proper
configuration option, which will survive a restart.
Thinking about it a bit more during my afternoon walk, I think we should
minimize (not allow) any compile-time default behavior variations. It should
all be based on the configuration, which if required to survive a reboot, be
added to the ovsdb.
>> Even if we decide a single option is right, this one to me looks more about
>> compile-time flags than enable ISA-specific code. Maybe something more in
>> line
>> with --prioritize-cpu-isa-functions? Or some other catchy name.
>
> Sure - that string is fine for me too.
>
> <snip patch contents>
>
> Regards, -Harry
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
