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 <[email protected]>
> Acked-by: Harry van Haaren <[email protected]>
> ---
> 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
