On Wed, Apr 30, 2025 at 7:54 AM Ales Musil <amu...@redhat.com> wrote:
> The vector has the same functionality as the x2nrealloc
> for insert. Use vector instead which slightly simplifies
> the final code.
>
> Signed-off-by: Ales Musil <amu...@redhat.com>
> Acked-by: Mark Michelson <mmich...@redhat.com>
> ---
> v4: Rebase on top of current main.
> Add ack from Mark.
> v3: Rebase on top of current main.
> v2: Rebase on top of current main.
> ---
> controller/binding.c | 4 +--
> controller/local_data.c | 62 +++++++++++++++++------------------
> controller/local_data.h | 15 +++++----
> controller/ovn-controller.c | 19 ++++-------
> controller/physical.c | 18 +++++-----
> controller/pinctrl.c | 12 ++++---
> controller/route.c | 8 ++---
> controller/test-lflow-cache.c | 27 +++++++--------
> 8 files changed, 79 insertions(+), 86 deletions(-)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index f7535051f..9aec6708f 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -738,10 +738,10 @@ update_ld_peers(const struct sbrec_port_binding *pb,
> * remove it from the ld peers list.
> */
> enum en_lport_type type = get_lport_type(pb);
> - int num_peers = ld->n_peer_ports;
> + size_t num_peers = vector_len(&ld->peer_ports);
> if (type != LP_PATCH) {
> remove_local_datapath_peer_port(pb, ld, local_datapaths);
> - if (num_peers != ld->n_peer_ports) {
> + if (num_peers != vector_len(&ld->peer_ports)) {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> VLOG_DBG_RL(&rl,
> "removing lport %s from the ld peers list",
> diff --git a/controller/local_data.c b/controller/local_data.c
> index e256de5fa..adda3b859 100644
> --- a/controller/local_data.c
> +++ b/controller/local_data.c
> @@ -86,6 +86,7 @@ local_datapath_alloc(const struct sbrec_datapath_binding
> *dp)
> ld->datapath = dp;
> ld->is_switch = datapath_is_switch(dp);
> ld->is_transit_switch = datapath_is_transit_switch(dp);
> + ld->peer_ports = VECTOR_EMPTY_INITIALIZER(struct peer_ports);
> shash_init(&ld->external_ports);
> shash_init(&ld->multichassis_ports);
> /* memory accounting - common part. */
> @@ -121,10 +122,9 @@ local_datapath_destroy(struct local_datapath *ld)
> local_datapath_usage -= (shash_count(&ld->multichassis_ports)
> * sizeof *node);
> local_datapath_usage -= sizeof *ld;
> - local_datapath_usage -=
> - ld->n_allocated_peer_ports * sizeof *ld->peer_ports;
> + local_datapath_usage -= vector_memory_usage(&ld->peer_ports);
>
> - free(ld->peer_ports);
> + vector_destroy(&ld->peer_ports);
> shash_destroy(&ld->external_ports);
> shash_destroy(&ld->multichassis_ports);
> free(ld);
> @@ -258,10 +258,11 @@ local_data_dump_peer_ports(struct hmap
> *local_datapaths, struct ds *peer_ports)
> HMAP_FOR_EACH (ld, hmap_node, local_datapaths) {
> const char *name = smap_get_def(&ld->datapath->external_ids,
> "name",
> "unknown");
> - for (size_t i = 0; i < ld->n_peer_ports; i++) {
> + struct peer_ports peers;
> + VECTOR_FOR_EACH (&ld->peer_ports, peers) {
> ds_put_format(peer_ports, "dp %s : local = %s, remote = %s\n",
> - name, ld->peer_ports[i].local->logical_port,
> - ld->peer_ports[i].remote->logical_port);
> + name, peers.local->logical_port,
> + peers.remote->logical_port);
> }
> }
> }
> @@ -272,25 +273,26 @@ remove_local_datapath_peer_port(const struct
> sbrec_port_binding *pb,
> struct hmap *local_datapaths)
> {
> size_t i = 0;
> - for (i = 0; i < ld->n_peer_ports; i++) {
> - if (ld->peer_ports[i].local == pb) {
> + const struct peer_ports *peers;
> + VECTOR_FOR_EACH_PTR (&ld->peer_ports, peers) {
> + if (peers->local == pb) {
> break;
> }
> + i++;
> }
>
> - if (i == ld->n_peer_ports) {
> + struct peer_ports removed;
> + if (!vector_remove_fast(&ld->peer_ports, i, &removed)) {
> return;
> }
>
> - const struct sbrec_port_binding *peer = ld->peer_ports[i].remote;
> -
> - /* Possible improvement: We can shrink the allocated peer ports
> - * if (ld->n_peer_ports < ld->n_allocated_peer_ports / 2).
> - */
> - ld->peer_ports[i].local = ld->peer_ports[ld->n_peer_ports - 1].local;
> - ld->peer_ports[i].remote = ld->peer_ports[ld->n_peer_ports -
> 1].remote;
> - ld->n_peer_ports--;
> + if (vector_len(&ld->peer_ports) < vector_capacity(&ld->peer_ports) /
> 2) {
> + local_datapath_usage -= vector_memory_usage(&ld->peer_ports);
> + vector_shrink_to_fit(&ld->peer_ports);
> + local_datapath_usage += vector_memory_usage(&ld->peer_ports);
> + }
>
> + const struct sbrec_port_binding *peer = removed.remote;
> struct local_datapath *peer_ld =
> get_local_datapath(local_datapaths, peer->datapath->tunnel_key);
> if (peer_ld) {
> @@ -679,24 +681,20 @@ local_datapath_peer_port_add(struct local_datapath
> *ld,
> const struct sbrec_port_binding *local,
> const struct sbrec_port_binding *remote)
> {
> - for (size_t i = 0; i < ld->n_peer_ports; i++) {
> - if (ld->peer_ports[i].local == local) {
> + const struct peer_ports *ptr;
> + VECTOR_FOR_EACH_PTR (&ld->peer_ports, ptr) {
> + if (ptr->local == local) {
> return;
> }
> }
> - ld->n_peer_ports++;
> - if (ld->n_peer_ports > ld->n_allocated_peer_ports) {
> - size_t old_n_ports = ld->n_allocated_peer_ports;
> - ld->peer_ports =
> - x2nrealloc(ld->peer_ports,
> - &ld->n_allocated_peer_ports,
> - sizeof *ld->peer_ports);
> - local_datapath_usage +=
> - (ld->n_allocated_peer_ports - old_n_ports) *
> - sizeof *ld->peer_ports;
> - }
> - ld->peer_ports[ld->n_peer_ports - 1].local = local;
> - ld->peer_ports[ld->n_peer_ports - 1].remote = remote;
> +
> + local_datapath_usage -= vector_memory_usage(&ld->peer_ports);
> + struct peer_ports peers = (struct peer_ports) {
> + .local = local,
> + .remote = remote,
> + };
> + vector_push(&ld->peer_ports, &peers);
> + local_datapath_usage += vector_memory_usage(&ld->peer_ports);
> }
>
> static bool
> diff --git a/controller/local_data.h b/controller/local_data.h
> index 19d558232..841829f2e 100644
> --- a/controller/local_data.h
> +++ b/controller/local_data.h
> @@ -23,6 +23,7 @@
>
> /* OVN includes. */
> #include "lib/ovn-util.h"
> +#include "vec.h"
>
> struct sbrec_datapath_binding;
> struct sbrec_port_binding;
> @@ -32,6 +33,11 @@ struct ovsrec_bridge;
> struct ovsrec_interface_table;
> struct sbrec_load_balancer;
>
> +struct peer_ports {
> + const struct sbrec_port_binding *local;
> + const struct sbrec_port_binding *remote;
> +};
> +
> /* A logical datapath that has some relevance to this hypervisor. A
> logical
> * datapath D is relevant to hypervisor H if:
> *
> @@ -53,13 +59,8 @@ struct local_datapath {
> /* The vtep port in this datapath, if any (at most one is allowed). */
> const struct sbrec_port_binding *vtep_port;
>
> - struct {
> - const struct sbrec_port_binding *local;
> - const struct sbrec_port_binding *remote;
> - } *peer_ports;
> -
> - size_t n_peer_ports;
> - size_t n_allocated_peer_ports;
> + /* Vector of struct peer_ports. */
> + struct vector peer_ports;
>
> struct shash external_ports;
> struct shash multichassis_ports;
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 37406f923..75b1ac750 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -2522,9 +2522,7 @@ en_mff_ovn_geneve_run(struct engine_node *node, void
> *data)
> struct load_balancers_by_dp {
> struct hmap_node node;
> const struct sbrec_datapath_binding *dp;
> - const struct sbrec_load_balancer **dp_lbs;
> - size_t n_allocated_dp_lbs;
> - size_t n_dp_lbs;
> + struct vector dp_lbs; /* Vector of const struct sbrec_load_balancer
> *. */
> };
>
> static struct load_balancers_by_dp *
> @@ -2534,6 +2532,8 @@ load_balancers_by_dp_create(struct hmap *lbs,
> struct load_balancers_by_dp *lbs_by_dp = xzalloc(sizeof *lbs_by_dp);
>
> lbs_by_dp->dp = dp;
> + lbs_by_dp->dp_lbs =
> + VECTOR_EMPTY_INITIALIZER(const struct sbrec_load_balancer *);
> hmap_insert(lbs, &lbs_by_dp->node, hash_uint64(dp->tunnel_key));
> return lbs_by_dp;
> }
> @@ -2545,7 +2545,7 @@ load_balancers_by_dp_destroy(struct
> load_balancers_by_dp *lbs_by_dp)
> return;
> }
>
> - free(lbs_by_dp->dp_lbs);
> + vector_destroy(&lbs_by_dp->dp_lbs);
> free(lbs_by_dp);
> }
>
> @@ -2583,12 +2583,7 @@ load_balancers_by_dp_add_one(const struct hmap
> *local_datapaths,
> lbs_by_dp = load_balancers_by_dp_create(lbs, ldp->datapath);
> }
>
> - if (lbs_by_dp->n_dp_lbs == lbs_by_dp->n_allocated_dp_lbs) {
> - lbs_by_dp->dp_lbs = x2nrealloc(lbs_by_dp->dp_lbs,
> - &lbs_by_dp->n_allocated_dp_lbs,
> - sizeof *lbs_by_dp->dp_lbs);
> - }
> - lbs_by_dp->dp_lbs[lbs_by_dp->n_dp_lbs++] = lb;
> + vector_push(&lbs_by_dp->dp_lbs, &lb);
> }
>
> /* Builds and returns a hmap of 'load_balancers_by_dp', one record for
> each
> @@ -3005,8 +3000,8 @@ lb_data_runtime_data_handler(struct engine_node
> *node, void *data OVS_UNUSED)
> continue;
> }
>
> - for (size_t i = 0; i < lbs_by_dp->n_dp_lbs; i++) {
> - const struct sbrec_load_balancer *sbrec_lb =
> lbs_by_dp->dp_lbs[i];
> + const struct sbrec_load_balancer *sbrec_lb;
> + VECTOR_FOR_EACH (&lbs_by_dp->dp_lbs, sbrec_lb) {
> struct ovn_controller_lb *lb =
> ovn_controller_lb_find(&lb_data->local_lbs,
> &sbrec_lb->header_.uuid);
> diff --git a/controller/physical.c b/controller/physical.c
> index b26d12d3c..a17d792c3 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -347,9 +347,9 @@ put_remote_port_redirect_bridged(const struct
> }
>
> uint32_t ls_dp_key = 0;
> - for (int i = 0; i < ld->n_peer_ports; i++) {
> - const struct sbrec_port_binding *sport_binding =
> - ld->peer_ports[i].remote;
> + const struct peer_ports *peers;
> + VECTOR_FOR_EACH_PTR (&ld->peer_ports, peers) {
> + const struct sbrec_port_binding *sport_binding =
> peers->remote;
> const char *sport_peer_name =
> smap_get(&sport_binding->options, "peer");
> const char *distributed_port =
> @@ -734,9 +734,9 @@ put_replace_chassis_mac_flows(const struct shash
> *ct_zones,
> int tag = localnet_port->tag ? *localnet_port->tag : 0;
> struct zone_ids zone_ids = get_zone_ids(localnet_port, ct_zones);
>
> - for (int i = 0; i < ld->n_peer_ports; i++) {
> - const struct sbrec_port_binding *rport_binding =
> - ld->peer_ports[i].remote;
> + const struct peer_ports *peers;
> + VECTOR_FOR_EACH_PTR (&ld->peer_ports, peers) {
> + const struct sbrec_port_binding *rport_binding = peers->remote;
> struct eth_addr router_port_mac;
> char *err_str = NULL;
> struct match match;
> @@ -881,9 +881,9 @@ put_replace_router_port_mac_flows(const struct
> physical_ctx *ctx,
> return;
> }
>
> - for (int i = 0; i < ld->n_peer_ports; i++) {
> - const struct sbrec_port_binding *rport_binding =
> - ld->peer_ports[i].remote;
> + const struct peer_ports *peers;
> + VECTOR_FOR_EACH_PTR (&ld->peer_ports, peers) {
> + const struct sbrec_port_binding *rport_binding = peers->remote;
> struct eth_addr router_port_mac;
> struct match match;
> struct ofpact_mac *replace_mac;
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index a269e5375..34fb2e6bf 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -1356,8 +1356,9 @@ fill_ipv6_prefix_state(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> {
> bool changed = false;
>
> - for (size_t i = 0; i < ld->n_peer_ports; i++) {
> - const struct sbrec_port_binding *pb = ld->peer_ports[i].local;
> + const struct peer_ports *peers;
> + VECTOR_FOR_EACH_PTR (&ld->peer_ports, peers) {
> + const struct sbrec_port_binding *pb = peers->local;
> struct ipv6_prefixd_state *pfd;
>
> if (!smap_get_bool(&pb->options, "ipv6_prefix", false)) {
> @@ -4814,9 +4815,10 @@ send_garp_locally(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> get_local_datapath(local_datapaths, in_pb->datapath->tunnel_key);
>
> ovs_assert(ldp);
> - for (size_t i = 0; i < ldp->n_peer_ports; i++) {
> - const struct sbrec_port_binding *local = ldp->peer_ports[i].local;
> - const struct sbrec_port_binding *remote =
> ldp->peer_ports[i].remote;
> + const struct peer_ports *peers;
> + VECTOR_FOR_EACH_PTR (&ldp->peer_ports, peers) {
> + const struct sbrec_port_binding *local = peers->local;
> + const struct sbrec_port_binding *remote = peers->remote;
>
> /* Skip "ingress" port. */
> if (local == in_pb) {
> diff --git a/controller/route.c b/controller/route.c
> index 55d3e0ae3..4e5862529 100644
> --- a/controller/route.c
> +++ b/controller/route.c
> @@ -158,7 +158,7 @@ route_run(struct route_ctx_in *r_ctx_in,
> build_port_mapping(&port_mapping,
> r_ctx_in->dynamic_routing_port_mapping);
>
> HMAP_FOR_EACH (ld, hmap_node, r_ctx_in->local_datapaths) {
> - if (!ld->n_peer_ports || ld->is_switch) {
> + if (vector_is_empty(&ld->peer_ports) || ld->is_switch) {
> continue;
> }
>
> @@ -169,9 +169,9 @@ route_run(struct route_ctx_in *r_ctx_in,
>
> /* This is a LR datapath, find LRPs with route exchange options
> * that are bound locally. */
> - for (size_t i = 0; i < ld->n_peer_ports; i++) {
> - const struct sbrec_port_binding *local_peer
> - = ld->peer_ports[i].local;
> + const struct peer_ports *peers;
> + VECTOR_FOR_EACH_PTR (&ld->peer_ports, peers) {
> + const struct sbrec_port_binding *local_peer = peers->local;
> const struct sbrec_port_binding *repb =
>
> route_exchange_find_port(r_ctx_in->sbrec_port_binding_by_name,
> r_ctx_in->chassis,
> diff --git a/controller/test-lflow-cache.c b/controller/test-lflow-cache.c
> index 7ce999360..6f29abce9 100644
> --- a/controller/test-lflow-cache.c
> +++ b/controller/test-lflow-cache.c
> @@ -20,6 +20,7 @@
> #include "tests/ovstest.h"
> #include "tests/test-utils.h"
> #include "util.h"
> +#include "vec.h"
>
> #include "lflow-cache.h"
>
> @@ -111,9 +112,7 @@ test_lflow_cache_operations(struct ovs_cmdl_context
> *ctx)
> struct lflow_cache *lc = lflow_cache_create();
> struct expr *e = expr_create_boolean(true);
> bool enabled = !strcmp(ctx->argv[1], "true");
> - struct uuid *lflow_uuids = NULL;
> - size_t n_allocated_lflow_uuids = 0;
> - size_t n_lflow_uuids = 0;
> + struct vector lflow_uuids = VECTOR_EMPTY_INITIALIZER(struct uuid);
> unsigned int shift = 2;
> unsigned int n_ops;
>
> @@ -152,16 +151,13 @@ test_lflow_cache_operations(struct ovs_cmdl_context
> *ctx)
> goto done;
> }
>
> - if (n_lflow_uuids == n_allocated_lflow_uuids) {
> - lflow_uuids = x2nrealloc(lflow_uuids,
> &n_allocated_lflow_uuids,
> - sizeof *lflow_uuids);
> - }
> - struct uuid *lflow_uuid = &lflow_uuids[n_lflow_uuids++];
> + struct uuid lflow_uuid;
> + uuid_generate(&lflow_uuid);
> + vector_push(&lflow_uuids, &lflow_uuid);
>
> - uuid_generate(lflow_uuid);
> - test_lflow_cache_add__(lc, op_type, lflow_uuid, conj_id_ofs,
> + test_lflow_cache_add__(lc, op_type, &lflow_uuid, conj_id_ofs,
> n_conjs, e);
> - test_lflow_cache_lookup__(lc, lflow_uuid);
> + test_lflow_cache_lookup__(lc, &lflow_uuid);
> } else if (!strcmp(op, "add-del")) {
> const char *op_type = test_read_value(ctx, shift++,
> "op_type");
> if (!op_type) {
> @@ -188,9 +184,10 @@ test_lflow_cache_operations(struct ovs_cmdl_context
> *ctx)
> test_lflow_cache_delete__(lc, &lflow_uuid);
> test_lflow_cache_lookup__(lc, &lflow_uuid);
> } else if (!strcmp(op, "del")) {
> - ovs_assert(n_lflow_uuids);
> - test_lflow_cache_delete__(lc, &lflow_uuids[n_lflow_uuids -
> 1]);
> - n_lflow_uuids--;
> + ovs_assert(!vector_is_empty(&lflow_uuids));
> + struct uuid lflow_uuid;
> + vector_pop(&lflow_uuids, &lflow_uuid);
> + test_lflow_cache_delete__(lc, &lflow_uuid);
> } else if (!strcmp(op, "enable")) {
> unsigned int limit;
> unsigned int mem_limit_kb;
> @@ -236,7 +233,7 @@ test_lflow_cache_operations(struct ovs_cmdl_context
> *ctx)
> }
> done:
> lflow_cache_destroy(lc);
> - free(lflow_uuids);
> + vector_destroy(&lflow_uuids);
> expr_destroy(e);
> }
>
> --
> 2.49.0
>
>
Recheck-request: github-robot
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
