Message ID | 20220713182807.3416578-3-harry.van.haaren@intel.com |
---|---|
State | Changes Requested |
Headers | show |
Series | Actions Infrastructure + Optimizations | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/intel-ovs-compilation | success | test: success |
Hey all,
Have tested the all the patches in the series.
<Snip>
Tested-by: Kumar Amber <kumar.amber@intel.com>
BR
Amber
Hi Harry/Emma, > -----Original Message----- > From: Van Haaren, Harry <harry.van.haaren@intel.com> > Sent: Wednesday, July 13, 2022 11:58 PM > To: dev@openvswitch.org > Cc: i.maximets@ovn.org; echaudro@redhat.com; Amber, Kumar > <kumar.amber@intel.com>; Pai G, Sunil <sunil.pai.g@intel.com>; Finn, Emma > <emma.finn@intel.com>; Stokes, Ian <ian.stokes@intel.com>; Van Haaren, > Harry <harry.van.haaren@intel.com> > Subject: [PATCH v10 02/10] odp-execute: Add function pointer for pop_vlan > action. > > From: Emma Finn <emma.finn@intel.com> > > 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 <emma.finn@intel.com> > Acked-by: Harry van Haaren <harry.van.haaren@intel.com> > --- > lib/odp-execute-private.c | 16 +++++++++++++++- lib/odp-execute- > private.h | 5 +++++ > lib/odp-execute.c | 32 ++++++++++++++++++++++++++------ > 3 files changed, 46 insertions(+), 7 deletions(-) Patch LGTM, Acked-by: Sunil Pai G <sunil.pai.g@intel.com> Thanks and regards Sunil
On 13 Jul 2022, at 20:27, Harry van Haaren wrote: > From: Emma Finn <emma.finn@intel.com> > > 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 <emma.finn@intel.com> > Acked-by: Harry van Haaren <harry.van.haaren@intel.com> Thanks for adding all the suggested changes, here and in the first patch. Acked-by: Eelco Chaudron <echaudro@redhat.com> One small nit, maybe can be fixed at commit time? But we need a v11 anyway, so you could add it there. > --- a/lib/odp-execute-private.h > +++ b/lib/odp-execute-private.h > @@ -71,6 +71,11 @@ BUILD_ASSERT_DECL(ACTION_IMPL_SCALAR == 0); > */ > void odp_execute_action_init(void); > > +/* Init functions for the action implementations. Initializes the function > + * pointers for optimized action types. > + */ The comment ending should be on the last line. //Eelco
diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c index 2c30ed05b..c1d153c6e 100644 --- a/lib/odp-execute-private.c +++ b/lib/odp-execute-private.c @@ -33,7 +33,7 @@ static struct odp_execute_action_impl action_impls[] = { [ACTION_IMPL_SCALAR] = { .available = false, .name = "scalar", - .init_func = NULL, + .init_func = odp_action_scalar_init, }, }; @@ -88,5 +88,19 @@ odp_execute_action_init(void) VLOG_INFO("Action implementation %s (available: %s)", action_impls[i].name, avail ? "Yes" : "No"); + + /* The following is a run-time check to make sure a scalar + * implementation exists for the given ISA implementation. This is to + * make sure the autovalidator works as expected. */ + if (avail && i != ACTION_IMPL_SCALAR) { + for (int j = 0; j < __OVS_ACTION_ATTR_MAX; j++) { + /* No ovs_assert(), as it can be compiled out. */ + if (action_impls[ACTION_IMPL_SCALAR].funcs[j] == NULL + && action_impls[i].funcs[j] != NULL) { + ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, + "Missing scalar action function!"); + } + } + } } } diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h index 24126cdca..ae06fbc09 100644 --- a/lib/odp-execute-private.h +++ b/lib/odp-execute-private.h @@ -71,6 +71,11 @@ BUILD_ASSERT_DECL(ACTION_IMPL_SCALAR == 0); */ void odp_execute_action_init(void); +/* Init functions for the action implementations. Initializes the function + * pointers for optimized action types. + */ +int odp_action_scalar_init(struct odp_execute_action_impl *self); + struct odp_execute_action_impl * odp_execute_action_set(const char *name); #endif /* ODP_EXTRACT_PRIVATE */ diff --git a/lib/odp-execute.c b/lib/odp-execute.c index 7f998add6..368876f27 100644 --- a/lib/odp-execute.c +++ b/lib/odp-execute.c @@ -834,6 +834,30 @@ requires_datapath_assistance(const struct nlattr *a) return false; } +static void +action_pop_vlan(struct dp_packet_batch *batch, + const struct nlattr *a OVS_UNUSED) +{ + struct dp_packet *packet; + + DP_PACKET_BATCH_FOR_EACH (i, packet, batch) { + eth_pop_vlan(packet); + } +} + +/* Implementation of the scalar actions impl init function. Build up the + * array of func ptrs here. + */ +int +odp_action_scalar_init(struct odp_execute_action_impl *self) +{ + /* Set function pointers for actions that can be applied directly, these + * are identified by OVS_ACTION_ATTR_*. */ + 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. @@ -982,12 +1006,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); @@ -1138,6 +1156,8 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, case OVS_ACTION_ATTR_CT: case OVS_ACTION_ATTR_UNSPEC: case __OVS_ACTION_ATTR_MAX: + /* The following actions are handled by the scalar implementation. */ + case OVS_ACTION_ATTR_POP_VLAN: OVS_NOT_REACHED(); }