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> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
