On Thu, Apr 10, 2025 at 4:13 AM Mark Michelson via dev <
ovs-dev@openvswitch.org> wrote:
> In current OVN, an engine_node has two function callbacks that are
> optional, is_valid() and clear_tracked_data(). There are individual
> macros that allow for each of these optional functions to be defined in
> the engine_node. With two optional callbacks, this means there are four
> possible macros:
>
> - vanilla engine node (no is_valid(), no clear_tracked_data())
> - has clear_tracked_data(), does not have is_valid()
> - has is_valid(), does not have clear_tracked_data()
> - has both is_valid() and clear_tracked_data()
>
> In OVN we have bespoke macros for the first, second, and fourth
> scenarios [1].
>
> The main issues with this are:
> - This does not scale well if engine_nodes need new optional callbacks.
> - The macro names are quite long.
>
> This patch offers a new alternative. It alters the ENGINE_NODE macro so
> that it can take a variable number of arguments. Some preprocessor magic
> uses the number of arguments passed in to call a specific variant
> (ENGINE_NODE2, ENGINE_NODE3, or ENGINE_NODE4 currently,
> since these are the possible number of inputs to ENGINE_NODE). Depending
> on the variant called, additional optional elements of the resulting
> engine_node can be set. Convenience macros are provided for the two
> optional callbacks.
>
> Under the hood, there's a lot going on. But from the engine node
> developer's perspective, it's much easier to declare engine nodes, since
> there is only one macro to declare an engine node, and its arguments
> help define which optional callbacks are defined.
>
> [1] There is one engine_node (acl_id) that falls into the third
> scenario but rather than use a custom macro, it just defines its
> is_valid() callback after the initial engine_node definition.
>
> Signed-off-by: Mark Michelson <mmich...@redhat.com>
> ---
>
controller/ovn-controller.c | 22 ++++++++++-----------
> lib/inc-proc-eng.h | 38 +++++++++++++++++++++++--------------
> lib/ovn-util.h | 36 +++++++++++++++++++++++++++++++++++
> northd/inc-proc-northd.c | 20 +++++++++----------
> 4 files changed, 80 insertions(+), 36 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 440c76ca1..80e6b2b3f 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -5598,30 +5598,28 @@ main(int argc, char *argv[])
>
> /* Define inc-proc-engine nodes. */
> ENGINE_NODE(sb_ro, "sb_ro");
> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(template_vars, "template_vars");
> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(ct_zones, "ct_zones");
> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ovs_interface_shadow,
> - "ovs_interface_shadow");
> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(runtime_data, "runtime_data");
> + ENGINE_NODE(template_vars, "template_vars", CLEAR_TRACKED_DATA);
> + ENGINE_NODE(ct_zones, "ct_zones", CLEAR_TRACKED_DATA, IS_VALID);
> + ENGINE_NODE(ovs_interface_shadow, "ovs_interface_shadow",
> CLEAR_TRACKED_DATA);
> + ENGINE_NODE(runtime_data, "runtime_data", CLEAR_TRACKED_DATA);
> ENGINE_NODE(non_vif_data, "non_vif_data");
> ENGINE_NODE(mff_ovn_geneve, "mff_ovn_geneve");
> ENGINE_NODE(ofctrl_is_connected, "ofctrl_is_connected");
> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(activated_ports, "activated_ports");
> + ENGINE_NODE(activated_ports, "activated_ports", CLEAR_TRACKED_DATA);
> ENGINE_NODE(postponed_ports, "postponed_ports");
> ENGINE_NODE(pflow_output, "physical_flow_output");
> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lflow_output,
> "logical_flow_output");
> + ENGINE_NODE(lflow_output, "logical_flow_output", CLEAR_TRACKED_DATA);
> ENGINE_NODE(controller_output, "controller_output");
> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(addr_sets, "addr_sets");
> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups");
> + ENGINE_NODE(addr_sets, "addr_sets", CLEAR_TRACKED_DATA);
> + ENGINE_NODE(port_groups, "port_groups", CLEAR_TRACKED_DATA);
> ENGINE_NODE(northd_options, "northd_options");
> ENGINE_NODE(dhcp_options, "dhcp_options");
> ENGINE_NODE(if_status_mgr, "if_status_mgr");
> - ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lb_data, "lb_data");
> + ENGINE_NODE(lb_data, "lb_data", CLEAR_TRACKED_DATA);
> ENGINE_NODE(mac_cache, "mac_cache");
> ENGINE_NODE(bfd_chassis, "bfd_chassis");
> ENGINE_NODE(dns_cache, "dns_cache");
> - ENGINE_NODE(acl_id, "acl_id");
> - en_acl_id.is_valid = en_acl_id_is_valid;
> + ENGINE_NODE(acl_id, "acl_id", IS_VALID);
> ENGINE_NODE(route, "route");
> ENGINE_NODE(route_table_notify, "route_table_notify");
> ENGINE_NODE(route_exchange, "route_exchange");
> diff --git a/lib/inc-proc-eng.h b/lib/inc-proc-eng.h
> index 12e232020..e4c71d773 100644
> --- a/lib/inc-proc-eng.h
> +++ b/lib/inc-proc-eng.h
> @@ -397,29 +397,39 @@ engine_noop_handler(struct engine_node *node
> OVS_UNUSED, void *data OVS_UNUSED)
> void engine_ovsdb_node_add_index(struct engine_node *, const char *name,
> struct ovsdb_idl_index *);
>
> -/* Macro to define an engine node. */
> -#define ENGINE_NODE_DEF(NAME, NAME_STR, CLEAR_TRACKED_DATA, IS_VALID) \
> +#define ENGINE_NODE_DEF_START(NAME, NAME_STR) \
> struct engine_node en_##NAME = { \
> .name = NAME_STR, \
> .data = NULL, \
> .state = EN_STALE, \
> .init = en_##NAME##_init, \
> .run = en_##NAME##_run, \
> - .cleanup = en_##NAME##_cleanup, \
> - .is_valid = IS_VALID, \
> - .clear_tracked_data = CLEAR_TRACKED_DATA, \
> - };
> + .cleanup = en_##NAME##_cleanup,
>
> -#define ENGINE_NODE(NAME, NAME_STR) \
> - ENGINE_NODE_DEF(NAME, NAME_STR, NULL, NULL)
> +#define ENGINE_NODE_DEF_END };
>
> -#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA(NAME, NAME_STR) \
> - ENGINE_NODE_DEF(NAME, NAME_STR, en_##NAME##_clear_tracked_data, NULL)
> +#define ENGINE_NODE2(NAME, NAME_STR) \
> + ENGINE_NODE_DEF_START(NAME, NAME_STR) \
> + ENGINE_NODE_DEF_END
>
> -#define ENGINE_NODE_WITH_CLEAR_TRACK_DATA_IS_VALID(NAME, NAME_STR) \
> - ENGINE_NODE_DEF(NAME, NAME_STR, \
> - en_##NAME##_clear_tracked_data, \
> - en_##NAME##_is_valid)
> +#define CLEAR_TRACKED_DATA(NAME) \
> + .clear_tracked_data = en_##NAME##_clear_tracked_data
> +
> +#define IS_VALID(NAME) \
> + .is_valid = en_##NAME##_is_valid
> +
> +#define ENGINE_NODE3(NAME, NAME_STR, ARG1) \
> + ENGINE_NODE_DEF_START(NAME, #NAME) \
> + ARG1(NAME), \
> + ENGINE_NODE_DEF_END
> +
> +#define ENGINE_NODE4(NAME, NAME_STR, ARG1, ARG2) \
> + ENGINE_NODE_DEF_START(NAME, #NAME) \
> + ARG1(NAME), \
> + ARG2(NAME), \
> + ENGINE_NODE_DEF_END
> +
> +#define ENGINE_NODE(...) VFUNC(ENGINE_NODE, __VA_ARGS__)
>
> /* Macro to define member functions of an engine node which represents
> * a table of OVSDB */
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index 0fff9b463..de15757a9 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -496,4 +496,40 @@ const struct sbrec_port_binding *lport_lookup_by_name(
> struct ovsdb_idl_index *sbrec_port_binding_by_name,
> const char *name);
>
> +/* __NARG__ provides a way to count the number of arguments passed to a
> + * variadic macro. As defined below, it's capable of counting up to
> + * 16 arguments. This should be more than enough for our purposes. However
> + * the __ARG_N and __RSEQ_N macros can be updated to include more numbers
> + * if we wish to be able to count higher.
> + * */
> +#define __NARG__(...) __NARG_I_(__VA_ARGS__,__RSEQ_N())
> +#define __NARG_I_(...) __ARG_N(__VA_ARGS__)
> +#define __ARG_N( \
> + _1, _2, _3, _4, _5, _6, _7, _8, _9, \
> + _10, _11, _12, _13, _14, _15, _16, N,...) N
> +#define __RSEQ_N() \
> + 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0
> +
> +/* VFUNC provides a way to overload macros so that they can
> + * accept a variable number of arguments.
> + *
> + * It works by using __NARG__ to count the number of arguments
> + * that were passed into the macro. Then this is concatenated
> + * to the end of the macro name. Then this concatenated macro
> + * is called.
> + *
> + * As an example, let's say we have
> + * # define FOO(...) VFUNC(FOO, __VA_ARGS)
> + *
> + * Then if a caller were to call FOO(bar)
> + * then we would count one argument (bar), concatenate
> + * the 1 to the end of FOO, and end up calling FOO1(bar).
> + *
> + * If a caller were to call FOO(bar, baz), then the
> + * result would be to call FOO2(bar, baz).
> + */
> +#define _VFUNC_(name, n) name##n
> +#define _VFUNC(name, n) _VFUNC_(name, n)
> +#define VFUNC(func, ...) _VFUNC(func, __NARG__(__VA_ARGS__)) (__VA_ARGS__)
> +
> #endif /* OVN_UTIL_H */
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index 7f92c0cb7..823593e7e 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -147,7 +147,7 @@ enum sb_engine_node {
>
> /* Define engine nodes for other nodes. They should be defined as static
> to
> * avoid sparse errors. */
> -static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(northd, "northd");
> +static ENGINE_NODE(northd, "northd", CLEAR_TRACKED_DATA);
> static ENGINE_NODE(sync_from_sb, "sync_from_sb");
> static ENGINE_NODE(sampling_app, "sampling_app");
> static ENGINE_NODE(lflow, "lflow");
> @@ -157,16 +157,16 @@ static ENGINE_NODE(northd_output, "northd_output");
> static ENGINE_NODE(sync_meters, "sync_meters");
> static ENGINE_NODE(sync_to_sb, "sync_to_sb");
> static ENGINE_NODE(sync_to_sb_addr_set, "sync_to_sb_addr_set");
> -static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_group, "port_group");
> +static ENGINE_NODE(port_group, "port_group", CLEAR_TRACKED_DATA);
> static ENGINE_NODE(fdb_aging, "fdb_aging");
> static ENGINE_NODE(fdb_aging_waker, "fdb_aging_waker");
> static ENGINE_NODE(sync_to_sb_lb, "sync_to_sb_lb");
> static ENGINE_NODE(sync_to_sb_pb, "sync_to_sb_pb");
> -static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(global_config, "global_config");
> -static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lb_data, "lb_data");
> -static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lr_nat, "lr_nat");
> -static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lr_stateful, "lr_stateful");
> -static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ls_stateful, "ls_stateful");
> +static ENGINE_NODE(global_config, "global_config", CLEAR_TRACKED_DATA);
> +static ENGINE_NODE(lb_data, "lb_data", CLEAR_TRACKED_DATA);
> +static ENGINE_NODE(lr_nat, "lr_nat", CLEAR_TRACKED_DATA);
> +static ENGINE_NODE(lr_stateful, "lr_stateful", CLEAR_TRACKED_DATA);
> +static ENGINE_NODE(ls_stateful, "ls_stateful", CLEAR_TRACKED_DATA);
> static ENGINE_NODE(route_policies, "route_policies");
> static ENGINE_NODE(routes, "routes");
> static ENGINE_NODE(bfd, "bfd");
> @@ -175,10 +175,10 @@ static ENGINE_NODE(ecmp_nexthop, "ecmp_nexthop");
> static ENGINE_NODE(multicast_igmp, "multicast_igmp");
> static ENGINE_NODE(acl_id, "acl_id");
> static ENGINE_NODE(advertised_route_sync, "advertised_route_sync");
> -static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(learned_route_sync,
> - "learned_route_sync");
> +static ENGINE_NODE(learned_route_sync, "learned_route_sync",
> + CLEAR_TRACKED_DATA);
> static ENGINE_NODE(dynamic_routes, "dynamic_routes");
> -static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(group_ecmp_route,
> "group_ecmp_route");
> +static ENGINE_NODE(group_ecmp_route, "group_ecmp_route",
> CLEAR_TRACKED_DATA);
>
> void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> struct ovsdb_idl_loop *sb)
> --
> 2.47.0
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
With the checkpatch warning fixed:
Acked-by: Ales Musil <amu...@redhat.com>
Thanks,
Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
