[ovs-dev,v7,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 | 18 +++++++++++++++-
 lib/odp-execute-private.h |  8 +++++++
 lib/odp-execute.c         | 44 +++++++++++++++++++++++++++++++++------
 lib/odp-execute.h         |  2 ++
 4 files changed, 65 insertions(+), 7 deletions(-)

On 14 Jun 2022, at 13:57, 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 | 18 +++++++++++++++-
>  lib/odp-execute-private.h |  8 +++++++
>  lib/odp-execute.c         | 44 +++++++++++++++++++++++++++++++++------
>  lib/odp-execute.h         |  2 ++
>  4 files changed, 65 insertions(+), 7 deletions(-)
>
> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
> index 92db2386c..25dbbfefc 100644
> --- a/lib/odp-execute-private.c
> +++ b/lib/odp-execute-private.c
> @@ -27,12 +27,13 @@
>  #include "openvswitch/vlog.h"
>
>  VLOG_DEFINE_THIS_MODULE(odp_execute_impl);
> +static int active_action_impl_index;
>
>  static struct odp_execute_action_impl action_impls[] = {
>      [ACTION_IMPL_SCALAR] = {
>          .available = false,
>          .name = "scalar",
> -        .init_func = NULL,
> +        .init_func = odp_action_scalar_init,
>      },
>  };
>
> @@ -45,6 +46,21 @@ action_impl_copy_funcs(struct odp_execute_action_impl *src,
>      }
>  }
>
> +int
> +odp_execute_action_set(const char *name,
> +                       struct odp_execute_action_impl *active)
> +{
> +    for (int i = 0; i < ACTION_IMPL_MAX; i++) {
> +        /* String compare, and set ptrs atomically. */
> +        if (!strcmp(action_impls[i].name, name)) {
> +            action_impl_copy_funcs(active, &action_impls[i]);;
> +            active_action_impl_index = i;

Discussion from v6:

EC> Why do we copy the content!? Would it be way easier to just return the
EC> pointer to the struct here, and use it in odp-execute.c.
EC>
EC> Something like:
EC>
EC>   ATOMIC(static struct odp_execute_action_impl *actions_active_impl);
EC>   impl = odp_execute_action_GET(name)
EC>   if (impl)
EC>      atomic_store_relaxed(actions_active_impl, impl)
EC>
EC> With this, you might need a call into odp-execute.c to get the current
EC> configured value. But it would make the implementation more
EC> straightforward.
EC>

EF> This rework isn't included in v7. What is the concern with the current approach?
EF> An atomic store of the entire impls struct is not possible. Can you provide a better explanation as to what you want here?

Guess I was not clear, but I was suggesting to change:

  static struct odp_execute_action_impl actions_active_impl;

into

  static struct odp_execute_action_impl *actions_active_impl;

This way, only a single pointer needs to be swapped, and the design looks nicer. If this is a performance choose, what is the delta in pp throughput if not done? If you want I can provide you with a diff with this change.

> +            return 0;

From v6:

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

EF> 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."

This is some debug log not enabled by default, so I think this warrants an explicit message.

   VLOG_INFO("Action implementation set to %s", name);

> +        }
> +    }
> +    return -EINVAL;
> +}
> +
>  void
>  odp_execute_action_init(void)
>  {
> diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
> index 08d1faf38..c5ab00b07 100644
> --- a/lib/odp-execute-private.h
> +++ b/lib/odp-execute-private.h
> @@ -71,4 +71,12 @@ BUILD_ASSERT_DECL(ACTION_IMPL_SCALAR == 0);
>   */
>  void odp_execute_action_init(void);
>
> +/* Init functions for the action implementations. Initializes the function
> + * pointers for optimized action types.
> + */
> +int odp_action_scalar_init(struct odp_execute_action_impl *self);
> +
> +int odp_execute_action_set(const char *name,
> +                               struct odp_execute_action_impl *active);
> +
>  #endif /* ODP_EXTRACT_PRIVATE */
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index 67ebbe195..eff80d93f 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -834,6 +834,30 @@ requires_datapath_assistance(const struct nlattr *a)
>      return false;
>  }
>
> +static void
> +action_pop_vlan(struct dp_packet_batch *batch,
> +                const struct nlattr *a OVS_UNUSED)
> +{
> +    struct dp_packet *packet;
> +
> +    DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
> +        eth_pop_vlan(packet);
> +    }
> +}
> +
> +/* Implementation of the scalar actions impl init function. Build up the
> + * array of func ptrs here.
> + */
> +int
> +odp_action_scalar_init(struct odp_execute_action_impl *self)
> +{
> +    /* Set function pointers for actions that can be applied directly, these
> +     * are identified by OVS_ACTION_ATTR_*. */
> +    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 +870,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);
>      }
>  }
>
> +int
> +odp_actions_impl_set(const char *name)
> +{
> +
> +    int err = odp_execute_action_set(name, &actions_active_impl);
> +    if (err) {
> +        VLOG_ERR("Failed setting action implementation to %s, error %d",
> +                 name, err);
> +    }
> +    return err;
> +}
>
>  /* Executes all of the 'actions_len' bytes of datapath actions in 'actions' on
>   * the packets in 'batch'.  If 'steal' is true, possibly modifies and
> @@ -964,12 +1000,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);
>
> @@ -1120,6 +1150,8 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
>          case OVS_ACTION_ATTR_CT:
>          case OVS_ACTION_ATTR_UNSPEC:
>          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 0921ee924..50d47b716 100644
> --- a/lib/odp-execute.h
> +++ b/lib/odp-execute.h
> @@ -35,6 +35,8 @@ void odp_execute_init(void);
>  typedef void (*odp_execute_action_cb)(struct dp_packet_batch *batch,
>                                        const struct nlattr *action);
>
> +int odp_actions_impl_set(const char *name);
> +
>  typedef void (*odp_execute_cb)(void *dp, struct dp_packet_batch *batch,
>                                 const struct nlattr *action, bool should_steal);
>
> -- 
> 2.32.0


Discussion from v6:

EC> 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.
EC> 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.
EC> My personal preference would be to move all handing to the scaler version in one patch, and rip out the existing switch() existing code ;)

EF> Yes, ideally all actions are in self-contained functions that process a burst of packets internally.
EF> 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.

EF> 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.
EF> Scalar actions can always be converted to the new function style later. For 2.18, the current actions are being incrementally updated.

I'm worried that, for some reason, this might get by unnoticed? What about adding a run-time check?

Here are my suggested changes for this patch including the check:

diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
index bb398008b..8860b399e 100644
--- a/lib/odp-execute-private.c
+++ b/lib/odp-execute-private.c
@@ -55,6 +55,8 @@ odp_execute_action_set(const char *name,
         if (!strcmp(action_impls[i].name, name)) {
             action_impl_copy_funcs(active, &action_impls[i]);;
             active_action_impl_index = i;
+
+            VLOG_INFO("Action implementation set to %s", name);
             return 0;
         }
     }
@@ -88,5 +90,19 @@ odp_execute_action_init(void)

         VLOG_INFO("Action implementation %s (available: %s)",
                   action_impls[i].name, avail ? "Yes" : "No");
+
+        /* The following is a run-time check to make sure a scalar
+         * implementation exists for the given ISA implementation. This to
+         * make sure the autovalidator works as expected. */
+         if (avail && i != ACTION_IMPL_SCALAR) {
+             for (int j = 0; j < __OVS_ACTION_ATTR_MAX; j++) {
+                 /* No ovs_assert(), as it can be compiled out. */
+                 if (action_impls[ACTION_IMPL_SCALAR].funcs[j] == NULL
+                     && action_impls[i].funcs[j] != NULL) {
+                     ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__,
+                                        "Missing scalar action function!");
+                 }
+             }
+         }
     }
 }