> -----Original Message-----
> From: Eelco Chaudron <[email protected]>
> Sent: Thursday 2 June 2022 15:06
> To: Finn, Emma <[email protected]>
> Cc: Van Haaren, Harry <[email protected]>; Amber, Kumar
> <[email protected]>; Stokes, Ian <[email protected]>;
> [email protected]
> Subject: Re: [v6 03/11] odp-execute: Add function pointer for pop_vlan
> action.
>
> On 10 May 2022, at 16:21, Emma Finn wrote:
>
> > This commit removes the pop_vlan action from the large switch and
> > creates a separate function for batched processing. A function pointer
> > is also added to call the new batched function for the pop_vlan
> > action.
> >
> > Signed-off-by: Emma Finn <[email protected]>
> > Acked-by: Harry van Haaren <[email protected]>
> > ---
> > lib/odp-execute-private.c | 19 +++++++++++++++++-
> > lib/odp-execute.c | 41 +++++++++++++++++++++++++++++++++-----
> -
> > lib/odp-execute.h | 2 ++
> > 3 files changed, 55 insertions(+), 7 deletions(-)
> >
<snip>
> int
>
> > +odp_execute_action_set(const char *name,
> > + struct odp_execute_action_impl *active) {
> > + uint32_t i;
> > + for (i = 0; i < ACTION_IMPL_MAX; i++) {
> > + /* String compare, and set ptrs atomically. */
> > + if (strcmp(action_impls[i].name, name) == 0) {
>
> OVS style seems to be (!strcmp())
>
> > + action_impl_copy_funcs(active, &action_impls[i]);
> > + active_action_impl_index = i;
>
> Why do we copy the content!? Would it be way easier to just return the
> pointer to the struct here, and use it in odp-execute.c.
>
> Something like:
>
> ATOMIC(static struct odp_execute_action_impl *actions_active_impl);
> impl = odp_execute_action_GET(name)
> if (impl)
> atomic_store_relaxed(actions_active_impl, impl)
>
> With this, you might need a call into odp-execute.c to get the current
> configured value. But it would make the implementation more
> straightforward.
>
This rework isn't included in v7. What is the concern with the current approach?
An atomic store of the entire impls struct is not possible. Can you provide a
better explanation as to what you want here?
> > + return 0;
>
> Maybe we should add a log on the successful change of the
> implementation, so we can see this in the log?
Log for this is already in place -
2022-06-10T08:55:25.047Z|00189|unixctl|DBG|replying with success, id=0: "Action
implementation set to avx512."
>
> > + }
> > + }
> > + return -1;
>
> return -EINVAL
>
> > +}
> > +
> > void
> > odp_execute_action_init(void)
> > {
> > diff --git a/lib/odp-execute.c b/lib/odp-execute.c index
> > 165386e66..c2be74454 100644
> > --- a/lib/odp-execute.c
> > +++ b/lib/odp-execute.c
> > @@ -834,6 +834,28 @@ requires_datapath_assistance(const struct
> nlattr *a)
> > return false;
> > }
> >
> > +static void
> > +action_pop_vlan(void *dp OVS_UNUSED, struct dp_packet_batch
> *batch,
> > + const struct nlattr *a OVS_UNUSED,
> > + bool should_steal OVS_UNUSED) {
> > + struct dp_packet *packet;
>
> Add a line break here.
>
> > + DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
> > + eth_pop_vlan(packet);
> > + }
> > +}
>
> Why was this implementation moved to a function? If it's because the
> lookup is faster than running the switch() if this is the case, should all be
> moved? Asking as other more general function would be called more often.
>
> It would also avoid problems where people will add a SIMD-specific
> implementation but do not at the scalar version, this would fail to be
> checked by the autovalidator.
>
> My personal preference would be to move all handing to the scaler version
> in one patch, and rip out the existing switch() existing code ;)
>
Yes, ideally all actions are in self-contained functions that process a burst
of packets internally.
A scalar implementation *must* be provided for any given action. OVS Community
should not accept "simd only" actions, as you pointed out scalar impl is
required for autovalidation.
Changing all the actions code at once would be a very large change, the
approach taken here is to incrementally improve each action as we do a SIMD
optimized version.
Scalar actions can always be converted to the new function style later. For
2.18, the current actions are being incrementally updated.
<snip>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
