> -----Original Message----- > From: Pai G, Sunil <[email protected]> > Sent: Tuesday 12 July 2022 12:04 > To: Finn, Emma <[email protected]>; [email protected] > Cc: [email protected]; [email protected]; Van Haaren, Harry > <[email protected]>; Amber, Kumar <[email protected]> > 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> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
