> -----Original Message-----
> From: Stokes, Ian <ian.sto...@intel.com>
> Sent: Thursday, July 9, 2020 2:21 PM
> To: Van Haaren, Harry <harry.van.haa...@intel.com>; ovs-dev@openvswitch.org
> Cc: i.maxim...@ovn.org; u9012...@gmail.com; fie...@redhat.com
> Subject: Re: [PATCH v6 1/6] dpif-netdev: implement subtable lookup validation.
>
>
>
> On 7/2/2020 6:42 PM, Harry van Haaren wrote:
> > This commit refactors the existing dpif subtable function pointer
> > infrastructure, and implements an autovalidator component.
> >
> > The refactoring of the existing dpcls subtable lookup function
> > handling, making it more generic, and cleaning up how to enable
> > more implementations in future.
> >
> > In order to ensure all implementations provide identical results,
> > the autovalidator is added. The autovalidator itself implements
> > the subtable lookup function prototype, but internally iterates
> > over all other available implementations. The end result is that
> > testing of each implementation becomes automatic, when the auto-
> > validator implementation is selected.
> >
> > Signed-off-by: Harry van Haaren <harry.van.haa...@intel.com>
>
> Thasnk for this Harry,
>
> a few comments below, more for discussion.
>
> Theres a few minor typos and style fixes required, nothing major that
> cannot be fixed on commit so no need for revision.
Fixed in v7.
<snip details>
> > +VLOG_DEFINE_THIS_MODULE(dpif_lookup_autovalidator);
> > +
> > +/* This file implements an automated validator for subtable search
> > + * implementations. It compares the results of the generic scalar search
> > result
> > + * with ISA optimized implementations.
> > + *
> > + * Note the goal is *NOT* to test the *specialized* versions of subtables,
> > as
> > + * the compiler performs the specialization - and we rely on the
> > correctness of
> > + * the compiler to not break those specialized variantes.
>
> So if we depend on the compiler for correctness, should we provide a
> list of known "Correct compilers" that this feature was tested with? I
> guess there could be a case where a compiler has a bug and as such may
> break the feature?
Compilers must be assumed to be correct. There is no value in attempting to
provide
a list of compilers here - optimizing compilers do constant-propagation all the
time,
and resulting generated code must be correct: the same rules apply here.
> Minor typo, variants, can be done on commit.
Fixed in v7.
> > + * The goal is to ensure identical results of the different
> > implementations,
> > + * despite that the implementations may have different methods to get those
> > + * results.
> > + *
> > + * Example: AVX-512 ISA uses different instructions and algorithm to the
> > scalar
> > + * implementation, however the results (rules[] output) must be the same.
> > + */
> > +
> > +dpcls_subtable_lookup_func
> > +dpcls_subtable_autovalidator_probe(uint32_t u0 OVS_UNUSED,
> > + uint32_t u1 OVS_UNUSED);
> > +
> > +static uint32_t
> > +dpcls_subtable_autovalidator(struct dpcls_subtable *subtable,
> > + uint32_t keys_map,
> > + const struct netdev_flow_key *keys[],
> > + struct dpcls_rule **rules_good)
> > +{
> > + const uint32_t u0_bit_count = subtable->mf_bits_set_unit0;
> > + const uint32_t u1_bit_count = subtable->mf_bits_set_unit1;
> > +
> > + /* Scalar generic - the "known correct" version */
> Minor, for this and all other comments missing period. Fairly minor so
> can be fixed on commit.
Fixing along the way in v7.
<snip details>
> > /* Generic lookup function that uses runtime provided mf bits for
> > iterating. */
> > -uint32_t
> > +static uint32_t
> > dpcls_subtable_lookup_generic(struct dpcls_subtable *subtable,
> > uint32_t keys_map,
> > const struct netdev_flow_key *keys[],
> > @@ -310,6 +311,10 @@ dpcls_subtable_generic_probe(uint32_t u0_bits,
> uint32_t u1_bits)
> > if (f) {
> > VLOG_DBG("Subtable using Generic Optimized for u0 %d, u1 %d\n",
> > u0_bits, u1_bits);
> > + } else {
> > + /* Always return the generic function */
> > + f = dpcls_subtable_lookup_generic;
>
> I had always assumed that the generic lookup would be selected anyway if
> the optimized scalar lookup wasn't available prioir to this patch, maybe
> I missed something as regards why you have to sepcify it here?
As per your below comment, scalar must always return a valid pointer.
<snip lots of patch>
> > - /* Probe for a specialized generic lookup function. */
> > - subtable->lookup_func = dpcls_subtable_generic_probe(unit0, unit1);
> > -
> > - /* If not set, assign generic lookup. Generic works for any miniflow.
> > */
> > - if (!subtable->lookup_func) {
> > - subtable->lookup_func = dpcls_subtable_lookup_generic;
> > - }
> Ah, ignore the comment above, this is where the confusion was coming
> from on my part, generic as selected here previously.
>
>
> Completed testing on this patch as well LGTM.
Thanks, -Harry
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
