| Message ID | 20220707153900.3147694-2-emma.finn@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 |
Hi Emma, Thanks for the patch, generally looks good, few comments inline. <snipped> > diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c new > file mode 100644 index 000000000..b10880ed9 > --- /dev/null > +++ b/lib/odp-execute-private.c > @@ -0,0 +1,92 @@ > +/* > + * 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" > + > +VLOG_DEFINE_THIS_MODULE(odp_execute_impl); > +static int active_action_impl_index; > + > +static struct odp_execute_action_impl action_impls[] = { > + [ACTION_IMPL_SCALAR] = { > + .available = false, > + .name = "scalar", > + .init_func = NULL, > + }, > +}; > + > +static void > +action_impl_copy_funcs(struct odp_execute_action_impl *src, > + const struct odp_execute_action_impl *dest) { > + for (int i = 0; i < __OVS_ACTION_ATTR_MAX; i++) { > + atomic_store_relaxed(&src->funcs[i], dest->funcs[i]); > + } > +} I think the variable names "src" and "dest" should be the other way around ? <snipped> > diff --git a/lib/odp-execute.c b/lib/odp-execute.c index > 7da56793d..f6f5bea48 100644 > --- a/lib/odp-execute.c > +++ b/lib/odp-execute.c > @@ -17,6 +17,7 @@ <snipped> > /* 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. > @@ -857,6 +890,7 @@ 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); > + enum ovs_action_attr attr_type = (enum ovs_action_attr) type; > bool last_action = (left <= NLA_ALIGN(a->nla_len)); > > if (requires_datapath_assistance(a)) { @@ -879,8 +913,20 @@ > 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 (actions_active_impl->funcs[attr_type] && > + attr_type <= OVS_ACTION_ATTR_MAX) { I would rather prefer using the __OVS_ACTION_ATTR_MAX over the OVS_ACTION_ATTR_MAX with "<" semantics than "<=" Lets try to be consistent in terms of usage as well 😊 <snipped>
> -----Original Message----- > From: Pai G, Sunil <sunil.pai.g@intel.com> > Sent: Tuesday 12 July 2022 12:04 > To: Finn, Emma <emma.finn@intel.com>; dev@openvswitch.org > Cc: i.maximets@ovn.org; echaudro@redhat.com; Van Haaren, Harry > <harry.van.haaren@intel.com>; Amber, Kumar <kumar.amber@intel.com> > Subject: RE: [ovs-dev] [v8 01/10] odp-execute: Add function pointers to odp- > execute for different action implementations. > > Hi Emma, > > Thanks for the patch, generally looks good, few comments inline. > > <snipped> > > > diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c new > > file mode 100644 index 000000000..b10880ed9 > > --- /dev/null > > +++ b/lib/odp-execute-private.c > > @@ -0,0 +1,92 @@ > > +/* > > + * 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" > > + > > +VLOG_DEFINE_THIS_MODULE(odp_execute_impl); > > +static int active_action_impl_index; > > + > > +static struct odp_execute_action_impl action_impls[] = { > > + [ACTION_IMPL_SCALAR] = { > > + .available = false, > > + .name = "scalar", > > + .init_func = NULL, > > + }, > > +}; > > + > > +static void > > +action_impl_copy_funcs(struct odp_execute_action_impl *src, > > + const struct odp_execute_action_impl *dest) { > > + for (int i = 0; i < __OVS_ACTION_ATTR_MAX; i++) { > > + atomic_store_relaxed(&src->funcs[i], dest->funcs[i]); > > + } > > +} > > I think the variable names "src" and "dest" should be the other way around ? > > <snipped> > > > diff --git a/lib/odp-execute.c b/lib/odp-execute.c index > > 7da56793d..f6f5bea48 100644 > > --- a/lib/odp-execute.c > > +++ b/lib/odp-execute.c > > @@ -17,6 +17,7 @@ > > <snipped> > > > /* 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. > > @@ -857,6 +890,7 @@ 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); > > + enum ovs_action_attr attr_type = (enum ovs_action_attr) type; > > bool last_action = (left <= NLA_ALIGN(a->nla_len)); > > > > if (requires_datapath_assistance(a)) { @@ -879,8 +913,20 @@ > > 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 (actions_active_impl->funcs[attr_type] && > > + attr_type <= OVS_ACTION_ATTR_MAX) { > > I would rather prefer using the __OVS_ACTION_ATTR_MAX over the > OVS_ACTION_ATTR_MAX with "<" semantics than "<=" > Lets try to be consistent in terms of usage as well 😊 > Sure, I will fix in next revision. > <snipped>
On 12 Jul 2022, at 13:45, Finn, Emma wrote: >> -----Original Message----- >> From: Pai G, Sunil <sunil.pai.g@intel.com> >> Sent: Tuesday 12 July 2022 12:04 >> To: Finn, Emma <emma.finn@intel.com>; dev@openvswitch.org >> Cc: i.maximets@ovn.org; echaudro@redhat.com; Van Haaren, Harry >> <harry.van.haaren@intel.com>; Amber, Kumar <kumar.amber@intel.com> >> Subject: RE: [ovs-dev] [v8 01/10] odp-execute: Add function pointers to odp- >> execute for different action implementations. >> >> Hi Emma, >> >> Thanks for the patch, generally looks good, few comments inline. >> >> <snipped> >> >>> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c new >>> file mode 100644 index 000000000..b10880ed9 >>> --- /dev/null >>> +++ b/lib/odp-execute-private.c >>> @@ -0,0 +1,92 @@ >>> +/* >>> + * 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" >>> + >>> +VLOG_DEFINE_THIS_MODULE(odp_execute_impl); >>> +static int active_action_impl_index; >>> + >>> +static struct odp_execute_action_impl action_impls[] = { >>> + [ACTION_IMPL_SCALAR] = { >>> + .available = false, >>> + .name = "scalar", >>> + .init_func = NULL, >>> + }, >>> +}; >>> + >>> +static void >>> +action_impl_copy_funcs(struct odp_execute_action_impl *src, >>> + const struct odp_execute_action_impl *dest) { >>> + for (int i = 0; i < __OVS_ACTION_ATTR_MAX; i++) { >>> + atomic_store_relaxed(&src->funcs[i], dest->funcs[i]); >>> + } >>> +} >> >> I think the variable names "src" and "dest" should be the other way around ? >> >> <snipped> >> >>> diff --git a/lib/odp-execute.c b/lib/odp-execute.c index >>> 7da56793d..f6f5bea48 100644 >>> --- a/lib/odp-execute.c >>> +++ b/lib/odp-execute.c >>> @@ -17,6 +17,7 @@ >> >> <snipped> >> >>> /* 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. >>> @@ -857,6 +890,7 @@ 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); >>> + enum ovs_action_attr attr_type = (enum ovs_action_attr) type; >>> bool last_action = (left <= NLA_ALIGN(a->nla_len)); >>> >>> if (requires_datapath_assistance(a)) { @@ -879,8 +913,20 @@ >>> 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 (actions_active_impl->funcs[attr_type] && >>> + attr_type <= OVS_ACTION_ATTR_MAX) { >> >> I would rather prefer using the __OVS_ACTION_ATTR_MAX over the >> OVS_ACTION_ATTR_MAX with "<" semantics than "<=" >> Lets try to be consistent in terms of usage as well 😊 >> > > Sure, I will fix in next revision. Please undo this, we should use OVS_ACTION_ATTR_MAX here. The __ indicates this is a more private variable, so you should use this in the switch() cases where it’s a must. //Eelco
On 12 Jul 2022, at 13:04, Pai G, Sunil wrote: > Hi Emma, > > Thanks for the patch, generally looks good, few comments inline. > > <snipped> > >> diff --git a/lib/odp-execute-private.c b/lib/odp-execute-private.c new >> file mode 100644 index 000000000..b10880ed9 >> --- /dev/null >> +++ b/lib/odp-execute-private.c >> @@ -0,0 +1,92 @@ >> +/* >> + * 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" >> + >> +VLOG_DEFINE_THIS_MODULE(odp_execute_impl); >> +static int active_action_impl_index; >> + >> +static struct odp_execute_action_impl action_impls[] = { >> + [ACTION_IMPL_SCALAR] = { >> + .available = false, >> + .name = "scalar", >> + .init_func = NULL, >> + }, >> +}; >> + >> +static void >> +action_impl_copy_funcs(struct odp_execute_action_impl *src, >> + const struct odp_execute_action_impl *dest) { >> + for (int i = 0; i < __OVS_ACTION_ATTR_MAX; i++) { >> + atomic_store_relaxed(&src->funcs[i], dest->funcs[i]); >> + } >> +} > > I think the variable names "src" and "dest" should be the other way around ? > > <snipped> I think the function used to have dest first? But I see it’s fixed in v9. //Eelco
> -----Original Message----- > From: Eelco Chaudron <echaudro@redhat.com> > Sent: Wednesday, July 13, 2022 9:55 AM > To: Finn, Emma <emma.finn@intel.com> > Cc: Pai G, Sunil <sunil.pai.g@intel.com>; dev@openvswitch.org; > i.maximets@ovn.org; Van Haaren, Harry <harry.van.haaren@intel.com>; Amber, > Kumar <kumar.amber@intel.com> > Subject: Re: [ovs-dev] [v8 01/10] odp-execute: Add function pointers to odp-execute > for different action implementations. <snip unrelated code> > >>> - 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 (actions_active_impl->funcs[attr_type] && > >>> + attr_type <= OVS_ACTION_ATTR_MAX) { > >> > >> I would rather prefer using the __OVS_ACTION_ATTR_MAX over the > >> OVS_ACTION_ATTR_MAX with "<" semantics than "<=" > >> Lets try to be consistent in terms of usage as well 😊 > >> > > > > Sure, I will fix in next revision. > > Please undo this, we should use OVS_ACTION_ATTR_MAX here. The __ indicates this > is a more private variable, so you should use this in the switch() cases where it’s a > must. Ok, moving back to "<= OVS_ACTION_ATTR_MAX" here for this instance. Note there is no functional difference. If required, wider clean-ups/consistency improvements can be done after patchset merge.
On 13 Jul 2022, at 12:59, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Eelco Chaudron <echaudro@redhat.com> >> Sent: Wednesday, July 13, 2022 9:55 AM >> To: Finn, Emma <emma.finn@intel.com> >> Cc: Pai G, Sunil <sunil.pai.g@intel.com>; dev@openvswitch.org; >> i.maximets@ovn.org; Van Haaren, Harry <harry.van.haaren@intel.com>; Amber, >> Kumar <kumar.amber@intel.com> >> Subject: Re: [ovs-dev] [v8 01/10] odp-execute: Add function pointers to odp-execute >> for different action implementations. > > <snip unrelated code> > >>>>> - 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 (actions_active_impl->funcs[attr_type] && >>>>> + attr_type <= OVS_ACTION_ATTR_MAX) { >>>> >>>> I would rather prefer using the __OVS_ACTION_ATTR_MAX over the >>>> OVS_ACTION_ATTR_MAX with "<" semantics than "<=" >>>> Lets try to be consistent in terms of usage as well 😊 >>>> >>> >>> Sure, I will fix in next revision. >> >> Please undo this, we should use OVS_ACTION_ATTR_MAX here. The __ indicates this >> is a more private variable, so you should use this in the switch() cases where it’s a >> must. > > Ok, moving back to "<= OVS_ACTION_ATTR_MAX" here for this instance. > Note there is no functional difference. If required, wider clean-ups/consistency > improvements can be done after patchset merge. ACK!
diff --git a/lib/automake.mk b/lib/automake.mk index 1d00cfa20..23ba4fab0 100644 --- a/lib/automake.mk +++ b/lib/automake.mk @@ -216,6 +216,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 d09138f2c..58be9d0f0 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -1678,6 +1678,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..b10880ed9 --- /dev/null +++ b/lib/odp-execute-private.c @@ -0,0 +1,92 @@ +/* + * 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" + +VLOG_DEFINE_THIS_MODULE(odp_execute_impl); +static int active_action_impl_index; + +static struct odp_execute_action_impl action_impls[] = { + [ACTION_IMPL_SCALAR] = { + .available = false, + .name = "scalar", + .init_func = NULL, + }, +}; + +static void +action_impl_copy_funcs(struct odp_execute_action_impl *src, + const struct odp_execute_action_impl *dest) +{ + for (int i = 0; i < __OVS_ACTION_ATTR_MAX; i++) { + atomic_store_relaxed(&src->funcs[i], dest->funcs[i]); + } +} + +struct odp_execute_action_impl * +odp_execute_action_set(const char *name) +{ + for (int i = 0; i < ACTION_IMPL_MAX; i++) { + /* String compare, and set ptrs atomically. */ + if (!strcmp(action_impls[i].name, name)) { + active_action_impl_index = i; + + VLOG_INFO("Action implementation set to %s", name); + return &action_impls[i]; + } + } + return NULL; +} + +void +odp_execute_action_init(void) +{ + /* 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 will check if impl is available and copies the scalar functions + * to all other implementations. + */ + for (int i = 0; i < ACTION_IMPL_MAX; i++) { + bool avail = true; + + if (i != ACTION_IMPL_SCALAR) { + action_impl_copy_funcs(&action_impls[i], + &action_impls[ACTION_IMPL_SCALAR]); + } + + if (action_impls[i].init_func) { + /* Return zero is success, non-zero means error. */ + avail = (action_impls[i].init_func(&action_impls[i]) == 0); + } + + action_impls[i].available = avail; + + VLOG_INFO("Action implementation %s (available: %s)", + action_impls[i].name, avail ? "Yes" : "No"); + } +} diff --git a/lib/odp-execute-private.h b/lib/odp-execute-private.h new file mode 100644 index 000000000..24126cdca --- /dev/null +++ b/lib/odp-execute-private.h @@ -0,0 +1,76 @@ +/* + * 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 int (*odp_execute_action_init_func) + (struct odp_execute_action_impl *self); + +/* 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; + + /* Function is used to detect if this CPU has the ISA required + * to run the optimized action implementation and if available, initializes + * the implementation for use. + */ + odp_execute_action_init_func init_func; + + /* An array of callback functions, one for each action. */ + ATOMIC(odp_execute_action_cb) funcs[__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); + +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 7da56793d..f6f5bea48 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,38 @@ 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; + +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; + return 0; + +} + +void +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); + } +} + /* 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. @@ -857,6 +890,7 @@ 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); + enum ovs_action_attr attr_type = (enum ovs_action_attr) type; bool last_action = (left <= NLA_ALIGN(a->nla_len)); if (requires_datapath_assistance(a)) { @@ -879,8 +913,20 @@ 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 (actions_active_impl->funcs[attr_type] && + attr_type <= OVS_ACTION_ATTR_MAX) { + actions_active_impl->funcs[attr_type](batch, a); + 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); @@ -1094,6 +1140,9 @@ 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. */ } dp_packet_delete_batch(batch, steal); diff --git a/lib/odp-execute.h b/lib/odp-execute.h index a3578a575..0921ee924 100644 --- a/lib/odp-execute.h +++ b/lib/odp-execute.h @@ -28,6 +28,13 @@ struct dp_packet; struct pkt_metadata; struct dp_packet_batch; + +/* Called once at initialization time. */ +void odp_execute_init(void); + +typedef void (*odp_execute_action_cb)(struct dp_packet_batch *batch, + const struct nlattr *action); + typedef void (*odp_execute_cb)(void *dp, struct dp_packet_batch *batch, const struct nlattr *action, bool should_steal);