[ovs-dev] [PATCH v4 1/7] dpif-netdev: implement subtable lookup validation.
Van Haaren, Harry
harry.van.haaren at intel.com
Tue Jun 30 10:00:36 UTC 2020
> -----Original Message-----
> From: William Tu <u9012063 at gmail.com>
> Sent: Saturday, June 27, 2020 7:27 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 v4 1/7] dpif-netdev: implement subtable lookup validation.
> On Thu, Jun 18, 2020 at 9:53 AM Harry van Haaren
> <harry.van.haaren at intel.com> 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.haaren at intel.com>
> > ---
> > v4:
> > - Fix automake .c file order (William Tu)
> > - Fix include file ordering: netdev-lookup before netdev-perf (William Tu)
> > - Fix Typos (William Tu)
> > - Add --enable-autovalidator compile time flag to set the DPCLS Autovalidator
> > to the highest priority by default. This is required to run the unit tests
> > with all DPCLS implementations without changing every test-case.
> > Backwards compatibility is kept with OVS 2.13.
> Why adding this option at compile time?
> To run the unit test, we can simply use the set-prio command to set
> the autovalidator to the highest before starting the unit test, right?
As you noted later in the patchset (https://mail.openvswitch.org/pipermail/ovs-dev/2020-June/372114.html)
the goal is to enable autovalidator *by default*. This is not the default
behavior we want in packaged versions of OVS, only for DPCLS test builds.
By making this compile-time instead of runtime, it ensures any test that
exists, or is developed in future can be tested with the autovalidator without
any extra manual effort to enable autovalidation.
> > - Add check to ensure autovalidator is 0th item in lookup array
> What's your concern here? Users can only change the prio, not
> the .probe function. So autovalidator is always at 0th item.
> If you worry about code being changed accidentally in the future,
> how about using BUILD_ASSERT_DECL?
Yes - good improvement.
> The rest looks good to me.
Thanks for review.
More information about the dev