[ovs-dev,v6,02/11] odp-execute: Add function pointers to odp-execute for different action implementations.

This commit introduces the initial infrastructure required to allow
different implementations for OvS actions. The patch introduces action
function pointers which allows user to switch between different action
implementations available. This will allow for more performance and flexibility
so the user can choose the action implementation to best suite their use case.

Signed-off-by: Emma Finn <emma.finn@intel.com>
Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 lib/automake.mk           |  2 +
 lib/dpif-netdev.c         |  4 ++
 lib/odp-execute-private.c | 94 ++++++++++++++++++++++++++++++++++++++
 lib/odp-execute-private.h | 96 +++++++++++++++++++++++++++++++++++++++
 lib/odp-execute.c         | 39 ++++++++++++++--
 lib/odp-execute.h         |  4 ++
 6 files changed, 234 insertions(+), 5 deletions(-)
 create mode 100644 lib/odp-execute-private.c
 create mode 100644 lib/odp-execute-private.h

On 10 May 2022, at 16:21, Emma Finn wrote:

> This commit introduces the initial infrastructure required to allow
> different implementations for OvS actions. The patch introduces action
> function pointers which allows user to switch between different action
> implementations available. This will allow for more performance and flexibility
> so the user can choose the action implementation to best suite their use case.
>
> Signed-off-by: Emma Finn <emma.finn@intel.com>
> Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  lib/automake.mk           |  2 +
>  lib/dpif-netdev.c         |  4 ++
>  lib/odp-execute-private.c | 94 ++++++++++++++++++++++++++++++++++++++
>  lib/odp-execute-private.h | 96 +++++++++++++++++++++++++++++++++++++++
>  lib/odp-execute.c         | 39 ++++++++++++++--
>  lib/odp-execute.h         |  4 ++
>  6 files changed, 234 insertions(+), 5 deletions(-)
>  create mode 100644 lib/odp-execute-private.c
>  create mode 100644 lib/odp-execute-private.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index a23cdc4ad..625c0d9c9 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -210,6 +210,8 @@ lib_libopenvswitch_la_SOURCES = \
>  	lib/object-collection.h \
>  	lib/odp-execute.c \
>  	lib/odp-execute.h \
> +	lib/odp-execute-private.c \
> +	lib/odp-execute-private.h \
>  	lib/odp-util.c \
>  	lib/odp-util.h \
>  	lib/ofp-actions.c \
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 61929049c..f74f8b864 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1686,6 +1686,10 @@ create_dpif_netdev(struct dp_netdev *dp)
>      dpif->dp = dp;
>      dpif->last_port_seq = seq_read(dp->port_seq);
>
> +    /* Called once at initialization time. This handles setting up the state
> +     * of the actions functions at init time. */
> +    odp_execute_init();
> +
>      return &dpif->dpif;
>  }
>
> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
> new file mode 100644
> index 000000000..b3a02745c
> --- /dev/null
> +++ b/lib/odp-execute-private.c
> @@ -0,0 +1,94 @@
> +/*
> + * Copyright (c) 2022 Intel.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +#include <errno.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> +#include "dpdk.h"
> +#include "dp-packet.h"
> +#include "odp-execute-private.h"
> +#include "odp-netlink.h"
> +#include "odp-util.h"
> +#include "openvswitch/vlog.h"
> +
> +
> +int32_t action_autoval_init(struct odp_execute_action_impl *self);
> +VLOG_DEFINE_THIS_MODULE(odp_execute_private);

I don't think we need to add the _private extension here, as this is still the odp_execute framework.
However, I think we do not support duplicate module names, so you might need to add something :(
"private" does not feel like a user-friendly name the dpif module uses _impl, so maybe you can use that also?

> +
> +static struct odp_execute_action_impl action_impls[] = {
> +    [ACTION_IMPL_SCALAR] = {
> +        .available = 1,

Please use "true" here. Or should it be false, as the init function will take care of setting this to true?

> +        .name = "scalar",
> +        .probe = NULL,
> +        .init_func = NULL,
> +    },
> +};
> +
> +static void
> +action_impl_init_funcs(struct odp_execute_action_impl *to)
> +{
> +    for (uint32_t i = 0; i < __OVS_ACTION_ATTR_MAX; i++) {

Why not just int here? In OVS in general we do not XintXX_t unless really needed.

> +        atomic_init(&to->funcs[i], NULL);
> +    }
> +}
> +
> +static void
> +action_impl_copy_funcs(struct odp_execute_action_impl *to,
> +                       const struct odp_execute_action_impl *from)

nit: stdlib like functions seem to use dest and src as argument.

> +{
> +    for (uint32_t i = 0; i < __OVS_ACTION_ATTR_MAX; i++) {

Same int comment as above

> +        atomic_store_relaxed(&to->funcs[i], from->funcs[i]);
> +    }
> +}
> +
> +void
> +odp_execute_action_init(void)
> +{
> +    /* Call probe on each impl, and cache the result. */
> +    for (int i = 0; i < ACTION_IMPL_MAX; i++) {
> +        bool avail = true;

Newline?

> +        if (action_impls[i].probe) {
> +            /* Return zero is success, non-zero means error. */
> +            avail = (action_impls[i].probe() == 0);
> +        }
> +        VLOG_INFO("Action implementation %s (available: %s)\n",
> +                  action_impls[i].name, avail ? "available" : "not available");
> +        action_impls[i].available = avail;
> +    }
> +
> +    uint32_t i;
> +    for (i = 0; i < ACTION_IMPL_MAX; i++) {

Please use int for i and define it in the for loop as you do elsewhere.

> +        /* Initialize Actions function pointers. */
> +        action_impl_init_funcs(&action_impls[i]);


Don't think we need to initialise them, as this is done through the action_impls[] structure definition.

> +
> +        /* Each impl's function array is initialized to reflect the scalar
> +         * implementation. This simplifies adding optimized implementations,
> +         * as the autovalidator can always compare all actions.
> +         *
> +         * Below copies the scalar functions to all other implementations.
> +         */
> +        if (i != ACTION_IMPL_SCALAR) {
> +            action_impl_copy_funcs(&action_impls[i],
> +                                   &action_impls[ACTION_IMPL_SCALAR]);
> +        }
> +
> +        if (action_impls[i].init_func) {
> +            action_impls[i].init_func(&action_impls[i]);

You do not check the return value of the init function. Guess it should be checked and if an error is returned its avail flag should be set to false.

> +        }
> +    }
> +}
> diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
> new file mode 100644
> index 000000000..869478ce9
> --- /dev/null
> +++ b/lib/odp-execute-private.h
> @@ -0,0 +1,96 @@
> +/*
> + * Copyright (c) 2022 Intel.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef ODP_EXTRACT_PRIVATE
> +#define ODP_EXTRACT_PRIVATE 1
> +
> +#include "dp-packet.h"
> +#include "odp-execute.h"
> +#include "odp-netlink.h"
> +#include "ovs-atomic.h"
> +
> +/* Forward declaration for typedef. */
> +struct odp_execute_action_impl;
> +
> +/* Typedef for an initialization function that can initialize each
> + * implementation, checking requirements such as CPU ISA.
> + */
> +typedef int32_t (*odp_execute_action_init_func)

Please use int as a return value.

> +                    (struct odp_execute_action_impl *self);
> +
> +/* Probe function is used to detect if this CPU has the ISA required
> + * to run the optimized action implementation.
> + * Returns zero on successful probe and available will be true.
> + * Returns negative errno on failure and available will be false.
> + */
> +typedef int (*odp_execute_action_probe)(void);
> +
> +/* Structure represents an implementation of the odp actions. */
> +struct odp_execute_action_impl {
> +    /* When set, the CPU ISA required for this implementation is available
> +     * and the implementation can be used.
> +     */
> +    bool available;
> +
> +    /* Name of the implementation. */
> +    const char *name;
> +
> +    /* Probe function is used to detect if this CPU has the ISA required
> +     * to run the optimized miniflow implementation. It is optional and
> +     * if it is not used, then it must be null.
> +     */
> +    odp_execute_action_probe probe;
> +
> +    /* Called to check requirements and if usable, initializes the
> +     * implementation for use.
> +     */
> +    odp_execute_action_init_func init_func;

Can we not combine the probe and init function together? From the code above, I see no reason for them being separate?

> +
> +    /* An array of callback functions, one for each action. */
> +    ATOMIC(odp_execute_cb) funcs[__OVS_KEY_ATTR_MAX];

Guess this is cut/paste error, this should be __OVS_ACTION_ATTR_MAX

> +};
> +
> +/* Order of Actions implementations. */
> +enum odp_execute_action_impl_idx {
> +    ACTION_IMPL_SCALAR,
> +    /* 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.
> +     */
> +
> +    ACTION_IMPL_MAX,
> +};
> +
> +/* Index to start verifying implementations from. */
> +BUILD_ASSERT_DECL(ACTION_IMPL_SCALAR == 0);
> +
> +/* 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
> + * initializing the scalar/generic codepath.
> + */
> +void odp_execute_action_init(void);
> +
> +/* Update the current active functions to those requested in name. */
> +void odp_execute_action_get(struct ds *name);
> +int32_t odp_execute_action_set(const char *name,

Should be int ?

> +                               struct odp_execute_action_impl *active);
> +
> +/* Init function for the scalar implementation. Calls into the odp-execute.c
> + * file, and initializes the function pointers for optimized action types.
> + */
> +int32_t odp_action_scalar_init(struct odp_execute_action_impl *self);

Should be int.

I do not see the last three function definitions in this patch?

> +
> +#endif /* ODP_EXTRACT_PRIVATE */
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index 7da56793d..165386e66 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -17,6 +17,7 @@
>
>  #include <config.h>
>  #include "odp-execute.h"
> +#include "odp-execute-private.h"
>  #include <sys/types.h>
>  #include <netinet/in.h>
>  #include <arpa/inet.h>
> @@ -833,6 +834,23 @@ requires_datapath_assistance(const struct nlattr *a)
>      return false;
>  }
>
> +/* 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.
> + */
> +static struct odp_execute_action_impl actions_active_impl;
> +
> +void
> +odp_execute_init(void)
> +{
> +    static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +    if (ovsthread_once_start(&once)) {
> +        odp_execute_action_init();
> +        ovsthread_once_done(&once);
> +    }
> +}
> +
> +
>  /* Executes all of the 'actions_len' bytes of datapath actions in 'actions' on
>   * the packets in 'batch'.  If 'steal' is true, possibly modifies and
>   * definitely free the packets in 'batch', otherwise leaves 'batch' unchanged.
> @@ -858,13 +876,12 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
>      NL_ATTR_FOR_EACH_UNSAFE (a, left, actions, actions_len) {
>          int type = nl_attr_type(a);
>          bool last_action = (left <= NLA_ALIGN(a->nla_len));
> +        /* Allow 'dp_execute_action' to steal the packet data if we do
> +         * not need it any more. */

I think the comment doesn’t make sense anymore in this place, so I would just remove it.

> +        bool should_steal = steal && last_action;
>
>          if (requires_datapath_assistance(a)) {
>              if (dp_execute_action) {
> -                /* Allow 'dp_execute_action' to steal the packet data if we do
> -                 * not need it any more. */
> -                bool should_steal = steal && last_action;
> -
>                  dp_execute_action(dp, batch, a, should_steal);
>
>                  if (last_action || dp_packet_batch_is_empty(batch)) {
> @@ -879,8 +896,20 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,

Do we really need the steal flag here? As we are only handling modification actions.

>              continue;
>          }
>
> -        switch ((enum ovs_action_attr) type) {
> +        /* If type is set in the active actions implementation, call the
> +         * function-pointer and continue to the next action.
> +         */
> +        enum ovs_action_attr attr_type = (enum ovs_action_attr) type;

Rather than do this here, fix it at the place where type gets define, i.e. something like:

enum ovs_action_attr type = (enum ovs_action_attr) nl_attr_type(a);

> +        if (actions_active_impl.funcs[attr_type]) {

We should also check the attr_type is in bound of __OVS_ACTION_ATTR_MAX.

> +            actions_active_impl.funcs[attr_type](NULL, batch, a, should_steal);

Why is the first parameter NULL? Could we pass in dp? However I do not see it used anywhere, so why not remove it completely?

Same for should_steal, we do not need it as we handle only modification of packets, i.e. none data path actions.

I have another general architecture question? What if for a specific packet (or batch of packets) the SIMD implementation is not able to process the packet/batch?
How can we handle this in a uniform way? Can you give this some thought and add support?

> +            continue;
> +        }
> +
> +        /* If the action was not handled by the active function pointers above,
> +         * process them by switching on the type below.
> +         */
>
> +        switch (attr_type) {
>          case OVS_ACTION_ATTR_HASH: {
>              const struct ovs_action_hash *hash_act = nl_attr_get(a);

Maybe add something like the below to avoid potential issues in the future.

diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 165386e66..975f6e2cb 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -1123,6 +1123,8 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal,
         case __OVS_ACTION_ATTR_MAX:
             OVS_NOT_REACHED();
         }
+        /* Do not add any generic processing here, as it won't be executed when
+         * an ISA-specific action implementation exists. */
     }

>
> diff --git a/lib/odp-execute.h b/lib/odp-execute.h
> index a3578a575..c4f5303e7 100644
> --- a/lib/odp-execute.h
> +++ b/lib/odp-execute.h
> @@ -28,6 +28,10 @@ struct dp_packet;
>  struct pkt_metadata;
>  struct dp_packet_batch;
>
> +
> +/* Called once at initialization time. */
> +void odp_execute_init(void);
> +
>  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:00
> 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 02/11] odp-execute: Add function pointers to odp-execute
> for different action implementations.
> 
> On 10 May 2022, at 16:21, Emma Finn wrote:
> 
> > This commit introduces the initial infrastructure required to allow
> > different implementations for OvS actions. The patch introduces action
> > function pointers which allows user to switch between different action
> > implementations available. This will allow for more performance and
> > flexibility so the user can choose the action implementation to best suite
> their use case.
> >
> > Signed-off-by: Emma Finn <emma.finn@intel.com>
> > Acked-by: Harry van Haaren <harry.van.haaren@intel.com>
> > ---
> >  lib/automake.mk           |  2 +
> >  lib/dpif-netdev.c         |  4 ++
> >  lib/odp-execute-private.c | 94
> ++++++++++++++++++++++++++++++++++++++
> >  lib/odp-execute-private.h | 96
> +++++++++++++++++++++++++++++++++++++++
> >  lib/odp-execute.c         | 39 ++++++++++++++--
> >  lib/odp-execute.h         |  4 ++
> >  6 files changed, 234 insertions(+), 5 deletions(-)  create mode
> > 100644 lib/odp-execute-private.c  create mode 100644
> > lib/odp-execute-private.h
> >

<snip unrelated diffs and discussion>

> > +            actions_active_impl.funcs[attr_type](NULL, batch, a,
> > + should_steal);
> 
> Why is the first parameter NULL? Could we pass in dp? However I do not
> see it used anywhere, so why not remove it completely?
> 
> Same for should_steal, we do not need it as we handle only modification of
> packets, i.e. none data path actions.
> 
> I have another general architecture question? What if for a specific packet
> (or batch of packets) the SIMD implementation is not able to process the
> packet/batch?
> How can we handle this in a uniform way? Can you give this some thought
> and add support?

We support 100% of the action (regardless of input packet, we perform same action as scalar actions code).  Autovalidation ensures the modifications are identical.
As a result there is no value in a "fallback to scalar" backup, as this must never occur. 

> > +            continue;
> > +        }
> > +
> > +        /* If the action was not handled by the active function pointers
> above,
> > +         * process them by switching on the type below.
> > +         */
> >
> > +        switch (attr_type) {
> >          case OVS_ACTION_ATTR_HASH: {
> >              const struct ovs_action_hash *hash_act = nl_attr_get(a);
> 
<snip unrelated diffs and discussion>