On 10 May 2022, at 16:21, 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 <[email protected]>
> Acked-by: Harry van Haaren <[email protected]>
> ---
> lib/odp-execute-private.c | 19 +++++++++++++++++-
> lib/odp-execute.c | 41 +++++++++++++++++++++++++++++++++------
> lib/odp-execute.h | 2 ++
> 3 files changed, 55 insertions(+), 7 deletions(-)
>
> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c
> index b3a02745c..996de0bf6 100644
> --- a/lib/odp-execute-private.c
> +++ b/lib/odp-execute-private.c
> @@ -29,13 +29,14 @@
>
> int32_t action_autoval_init(struct odp_execute_action_impl *self);
> VLOG_DEFINE_THIS_MODULE(odp_execute_private);
> +static uint32_t active_action_impl_index;
>
> static struct odp_execute_action_impl action_impls[] = {
> [ACTION_IMPL_SCALAR] = {
> .available = 1,
> .name = "scalar",
> .probe = NULL,
> - .init_func = NULL,
> + .init_func = odp_action_scalar_init,
> },
> };
>
> @@ -56,6 +57,22 @@ action_impl_copy_funcs(struct odp_execute_action_impl *to,
> }
> }
>
> +int32_t
int
> +odp_execute_action_set(const char *name,
> + struct odp_execute_action_impl *active)
> +{
> + uint32_t i;
> + for (i = 0; i < ACTION_IMPL_MAX; i++) {
> + /* String compare, and set ptrs atomically. */
> + if (strcmp(action_impls[i].name, name) == 0) {
OVS style seems to be (!strcmp())
> + action_impl_copy_funcs(active, &action_impls[i]);
> + active_action_impl_index = i;
Why do we copy the content!? Would it be way easier to just return the pointer
to the struct here, and use it in odp-execute.c.
Something like:
ATOMIC(static struct odp_execute_action_impl *actions_active_impl);
impl = odp_execute_action_GET(name)
if (impl)
atomic_store_relaxed(actions_active_impl, impl)
With this, you might need a call into odp-execute.c to get the current
configured value. But it would make the implementation more straightforward.
> + return 0;
Maybe we should add a log on the successful change of the implementation, so we
can see this in the log?
> + }
> + }
> + return -1;
return -EINVAL
> +}
> +
> void
> odp_execute_action_init(void)
> {
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index 165386e66..c2be74454 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -834,6 +834,28 @@ requires_datapath_assistance(const struct nlattr *a)
> return false;
> }
>
> +static void
> +action_pop_vlan(void *dp OVS_UNUSED, struct dp_packet_batch *batch,
> + const struct nlattr *a OVS_UNUSED,
> + bool should_steal OVS_UNUSED)
> +{
> + struct dp_packet *packet;
Add a line break here.
> + DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
> + eth_pop_vlan(packet);
> + }
> +}
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.
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.
My personal preference would be to move all handing to the scaler version in
one patch, and rip out the existing switch() existing code ;)
> +/* Implementation of the scalar actions impl init function. Build up the
> + * array of func ptrs here.
> + */
> +int32_t
int ?
> +odp_action_scalar_init(struct odp_execute_action_impl *self)
> +{
> + 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 +868,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);
> }
> }
>
> +int32_t
int
> +odp_actions_impl_set(const char *name)
> +{
> +
> + int err = odp_execute_action_set(name, &actions_active_impl);
> + if (err) {
> + VLOG_ERR("error %d from action set to %s\n", err, name);
Make the error message more user friendly, and strip the \n. For example:
"Failed setting action implementation to %s, error %d", name, err
> + return -1;
> + }
> + return 0;
do a return err; here and remove the return -1;
> +}
>
> /* Executes all of the 'actions_len' bytes of datapath actions in 'actions'
> on
> * the packets in 'batch'. If 'steal' is true, possibly modifies and
> @@ -965,12 +999,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);
>
> @@ -1114,6 +1142,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch
> *batch, bool steal,
> }
> case OVS_ACTION_ATTR_OUTPUT:
> case OVS_ACTION_ATTR_LB_OUTPUT:
> + case OVS_ACTION_ATTR_POP_VLAN:
> case OVS_ACTION_ATTR_TUNNEL_PUSH:
> case OVS_ACTION_ATTR_TUNNEL_POP:
> case OVS_ACTION_ATTR_USERSPACE:
Maybe add another section with actions that are handled by the scalar action
implementation.
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 c4f5303e7..6441392b9 100644
> --- a/lib/odp-execute.h
> +++ b/lib/odp-execute.h
> @@ -32,6 +32,8 @@ struct dp_packet_batch;
> /* Called once at initialization time. */
> void odp_execute_init(void);
>
> +int32_t odp_actions_impl_set(const char *name);
int
> +
> 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
