> -----Original Message-----
> From: Finn, Emma <[email protected]>
> Sent: Wednesday, January 12, 2022 9:43 AM
> To: [email protected]; Van Haaren, Harry <[email protected]>;
> Amber, Kumar <[email protected]>; Stokes, Ian <[email protected]>;
> [email protected]
> Cc: Finn, Emma <[email protected]>
> Subject: [PATCH v5 1/8] 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 <[email protected]>
> Acked-by: Harry van Haaren <[email protected]>
Thanks for the patch Emma, few minor comments below.
> ---
> lib/automake.mk | 2 +
> lib/dpif-netdev.c | 2 +
> lib/odp-execute-private.c | 84 +++++++++++++++++++++++++++++++++
> lib/odp-execute-private.h | 98 +++++++++++++++++++++++++++++++++++++++
> lib/odp-execute.c | 39 ++++++++++++++--
> lib/odp-execute.h | 4 ++
> 6 files changed, 224 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 5224e0856..1bc855a6b 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -203,6 +203,8 @@ lib_libopenvswitch_la_SOURCES = \
> lib/nx-match.h \
> lib/object-collection.c \
> lib/object-collection.h \
> + lib/odp-execute-private.c \
> + lib/odp-execute-private.h \
These two should be placed after odp-execute.h below I would say (Similar to
how vport.c and vport-private.c were added in order above this section).
> lib/odp-execute.c \
> lib/odp-execute.h \
> lib/odp-util.c \
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 649c700cb..eada4fcd7 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1618,6 +1618,8 @@ create_dpif_netdev(struct dp_netdev *dp)
> dpif->dp = dp;
> dpif->last_port_seq = seq_read(dp->port_seq);
>
> + odp_execute_init();
Single line comment above this explaining its purpose would be nice to see.
> +
> return &dpif->dpif;
> }
>
> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
> new file mode 100644
> index 000000000..6441c491c
> --- /dev/null
> +++ b/lib/odp-execute-private.c
> @@ -0,0 +1,84 @@
> +/*
> + * Copyright (c) 2021 Intel.
> + *
Not sure if we need to update above to 2022 as that would be the year of
targeted merge to the code base? @Ilya any thoughts here?
> + * 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 <stdio.h>
> +#include <string.h>
> +#include <errno.h>
Above should be added in alphabetical order.
> +#include "dpdk.h"
This should be added with the block below (again alphabetical order)
> +
> +#include "openvswitch/vlog.h"
> +#include "odp-execute-private.h"
> +#include "odp-netlink.h"
> +#include "dp-packet.h"
> +#include "odp-util.h"
> +
> +
> +int32_t action_autoval_init(struct odp_execute_action_impl *self);
> +VLOG_DEFINE_THIS_MODULE(odp_execute_private);
> +
> +static struct odp_execute_action_impl action_impls[] = {
> + [ACTION_IMPL_SCALAR] = {
> + .available = 1,
> + .name = "scalar",
> + .probe = NULL,
> + .init_func = NULL,
> + },
> +};
> +
> +static void
> +action_impl_copy_funcs(struct odp_execute_action_impl *to,
> + const struct odp_execute_action_impl *from)
> +{
> + for (uint32_t i = 0; i < __OVS_KEY_ATTR_MAX; i++) {
> + atomic_uintptr_t *func = (void *) &to->funcs[i];
> + atomic_store_relaxed(func, (uintptr_t) 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;
> + if (action_impls[i].probe) {
> + /* Return zero is success, non-zero means error. */
I found this comment is a bit misleading.
You say return 0 on success, yet looking at the comments for typedef int
(*odp_execute_action_probe)(void); It says
/* Probe function is used to detect if this CPU has the ISA required
* to run the optimized action implementation.
* returns one on successful probe.
* returns negative errno on failure.
*/
So is it not more accurate to say if probe returns positive value then true
else if negative errno value returned set to false?
Or have I misunderstood the logic?
> + 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++) {
> + /* 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]);
> + }
> + }
> +}
> diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h
> new file mode 100644
> index 000000000..c2e86bbee
> --- /dev/null
> +++ b/lib/odp-execute-private.h
> @@ -0,0 +1,98 @@
> +/*
> + * Copyright (c) 2021 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 "odp-execute.h"
> +
> +/* For __OVS_KEY_ATTR_MAX. */
> +#include "odp-netlink.h"
> +#include "dp-packet.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)
> + (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 one on successful probe.
> + * returns negative errno on failure.
> + */
As flagged above, some concern here about the expected return values and the
functions use in the code, good to confirm the comments on return values match
that of whats in the code and how it is being used.
> +typedef int (*odp_execute_action_probe)(void);
> +
Snip..
> - switch ((enum ovs_action_attr) type) {
> + /* If type is set in the active actions implementation, call the
> + * function-pointer and an continue to the next action.
> + */
Typo above 'and continue'
> + enum ovs_action_attr attr_type = (enum ovs_action_attr) type;
> + if (actions_active_impl.funcs[attr_type]) {
> + actions_active_impl.funcs[attr_type](NULL, batch, a,
> should_steal);
> + 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);
>
> 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);
Thanks
Ian
>
> --
> 2.25.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
