On 3/12/25 5:38 PM, Ales Musil wrote:
> The objdep was adding "struct object_to_resources_list_node" to list
> that would be passed around and only used part would be the uuid.
> Create uuidset directly instead to avoid extra allocations of the
> intermediate struct.
>
> Signed-off-by: Ales Musil <[email protected]>
> ---
Hi Ales,
> controller/lflow.c | 21 ++++++---------------
> controller/lflow.h | 2 +-
> controller/ovn-controller.c | 15 +++++++--------
> lib/objdep.c | 10 +++-------
> lib/objdep.h | 2 +-
> 5 files changed, 18 insertions(+), 32 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 860869f55..0c6203fa1 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -748,7 +748,7 @@ done:
>
> bool
> lflow_handle_changed_ref(enum objdep_type type, const char *res_name,
> - struct ovs_list *objs_todo,
> + struct uuidset *objs_todo,
> const void *in_arg, void *out_arg)
> {
> struct lflow_ctx_in *l_ctx_in = CONST_CAST(struct lflow_ctx_in *,
> in_arg);
> @@ -756,22 +756,13 @@ lflow_handle_changed_ref(enum objdep_type type, const
> char *res_name,
>
> /* Re-parse the related lflows. */
> /* Firstly, flood remove the flows from desired flow table. */
> - struct object_to_resources_list_node *resource_list_node_uuid;
> - struct uuidset flood_remove_nodes =
> - UUIDSET_INITIALIZER(&flood_remove_nodes);
> - LIST_FOR_EACH_SAFE (resource_list_node_uuid, list_node, objs_todo) {
> - const struct uuid *obj_uuid = &resource_list_node_uuid->obj_uuid;
> - VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %s,"
> - " name: %s.",
> - UUID_ARGS(obj_uuid), objdep_type_name(type), res_name);
> - uuidset_insert(&flood_remove_nodes, obj_uuid);
> - free(resource_list_node_uuid);
> - }
> - ofctrl_flood_remove_flows(l_ctx_out->flow_table, &flood_remove_nodes);
> + ofctrl_flood_remove_flows(l_ctx_out->flow_table, objs_todo);
>
> /* Secondly, for each lflow that is actually removed, reprocessing it. */
> struct uuidset_node *ofrn;
> - UUIDSET_FOR_EACH (ofrn, &flood_remove_nodes) {
> + UUIDSET_FOR_EACH (ofrn, objs_todo) {
> + VLOG_DBG("Reprocess lflow "UUID_FMT" for resource type: %s, name:
> %s.",
> + UUID_ARGS(&ofrn->uuid), objdep_type_name(type), res_name);
> objdep_mgr_remove_obj(l_ctx_out->lflow_deps_mgr, &ofrn->uuid);
> lflow_conj_ids_free(l_ctx_out->conj_ids, &ofrn->uuid);
>
> @@ -798,7 +789,7 @@ lflow_handle_changed_ref(enum objdep_type type, const
> char *res_name,
>
> consider_logical_flow(lflow, false, l_ctx_in, l_ctx_out);
> }
Nit: I'd add a newline here to match the lb_data_handle_changed_ref()
handler style.
> - uuidset_destroy(&flood_remove_nodes);
> + uuidset_destroy(objs_todo);
> return true;
> }
>
> diff --git a/controller/lflow.h b/controller/lflow.h
> index ab026e3bd..fa99173e6 100644
> --- a/controller/lflow.h
> +++ b/controller/lflow.h
> @@ -169,7 +169,7 @@ bool lflow_handle_addr_set_update(const char *as_name,
> struct addr_set_diff *,
> struct lflow_ctx_out *,
> bool *changed);
> bool lflow_handle_changed_ref(enum objdep_type, const char *res_name,
> - struct ovs_list *objs_todo,
> + struct uuidset *objs_todo,
> const void *in_arg, void *out_arg);
>
> void lflow_handle_changed_mac_bindings(
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index c0f167c54..8fa32635f 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2755,15 +2755,15 @@ lb_data_local_lb_remove(struct ed_type_lb_data
> *lb_data,
>
> static bool
> lb_data_handle_changed_ref(enum objdep_type type, const char *res_name,
> - struct ovs_list *objs_todo, const void *in_arg,
> + struct uuidset *objs_todo, const void *in_arg,
> void *out_arg)
> {
> const struct lb_data_ctx_in *ctx_in = in_arg;
> struct ed_type_lb_data *lb_data = out_arg;
>
> - struct object_to_resources_list_node *resource_lb_uuid;
> - LIST_FOR_EACH_POP (resource_lb_uuid, list_node, objs_todo) {
> - struct uuid *uuid = &resource_lb_uuid->obj_uuid;
> + struct uuidset_node *ofrn;
> + UUIDSET_FOR_EACH (ofrn, objs_todo) {
> + struct uuid *uuid = &ofrn->uuid;
>
> VLOG_DBG("Reprocess LB "UUID_FMT" for resource type: %s, name: %s",
> UUID_ARGS(uuid), objdep_type_name(type), res_name);
> @@ -2771,7 +2771,6 @@ lb_data_handle_changed_ref(enum objdep_type type, const
> char *res_name,
> struct ovn_controller_lb *lb =
> ovn_controller_lb_find(&lb_data->local_lbs, uuid);
> if (!lb) {
> - free(resource_lb_uuid);
> continue;
> }
>
> @@ -2780,14 +2779,14 @@ lb_data_handle_changed_ref(enum objdep_type type,
> const char *res_name,
> const struct sbrec_load_balancer *sbrec_lb =
> sbrec_load_balancer_table_get_for_uuid(ctx_in->lb_table, uuid);
> if (!lb_is_local(sbrec_lb, ctx_in->local_datapaths)) {
> - free(resource_lb_uuid);
> continue;
> }
>
> lb_data_local_lb_add(lb_data, sbrec_lb, ctx_in->template_vars, true);
> -
> - free(resource_lb_uuid);
> }
> +
> + uuidset_destroy(objs_todo);
> +
Nit: no need for the empty line.
> return true;
> }
>
> diff --git a/lib/objdep.c b/lib/objdep.c
> index 06cf126f1..00f762b8e 100644
> --- a/lib/objdep.c
> +++ b/lib/objdep.c
> @@ -214,8 +214,7 @@ objdep_mgr_handle_change(struct objdep_mgr *mgr,
> " name: %s.", objdep_type_name(type), res_name);
> *changed = false;
>
> - struct ovs_list objs_todo = OVS_LIST_INITIALIZER(&objs_todo);
> -
> + struct uuidset objs_todo = UUIDSET_INITIALIZER(&objs_todo);
> struct object_to_resources_list_node *resource_list_node;
> HMAP_FOR_EACH (resource_list_node, hmap_node, &resource_node->objs) {
> if (uuidset_find(objs_processed, &resource_list_node->obj_uuid)) {
> @@ -223,12 +222,9 @@ objdep_mgr_handle_change(struct objdep_mgr *mgr,
> }
> /* Use object_to_resources_list_node as list node to store the uuid.
> * Other fields are not used here. */
> - struct object_to_resources_list_node *resource_list_node_uuid =
> - xmalloc(sizeof *resource_list_node_uuid);
> - resource_list_node_uuid->obj_uuid = resource_list_node->obj_uuid;
> - ovs_list_push_back(&objs_todo, &resource_list_node_uuid->list_node);
> + uuidset_insert(&objs_todo, &resource_list_node->obj_uuid);
> }
> - if (ovs_list_is_empty(&objs_todo)) {
> + if (uuidset_is_empty(&objs_todo)) {
> return true;
> }
> *changed = true;
> diff --git a/lib/objdep.h b/lib/objdep.h
> index 1ea781947..e25ae7f31 100644
> --- a/lib/objdep.h
> +++ b/lib/objdep.h
> @@ -35,7 +35,7 @@ enum objdep_type {
> * handled successfully. */
> typedef bool (*objdep_change_handler)(enum objdep_type,
> const char *res_name,
> - struct ovs_list *ref_nodes,
> + struct uuidset *ref_nodes,
> const void *in_arg, void *out_arg);
>
> /* A node pointing to all objects that refer to a given resource. */
I took care of the two nits and applied the patch to main.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
