On 12 Jul 2022, at 13:45, Finn, Emma wrote:
>> -----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. 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
