[ovs-dev] [PATCH v3 4/7] dpcls: enable cpu feature detection

[Van Haaren, Harry]

> -----Original Message-----
> From: William Tu <u9012063 at gmail.com>
> Sent: Tuesday, June 16, 2020 4:41 PM
> To: Van Haaren, Harry <harry.van.haaren at intel.com>
> Cc: ovs-dev <ovs-dev at openvswitch.org>; Stokes, Ian <ian.stokes at intel.com>;
> Ilya Maximets <i.maximets at ovn.org>; Federico Iezzi <fiezzi at redhat.com>
> Subject: Re: [PATCH v3 4/7] dpcls: enable cpu feature detection
> 
> btw, remember to add "." at the end of the commit title.
> so
> "dpcls: enable cpu feature detection."

Ah thanks, will do.

<snip patch details>
> > +    CHECK_CPU_FEATURE(feature, "avx512f", RTE_CPUFLAG_AVX512F);
> > +    CHECK_CPU_FEATURE(feature, "bmi2", RTE_CPUFLAG_BMI2);
> 
> why are "avx512f" and "bmi2" hard-coded here?
> I thought this function "dpdk_get_cpu_has_isa" allows you to check any
> cpu feature.

The strings are hard-coded here to abstract the OVS APIs from DPDK rte_cpu_flag_t.

Indeed the implementation here can be extended to check any CPU flags, but would
require some additional CHECK_CPU_FEATURE() macros. The number of ISA combinations
actually used is generally limited to < 10, so I see no issue with implementing as individual checks.

It is possible to change the API to directly use RTE_CPUFLAG_*, however I'm not sure
that's a clean API. DPDK uses an "__extension__ enum rte_cpu_flag_t" to define CPU flags,
which feels like it would better be contained in  a .c file inside OVS.

To contain the rte_cpu_flag_t in a .c I wrapped it into a "string arch, string cpu_feature" combo,
which I believe is much more flexible, and a more generic API for OVS.

/* An alternative API, but IMO it is a worse solution due to DPDK datatype in an OVS API. */
int dpdk_get_cpu_has_isa(const char *arch, enum rte_cpu_flag_t cpuflag);