On 13 Sep 2021, at 16:36, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Eelco Chaudron <echau...@redhat.com>
>> Sent: Friday, September 10, 2021 3:41 PM
>> To: Van Haaren, Harry <harry.van.haa...@intel.com>; i.maxim...@ovn.org;
>> Stokes, Ian <ian.sto...@intel.com>; f...@sysclose.org
>> Cc: Amber, Kumar <kumar.am...@intel.com>; ovs-dev@openvswitch.org;
>> ktray...@redhat.com
>> Subject: Re: [PATCH v1] configure: Allow opt-in to CPU ISA opts at compile
>> time
>>
>>
>>
>> On 8 Sep 2021, at 17:28, Van Haaren, Harry wrote:
>>
>>>> -----Original Message-----
>>>> From: Eelco Chaudron <echau...@redhat.com>
>>>> Sent: Wednesday, September 8, 2021 9:16 AM
>>>> To: Amber, Kumar <kumar.am...@intel.com>
>>>> Cc: ovs-dev@openvswitch.org; ktray...@redhat.com; i.maxim...@ovn.org;
>>>> Stokes, Ian <ian.sto...@intel.com>; f...@sysclose.org; Van Haaren, Harry
>>>> <harry.van.haa...@intel.com>
>>>> 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.
>
> Note that the default setting for this option is suggested as "off", meaning
> this is an entirely
> *opt in* strategy, to allow people to deploy OVS and automatically benefit
> from CPU ISA.
>
> To be more specific, this feature is a request from folks who intend to
> deploy with CPU ISA
> enabled by default - it suited their CI/CD/QA tooling to have this enabled by
> default compile
> time switch to ease validation as the CPU ISA will get picked up
> automatically when available.
>
> Note that it is not a "change in behaviour", because functionally its
> identical.
> (The fuzzing, autovalidation & unit tests are there to ensure it is
> functionally identical).
> It correct that there is a change in the default *implementation* of the
> functionality,
> which I think you meant (just clarifying the "change in behaviour" as not
> being "functional behaviour",
> only "implementation of behaviour")
>> 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.
Thinking about this more, I do not feel like we should introduce compile-time
default configuration options.
It should just be added to the ovsdb and picked up from there on startup. Same
as for configuring the PMD cores/queues, etc. etc.
Would be good to get some input from others (maintainers) on this?!
>>>> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
