[ovs-dev,v6,03/11] odp-execute: Add function pointer for pop_vlan action.

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 <emma.finn@intel.com>
Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 lib/odp-execute-private.c | 19 +++++++++++++++++-
 lib/odp-execute.c         | 41 +++++++++++++++++++++++++++++++++------
 lib/odp-execute.h         |  2 ++
 3 files changed, 55 insertions(+), 7 deletions(-)

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 <emma.finn@intel.com>
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  lib/odp-execute-private.c | 19 +++++++++++++++++-
>  lib/odp-execute.c         | 41 +++++++++++++++++++++++++++++++++------
>  lib/odp-execute.h         |  2 ++
>  3 files changed, 55 insertions(+), 7 deletions(-)
>
> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
> index b3a02745c..996de0bf6 100644
> --- a/lib/odp-execute-private.c
> +++ b/lib/odp-execute-private.c
> @@ -29,13 +29,14 @@
>
>  int32_t action_autoval_init(struct odp_execute_action_impl *self);
>  VLOG_DEFINE_THIS_MODULE(odp_execute_private);
> +static uint32_t active_action_impl_index;
>
>  static struct odp_execute_action_impl action_impls[] = {
>      [ACTION_IMPL_SCALAR] = {
>          .available = 1,
>          .name = "scalar",
>          .probe = NULL,
> -        .init_func = NULL,
> +        .init_func = odp_action_scalar_init,
>      },
>  };
>
> @@ -56,6 +57,22 @@ action_impl_copy_funcs(struct odp_execute_action_impl *to,
>      }
>  }
>
> +int32_t

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.

> +            return 0;

Maybe we should add a log on the successful change of the implementation, so we can see this in the log?

> +        }
> +    }
> +    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 ;)


> +/* Implementation of the scalar actions impl init function. Build up the
> + * array of func ptrs here.
> + */
> +int32_t

int ?

> +odp_action_scalar_init(struct odp_execute_action_impl *self)
> +{
> +    self->funcs[OVS_ACTION_ATTR_POP_VLAN] = action_pop_vlan;
> +
> +    return 0;
> +}
> +
>  /* The active function pointers on the datapath. ISA optimized implementations
>   * are enabled by plugging them into this static arary, which is consulted when
>   * applying actions on the datapath.
> @@ -846,10 +868,22 @@ odp_execute_init(void)
>      static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>      if (ovsthread_once_start(&once)) {
>          odp_execute_action_init();
> +        odp_actions_impl_set("scalar");
>          ovsthread_once_done(&once);
>      }
>  }
>
> +int32_t

int

> +odp_actions_impl_set(const char *name)
> +{
> +
> +    int err = odp_execute_action_set(name, &actions_active_impl);
> +    if (err) {
> +        VLOG_ERR("error %d from action set to %s\n", err, name);
Make the error message more user friendly, and strip the \n. For example:

  "Failed setting action implementation to %s, error %d", name, err

> +        return -1;
> +    }
> +    return 0;

do a return err; here and remove the return -1;

> +}
>
>  /* Executes all of the 'actions_len' bytes of datapath actions in 'actions' on
>   * the packets in 'batch'.  If 'steal' is true, possibly modifies and
> @@ -965,12 +999,6 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
>              break;
>          }
>
> -        case OVS_ACTION_ATTR_POP_VLAN:
> -            DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
> -                eth_pop_vlan(packet);
> -            }
> -            break;
> -
>          case OVS_ACTION_ATTR_PUSH_MPLS: {
>              const struct ovs_action_push_mpls *mpls = nl_attr_get(a);
>
> @@ -1114,6 +1142,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
>          }
>          case OVS_ACTION_ATTR_OUTPUT:
>          case OVS_ACTION_ATTR_LB_OUTPUT:
> +        case OVS_ACTION_ATTR_POP_VLAN:
>          case OVS_ACTION_ATTR_TUNNEL_PUSH:
>          case OVS_ACTION_ATTR_TUNNEL_POP:
>          case OVS_ACTION_ATTR_USERSPACE:

Maybe add another section with actions that are handled by the scalar action implementation.

         case __OVS_ACTION_ATTR_MAX:
+        /* The following actions are handled by the scalar implementation. */
+        case OVS_ACTION_ATTR_POP_VLAN:
         OVS_NOT_REACHED();

> diff --git a/lib/odp-execute.h b/lib/odp-execute.h
> index c4f5303e7..6441392b9 100644
> --- a/lib/odp-execute.h
> +++ b/lib/odp-execute.h
> @@ -32,6 +32,8 @@ struct dp_packet_batch;
>  /* Called once at initialization time. */
>  void odp_execute_init(void);
>
> +int32_t odp_actions_impl_set(const char *name);

int

> +
>  typedef void (*odp_execute_cb)(void *dp, struct dp_packet_batch *batch,
>                                 const struct nlattr *action, bool should_steal);
>
> -- 
> 2.25.1

> -----Original Message-----
> From: Eelco Chaudron <echaudro@redhat.com>
> Sent: Thursday 2 June 2022 15:06
> To: Finn, Emma <emma.finn@intel.com>
> Cc: Van Haaren, Harry <harry.van.haaren@intel.com>; Amber, Kumar
> <kumar.amber@intel.com>; Stokes, Ian <ian.stokes@intel.com>;
> dev@openvswitch.org
> 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 <emma.finn@intel.com>
> > Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
> > ---
> >  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>