On 14 Jul 2022, at 19:51, 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]>
>
<SNIP>
> +static int
> +odp_actions_impl_set(const char *name)
> +{
> + struct odp_execute_action_impl *active;
> + active = odp_execute_action_set(name);
> + if (!active) {
> + VLOG_ERR("Failed setting action implementation to %s", name);
> + return 1;
> + }
> +
> + actions_active_impl = active;
Guess you missed my previous comment, this needs an atomic store.
This is the diff:
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index 8a120223e..16960d001 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -837,7 +837,7 @@ requires_datapath_assistance(const struct nlattr *a)
/* 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;
+static ATOMIC(struct odp_execute_action_impl *)actions_active_impl;
static int
odp_actions_impl_set(const char *name)
@@ -849,9 +849,8 @@ odp_actions_impl_set(const char *name)
return 1;
}
- actions_active_impl = active;
+ atomic_store_relaxed(&actions_active_impl, active);
return 0;
-
}
Also removed the extra new-line.
> + return 0;
> +
> +}
<SNIP>
> @@ -879,8 +912,28 @@ odp_execute_actions(void *dp, struct dp_packet_batch
> *batch, bool steal,
> 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. */
> + if (attr_type <= OVS_ACTION_ATTR_MAX) {
> + /* Read the action function pointer atomically to avoid
> non-atomic
> + * read causing corruption if being written by another thread
> + * simultaneously. */
> + odp_execute_action_cb action_func;
> + atomic_uintptr_t *active_action_func =
> + (void *)&actions_active_impl->funcs[attr_type];
> + atomic_read_relaxed(active_action_func,
> + (uintptr_t *) &action_func);
It's not the actual function pointer you need to read atomically (those are
initialized once at startup before they are ever used), but the implementation
pointer.
So this is the diff fixing it:
@@ -915,17 +914,14 @@ odp_execute_actions(void *dp, struct dp_packet_batch
*batch, bool steal,
/* If type is set in the active actions implementation, call the
* function-pointer and continue to the next action. */
if (attr_type <= OVS_ACTION_ATTR_MAX) {
- /* Read the action function pointer atomically to avoid non-atomic
- * read causing corruption if being written by another thread
- * simultaneously. */
- odp_execute_action_cb action_func;
- atomic_uintptr_t *active_action_func =
- (void *)&actions_active_impl->funcs[attr_type];
- atomic_read_relaxed(active_action_func,
- (uintptr_t *) &action_func);
-
- if (action_func) {
- action_func(batch, a);
+ /* Read the action implementation pointer atomically to avoid
+ * non-atomic read causing corruption if being written by another
+ * thread simultaneously. */
+ struct odp_execute_action_impl *actions_impl;
+ atomic_read_relaxed(&actions_active_impl, &actions_impl);
+
+ if (actions_impl && actions_impl->funcs[attr_type]) {
+ actions_impl->funcs[attr_type](batch, a);
continue;
}
}
<SNIP>
//Eelco
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
