> -----Original Message-----
> From: Eelco Chaudron <[email protected]>
> Sent: Thursday 2 June 2022 15:15
> To: Finn, Emma <[email protected]>
> Cc: Van Haaren, Harry <[email protected]>; Amber, Kumar
> <[email protected]>; Stokes, Ian <[email protected]>;
> [email protected]
> Subject: Re: [v6 04/11] odp-execute: Add auto validation function for
> actions.
>
> On 10 May 2022, at 16:21, Emma Finn wrote:
>
> > This commit introduced the auto-validation function which allows users
> > to compare the batch of packets obtained from different action
> > implementations against the linear action implementation.
> >
> > The autovalidator function can be triggered at runtime using the
> > following command:
> >
> > $ ovs-appctl dpif-netdev/action-impl-set autovalidator
> >
> > Signed-off-by: Emma Finn <[email protected]>
> > Acked-by: Harry van Haaren <[email protected]>
> > ---
> > NEWS | 2 +
> > lib/dp-packet.c | 23 +++++++++
> > lib/dp-packet.h | 4 ++
> > lib/odp-execute-private.c | 99
> > +++++++++++++++++++++++++++++++++++++++
> > lib/odp-execute-private.h | 3 ++
> > 5 files changed, 131 insertions(+)
> >
<snip>
> > +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>
> Don't think we need this, see below.
>
> Autovalidator should be used in test cases, so we should not rate limit it.
I think it is best to avoid spamming CI log files to huge sizes.
> Don’t thinks we need the leading “\n”, also we do not know which
> implementation failed!
>
> > + ds_cstr(&log_msg));
>
> Do we need to free/re-init log_msg?
>
> We also need to reset failed, failed = false.
>
> > + }
> > + }
> > + dp_packet_delete_batch(&test_batch, 1);
> > + }
> > + dp_packet_delete_batch(&good_batch, 1);
> > +
> > + /* Apply the action to the original batch for continued
> > + processing. */
>
> Now we execute the scaler twice is there another way?
>
Not a clean way that I can think of.
> > + scalar->funcs[attr_type](NULL, batch, a, should_steal); }
> > +
> > +int32_t
> > +action_autoval_init(struct odp_execute_action_impl *self) {
> > + self->funcs[OVS_ACTION_ATTR_POP_VLAN] = action_autoval_generic;
> > +
>
> We should not initialize only the known function(s), we should assign the
> action_autoval_generic function to all possible actions
> (__OVS_ACTION_ATTR_MAX), and only skip the ones that would forward
> the packets ;)
>
> I think in general we need to be more clear that the function pointer
> implementation is only for packet transform operations, and we should
> identify which ones those are (add this to the odp_execute_action_impl
> definition).
>
> You can have the action_autoval_generic() skip the test if the two function
> pointers are the same?
>
What is the core problem to solve here?
I like the simplicity of setting the function pointer for the optimized
routines. Essentially, this manually selects autovalidation for the optimized
action variants.
A blanket "autovalidate all" approach, and then selectively disabling specific
actions feels "backwards" in finding out which actions do
transmit/have-side-effects.
I prefer the current approach of enabling func-ptr validation where it is
required, and limiting change to only the area where it is known to be required.
> > + return 0;
> > +}
> > diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
> > index 869478ce9..fed20930d 100644
> > --- a/lib/odp-execute-private.h
> > +++ b/lib/odp-execute-private.h
> > @@ -66,6 +66,7 @@ struct odp_execute_action_impl {
> > /* Order of Actions implementations. */ enum
> > odp_execute_action_impl_idx {
> > ACTION_IMPL_SCALAR,
> > + ACTION_IMPL_AUTOVALIDATOR,
> > /* See ACTION_IMPL_BEGIN below, for "first to-be-validated" impl.
> > * Do not change the autovalidator position in this list without
> updating
> > * the define below.
> > @@ -76,6 +77,8 @@ enum odp_execute_action_impl_idx {
> >
> > /* Index to start verifying implementations from. */
> > BUILD_ASSERT_DECL(ACTION_IMPL_SCALAR == 0);
> > +BUILD_ASSERT_DECL(ACTION_IMPL_AUTOVALIDATOR == 1);
>
> Add cr/lf
>
> > +#define ACTION_IMPL_BEGIN (ACTION_IMPL_AUTOVALIDATOR + 1)
> >
> > /* Odp execute init handles setting up the state of the actions functions
> at
> > * initialization time. It cannot return errors, as it must always
> > succeed in
> > --
> > 2.25.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
