On Tue, Apr 29, 2025 at 5:59 PM Mark Michelson <mmich...@redhat.com> wrote:
> Hi Ales,
>
> I have a couple of notes below.
>
Hi Mark,
thank you for the review.
> On 4/28/25 05:34, Ales Musil via dev wrote:
> > The statistics were stored as allocated object for each individual
> > statistic and linked together via list. Store them in vector instead
> > which allows us to avoid a lot of small allocations and free right
> > after using the statistic.
> >
> > The same applies to the buffered packets, use vector instead of moving
> > around single allocated objects. This should reduce the fragmentation.
> >
> > Signed-off-by: Ales Musil <amu...@redhat.com>
> > ---
> > v3: Rebase on top of current main.
> > v2: Rebase on top of current main.
> > ---
> > controller/mac-cache.c | 190 +++++++++++++++++------------------------
> > controller/mac-cache.h | 41 +++++----
> > controller/pinctrl.c | 13 ++-
> > controller/statctrl.c | 53 ++++++------
> > 4 files changed, 139 insertions(+), 158 deletions(-)
> >
> > diff --git a/controller/mac-cache.c b/controller/mac-cache.c
> > index 18236c440..304304bee 100644
> > --- a/controller/mac-cache.c
> > +++ b/controller/mac-cache.c
> > @@ -18,6 +18,7 @@
> >
> > #include "lflow.h"
> > #include "lib/mac-binding-index.h"
> > +#include "lib/vec.h"
> > #include "local_data.h"
> > #include "lport.h"
> > #include "mac-cache.h"
> > @@ -339,43 +340,30 @@ fdbs_clear(struct hmap *map)
> > }
> > }
> >
> > -struct mac_cache_stats {
> > - struct ovs_list list_node;
> > -
> > - int64_t idle_age_ms;
> > -
> > - union {
> > - /* Common data to identify MAC binding. */
> > - struct mac_binding_data mb;
> > - /* Common data to identify FDB. */
> > - struct fdb_data fdb;
> > - } data;
> > -};
> > -
> > /* MAC binding stat processing. */
> > void
> > -mac_binding_stats_process_flow_stats(struct ovs_list *stats_list,
> > +mac_binding_stats_process_flow_stats(struct vector *stats_vec,
> > struct ofputil_flow_stats
> *ofp_stats)
> > {
> > - struct mac_cache_stats *stats = xmalloc(sizeof *stats);
> > -
> > - stats->idle_age_ms = ofp_stats->idle_age * 1000;
> > - stats->data.mb = (struct mac_binding_data) {
> > - .cookie = ntohll(ofp_stats->cookie),
> > - /* The port_key must be zero to match
> mac_binding_data_from_sbrec. */
> > - .port_key = 0,
> > - .dp_key = ntohll(ofp_stats->match.flow.metadata),
> > - .mac = ofp_stats->match.flow.dl_src
> > + struct mac_cache_stats stats = (struct mac_cache_stats) {
> > + .idle_age_ms = ofp_stats->idle_age * 1000,
> > + .data.mb = (struct mac_binding_data) {
> > + .cookie = ntohll(ofp_stats->cookie),
> > + /* The port_key must be zero to match
> > + * mac_binding_data_from_sbrec. */
> > + .port_key = 0,
> > + .dp_key = ntohll(ofp_stats->match.flow.metadata),
> > + .mac = ofp_stats->match.flow.dl_src
> > + },
> > };
> >
> > if (ofp_stats->match.flow.dl_type == htons(ETH_TYPE_IP) ||
> > ofp_stats->match.flow.dl_type == htons(ETH_TYPE_ARP)) {
> > - stats->data.mb.ip =
> in6_addr_mapped_ipv4(ofp_stats->match.flow.nw_src);
> > + stats.data.mb.ip =
> in6_addr_mapped_ipv4(ofp_stats->match.flow.nw_src);
> > } else {
> > - stats->data.mb.ip = ofp_stats->match.flow.ipv6_src;
> > + stats.data.mb.ip = ofp_stats->match.flow.ipv6_src;
> > }
> > -
> > - ovs_list_push_back(stats_list, &stats->list_node);
> > + vector_push(stats_vec, &stats);
> > }
> >
> > static void
> > @@ -409,19 +397,18 @@ void
> > mac_binding_stats_run(
> > struct rconn *swconn OVS_UNUSED,
> > struct ovsdb_idl_index *sbrec_port_binding_by_name OVS_UNUSED,
> > - struct ovs_list *stats_list, uint64_t *req_delay, void *data)
> > + struct vector *stats_vec, uint64_t *req_delay, void *data)
> > {
> > struct mac_cache_data *cache_data = data;
> > long long timewall_now = time_wall_msec();
> >
> > struct mac_cache_stats *stats;
> > - LIST_FOR_EACH_POP (stats, list_node, stats_list) {
> > + VECTOR_FOR_EACH_PTR (stats_vec, stats) {
> > struct mac_binding *mb =
> mac_binding_find(&cache_data->mac_bindings,
> > &stats->data.mb);
> > if (!mb) {
> > mac_binding_update_log("Not found in the cache:",
> &stats->data.mb,
> > false, NULL, 0, 0);
> > - free(stats);
> > continue;
> > }
> >
> > @@ -449,7 +436,6 @@ mac_binding_stats_run(
> > threshold, stats->idle_age_ms,
> > since_updated_ms);
> > }
> > - free(stats);
> > }
> >
> > mac_cache_update_req_delay(&cache_data->thresholds, req_delay);
> > @@ -460,19 +446,18 @@ mac_binding_stats_run(
> >
> > /* FDB stat processing. */
> > void
> > -fdb_stats_process_flow_stats(struct ovs_list *stats_list,
> > +fdb_stats_process_flow_stats(struct vector *stats_vec,
> > struct ofputil_flow_stats *ofp_stats)
> > {
> > - struct mac_cache_stats *stats = xmalloc(sizeof *stats);
> > -
> > - stats->idle_age_ms = ofp_stats->idle_age * 1000;
> > - stats->data.fdb = (struct fdb_data) {
> > + struct mac_cache_stats stats = (struct mac_cache_stats) {
> > + .idle_age_ms = ofp_stats->idle_age * 1000,
> > + .data.fdb = (struct fdb_data) {
> > .port_key = ofp_stats->match.flow.regs[MFF_LOG_INPORT -
> MFF_REG0],
> > .dp_key = ntohll(ofp_stats->match.flow.metadata),
> > .mac = ofp_stats->match.flow.dl_src
> > + },
> > };
> > -
> > - ovs_list_push_back(stats_list, &stats->list_node);
> > + vector_push(stats_vec, &stats);
> > }
> >
> > static void
> > @@ -506,20 +491,19 @@ fdb_update_log(const char *action,
> > void
> > fdb_stats_run(struct rconn *swconn OVS_UNUSED,
> > struct ovsdb_idl_index *sbrec_port_binding_by_name
> OVS_UNUSED,
> > - struct ovs_list *stats_list,
> > + struct vector *stats_vec,
> > uint64_t *req_delay, void *data)
> > {
> > struct mac_cache_data *cache_data = data;
> > long long timewall_now = time_wall_msec();
> >
> > struct mac_cache_stats *stats;
> > - LIST_FOR_EACH_POP (stats, list_node, stats_list) {
> > + VECTOR_FOR_EACH_PTR (stats_vec, stats) {
> > struct fdb *fdb = fdb_find(&cache_data->fdbs,
> &stats->data.fdb);
> >
> > if (!fdb) {
> > fdb_update_log("Not found in the cache:", &stats->data.fdb,
> > false, NULL, 0, 0);
> > - free(stats);
> > continue;
> > }
> >
> > @@ -546,8 +530,6 @@ fdb_stats_run(struct rconn *swconn OVS_UNUSED,
> > fdb_update_log("Not updating non-active", &fdb->data, true,
> > threshold, stats->idle_age_ms,
> since_updated_ms);
> > }
> > -
> > - free(stats);
> > }
> >
> > mac_cache_update_req_delay(&cache_data->thresholds, req_delay);
> > @@ -557,34 +539,10 @@ fdb_stats_run(struct rconn *swconn OVS_UNUSED,
> > }
> >
> > /* Packet buffering. */
> > -struct bp_packet_data *
> > -bp_packet_data_create(const struct ofputil_packet_in *pin,
> > - const struct ofpbuf *continuation) {
> > - struct bp_packet_data *pd = xmalloc(sizeof *pd);
> > -
> > - pd->pin = (struct ofputil_packet_in) {
> > - .packet = xmemdup(pin->packet, pin->packet_len),
> > - .packet_len = pin->packet_len,
> > - .flow_metadata = pin->flow_metadata,
> > - .reason = pin->reason,
> > - .table_id = pin->table_id,
> > - .cookie = pin->cookie,
> > - /* Userdata are empty on purpose,
> > - * it is not needed for the continuation. */
> > - .userdata = NULL,
> > - .userdata_len = 0,
> > - };
> > - pd->continuation = ofpbuf_clone(continuation);
> > -
> > - return pd;
> > -}
> > -
> > -
> > void
> > bp_packet_data_destroy(struct bp_packet_data *pd) {
> > free(pd->pin.packet);
> > ofpbuf_delete(pd->continuation);
> > - free(pd);
> > }
> >
> > struct buffered_packets *
> > @@ -604,7 +562,8 @@ buffered_packets_add(struct buffered_packets_ctx
> *ctx,
> > /* Schedule the freshly added buffered packet to do lookup
> > * immediately. */
> > bp->lookup_at_ms = 0;
> > - ovs_list_init(&bp->queue);
> > + bp->queue = VECTOR_CAPACITY_INITIALIZER(struct bp_packet_data,
> > + BUFFER_QUEUE_DEPTH);
> > }
> >
> > bp->expire_at_ms = time_msec() + BUFFERED_PACKETS_TIMEOUT_MS;
> > @@ -614,14 +573,32 @@ buffered_packets_add(struct buffered_packets_ctx
> *ctx,
> >
> > void
> > buffered_packets_packet_data_enqueue(struct buffered_packets *bp,
> > - struct bp_packet_data *pd) {
> > - if (ovs_list_size(&bp->queue) == BUFFER_QUEUE_DEPTH) {
> > - struct bp_packet_data *p =
> CONTAINER_OF(ovs_list_pop_front(&bp->queue),
> > - struct bp_packet_data,
> node);
> > -
> > - bp_packet_data_destroy(p);
> > + const struct ofputil_packet_in
> *pin,
> > + const struct ofpbuf *continuation)
> > +{
> > + if (vector_len(&bp->queue) == BUFFER_QUEUE_DEPTH) {
> > + struct bp_packet_data pd;
> > + vector_remove(&bp->queue, 0, &pd);
> > + bp_packet_data_destroy(&pd);
> > }
> > - ovs_list_push_back(&bp->queue, &pd->node);
> > +
> > + struct bp_packet_data pd = (struct bp_packet_data) {
> > + .pin = (struct ofputil_packet_in) {
> > + .packet = xmemdup(pin->packet, pin->packet_len),
> > + .packet_len = pin->packet_len,
> > + .flow_metadata = pin->flow_metadata,
> > + .reason = pin->reason,
> > + .table_id = pin->table_id,
> > + .cookie = pin->cookie,
> > + /* Userdata are empty on purpose,
> > + * it is not needed for the continuation. */
> > + .userdata = NULL,
> > + .userdata_len = 0,
> > + },
> > + .continuation = ofpbuf_clone(continuation),
> > + };
> > +
> > + vector_push(&bp->queue, &pd);
> > }
> >
> > void
> > @@ -661,16 +638,18 @@ buffered_packets_ctx_run(struct
> buffered_packets_ctx *ctx,
> > }
> >
> > struct bp_packet_data *pd;
> > - LIST_FOR_EACH_POP (pd, node, &bp->queue) {
> > + VECTOR_FOR_EACH_PTR (&bp->queue, pd) {
> > struct dp_packet packet;
> > dp_packet_use_const(&packet, pd->pin.packet,
> pd->pin.packet_len);
> >
> > struct eth_header *eth = dp_packet_data(&packet);
> > eth->eth_dst = mac;
> > -
> > - ovs_list_push_back(&ctx->ready_packets_data, &pd->node);
> > }
> >
> > + vector_push_array(&ctx->ready_packets_data,
> > + vector_get_array(&bp->queue),
> > + vector_len(&bp->queue));
> > + vector_clear(&bp->queue);
> > buffered_packets_remove(ctx, bp);
> > }
> >
> > @@ -679,7 +658,7 @@ buffered_packets_ctx_run(struct buffered_packets_ctx
> *ctx,
> >
> > bool
> > buffered_packets_ctx_is_ready_to_send(struct buffered_packets_ctx
> *ctx) {
> > - return !ovs_list_is_empty(&ctx->ready_packets_data);
> > + return !vector_is_empty(&ctx->ready_packets_data);
> > }
> >
> > bool
> > @@ -689,14 +668,14 @@ buffered_packets_ctx_has_packets(struct
> buffered_packets_ctx *ctx) {
> >
> > void
> > buffered_packets_ctx_init(struct buffered_packets_ctx *ctx) {
> > + ctx->ready_packets_data = VECTOR_EMPTY_INITIALIZER(struct
> bp_packet_data);
> > hmap_init(&ctx->buffered_packets);
> > - ovs_list_init(&ctx->ready_packets_data);
> > }
> >
> > void
> > buffered_packets_ctx_destroy(struct buffered_packets_ctx *ctx) {
> > struct bp_packet_data *pd;
> > - LIST_FOR_EACH_POP (pd, node, &ctx->ready_packets_data) {
> > + VECTOR_FOR_EACH_PTR (&ctx->ready_packets_data, pd) {
> > bp_packet_data_destroy(pd);
> > }
>
> Should we vector_destoy(&ctx->ready_packets_data) here?
>
Good catch, we definitely should.
> >
> > @@ -707,16 +686,6 @@ buffered_packets_ctx_destroy(struct
> buffered_packets_ctx *ctx) {
> > hmap_destroy(&ctx->buffered_packets);
> > }
> >
> > -
> > -void
> > -mac_cache_stats_destroy(struct ovs_list *stats_list)
> > -{
> > - struct mac_cache_stats *stats;
> > - LIST_FOR_EACH_POP (stats, list_node, stats_list) {
> > - free(stats);
> > - }
> > -}
> > -
> > static uint32_t
> > mac_binding_data_hash(const struct mac_binding_data *mb_data)
> > {
> > @@ -815,11 +784,12 @@ static void
> > buffered_packets_remove(struct buffered_packets_ctx *ctx,
> > struct buffered_packets *bp) {
> > struct bp_packet_data *pd;
> > - LIST_FOR_EACH_POP (pd, node, &bp->queue) {
> > + VECTOR_FOR_EACH_PTR (&bp->queue, pd) {
> > bp_packet_data_destroy(pd);
> > }
> >
> > hmap_remove(&ctx->buffered_packets, &bp->hmap_node);
> > + vector_destroy(&bp->queue);
> > free(bp);
> > }
> >
> > @@ -861,48 +831,49 @@ buffered_packets_db_lookup(struct buffered_packets
> *bp, struct ds *ip,
> >
> > void
> > mac_binding_probe_stats_process_flow_stats(
> > - struct ovs_list *stats_list,
> > + struct vector *stats_vec,
> > struct ofputil_flow_stats *ofp_stats)
> > {
> > - struct mac_cache_stats *stats = xmalloc(sizeof *stats);
> > -
> > - stats->idle_age_ms = ofp_stats->idle_age * 1000;
> > - stats->data.mb = (struct mac_binding_data) {
> > - .cookie = ntohll(ofp_stats->cookie),
> > - /* The port_key must be zero to match
> mac_binding_data_from_sbrec. */
> > - .port_key = 0,
> > - .dp_key = ntohll(ofp_stats->match.flow.metadata),
> > + struct mac_cache_stats stats = (struct mac_cache_stats) {
> > + .idle_age_ms = ofp_stats->idle_age * 1000,
> > + .data.mb = (struct mac_binding_data) {
> > + .cookie = ntohll(ofp_stats->cookie),
> > + /* The port_key must be zero to match
> > + * mac_binding_data_from_sbrec. */
> > + .port_key = 0,
> > + .dp_key = ntohll(ofp_stats->match.flow.metadata),
> > + .mac = ofp_stats->match.flow.dl_src
> > + },
> > };
> >
> > if (ofp_stats->match.flow.regs[0]) {
> > - stats->data.mb.ip =
> > + stats.data.mb.ip =
> > in6_addr_mapped_ipv4(htonl(ofp_stats->match.flow.regs[0]));
> > } else {
> > ovs_be128 ip6 = hton128(flow_get_xxreg(&ofp_stats->match.flow,
> 1));
> > - memcpy(&stats->data.mb.ip, &ip6, sizeof stats->data.mb.ip);
> > + memcpy(&stats.data.mb.ip, &ip6, sizeof stats.data.mb.ip);
> > }
> >
> > - ovs_list_push_back(stats_list, &stats->list_node);
> > + vector_push(stats_vec, &stats);
> > }
> >
> > void
> > mac_binding_probe_stats_run(
> > struct rconn *swconn,
> > struct ovsdb_idl_index *sbrec_port_binding_by_name,
> > - struct ovs_list *stats_list,
> > + struct vector *stats_vec,
> > uint64_t *req_delay, void *data)
> > {
> > long long timewall_now = time_wall_msec();
> > struct mac_cache_data *cache_data = data;
> >
> > struct mac_cache_stats *stats;
> > - LIST_FOR_EACH_POP (stats, list_node, stats_list) {
> > + VECTOR_FOR_EACH_PTR (stats_vec, stats) {
> > struct mac_binding *mb =
> mac_binding_find(&cache_data->mac_bindings,
> > &stats->data.mb);
> > if (!mb) {
> > mac_binding_update_log("Probe: not found in the cache:",
> > &stats->data.mb, false, NULL, 0, 0);
> > - free(stats);
> > continue;
> > }
> >
> > @@ -915,7 +886,6 @@ mac_binding_probe_stats_run(
> > mac_binding_update_log("Not sending ARP/ND request for
> non-active",
> > &mb->data, true, threshold,
> > stats->idle_age_ms,
> since_updated_ms);
> > - free(stats);
> > continue;
> > }
> >
> > @@ -924,7 +894,6 @@ mac_binding_probe_stats_run(
> > "Not sending ARP/ND request for recently updated",
> > &mb->data, true, threshold, stats->idle_age_ms,
> > since_updated_ms);
> > - free(stats);
> > continue;
> > }
> >
> > @@ -932,13 +901,11 @@ mac_binding_probe_stats_run(
> > lport_lookup_by_name(sbrec_port_binding_by_name,
> > sbrec->logical_port);
> > if (!pb) {
> > - free(stats);
> > continue;
> > }
> >
> > struct lport_addresses laddr;
> > if (!extract_lsp_addresses(pb->mac[0], &laddr)) {
> > - free(stats);
> > continue;
> > }
> >
> > @@ -960,7 +927,6 @@ mac_binding_probe_stats_run(
> > OFTABLE_LOCAL_OUTPUT);
> > }
> >
> > - free(stats);
> > destroy_lport_addresses(&laddr);
> > }
> >
> > diff --git a/controller/mac-cache.h b/controller/mac-cache.h
> > index f5dbaf813..295dc9e23 100644
> > --- a/controller/mac-cache.h
> > +++ b/controller/mac-cache.h
> > @@ -28,6 +28,9 @@
> > #include "ovn-sb-idl.h"
> >
> > struct ovsdb_idl_index;
> > +struct vector;
> > +
> > +#define MAC_CACHE_VEC_CAPACITY_THRESHOLD 1024
> >
> > struct mac_cache_data {
> > /* 'struct mac_cache_threshold' by datapath's tunnel_key. */
> > @@ -89,9 +92,18 @@ struct fdb {
> > long long timestamp;
> > };
> >
> > -struct bp_packet_data {
> > - struct ovs_list node;
> > +struct mac_cache_stats {
> > + int64_t idle_age_ms;
> > +
> > + union {
> > + /* Common data to identify MAC binding. */
> > + struct mac_binding_data mb;
> > + /* Common data to identify FDB. */
> > + struct fdb_data fdb;
> > + } data;
> > +};
> >
> > +struct bp_packet_data {
> > struct ofpbuf *continuation;
> > struct ofputil_packet_in pin;
> > };
> > @@ -102,7 +114,7 @@ struct buffered_packets {
> > struct mac_binding_data mb_data;
> >
> > /* Queue of packet_data associated with this struct. */
> > - struct ovs_list queue;
> > + struct vector queue;
> >
> > /* Timestamp in ms when the buffered packet should expire. */
> > long long int expire_at_ms;
> > @@ -115,7 +127,7 @@ struct buffered_packets_ctx {
> > /* Map of all buffered packets waiting for the MAC address. */
> > struct hmap buffered_packets;
> > /* List of packet data that are ready to be sent. */
> > - struct ovs_list ready_packets_data;
> > + struct vector ready_packets_data;
> > };
> >
> > /* Thresholds. */
> > @@ -177,30 +189,24 @@ void fdbs_clear(struct hmap *map);
> >
> > /* MAC binding stat processing. */
> > void
> > -mac_binding_stats_process_flow_stats(struct ovs_list *stats_list,
> > +mac_binding_stats_process_flow_stats(struct vector *stats_vec,
> > struct ofputil_flow_stats
> *ofp_stats);
> >
> > void mac_binding_stats_run(
> > struct rconn *swconn OVS_UNUSED,
> > struct ovsdb_idl_index *sbrec_port_binding_by_name OVS_UNUSED,
> > - struct ovs_list *stats_list, uint64_t *req_delay, void *data);
> > + struct vector *stats_vec, uint64_t *req_delay, void *data);
> >
> > /* FDB stat processing. */
> > -void fdb_stats_process_flow_stats(struct ovs_list *stats_list,
> > +void fdb_stats_process_flow_stats(struct vector *stats_vec,
> > struct ofputil_flow_stats
> *ofp_stats);
> >
> > void fdb_stats_run(
> > struct rconn *swconn OVS_UNUSED,
> > struct ovsdb_idl_index *sbrec_port_binding_by_name OVS_UNUSED,
> > - struct ovs_list *stats_list, uint64_t *req_delay, void *data);
> > -
> > -void mac_cache_stats_destroy(struct ovs_list *stats_list);
> > + struct vector *stats_vec, uint64_t *req_delay, void *data);
> >
> > /* Packet buffering. */
> > -struct bp_packet_data *
> > -bp_packet_data_create(const struct ofputil_packet_in *pin,
> > - const struct ofpbuf *continuation);
> > -
> > void bp_packet_data_destroy(struct bp_packet_data *pd);
> >
> > struct buffered_packets *
> > @@ -208,7 +214,8 @@ buffered_packets_add(struct buffered_packets_ctx
> *ctx,
> > struct mac_binding_data mb_data);
> >
> > void buffered_packets_packet_data_enqueue(struct buffered_packets *bp,
> > - struct bp_packet_data *pd);
> > + const struct
> ofputil_packet_in *pin,
> > + const struct ofpbuf
> *continuation);
> >
> > void buffered_packets_ctx_run(struct buffered_packets_ctx *ctx,
> > const struct hmap *recent_mbs,
> > @@ -226,12 +233,12 @@ bool buffered_packets_ctx_is_ready_to_send(struct
> buffered_packets_ctx *ctx);
> > bool buffered_packets_ctx_has_packets(struct buffered_packets_ctx
> *ctx);
> >
> > void mac_binding_probe_stats_process_flow_stats(
> > - struct ovs_list *stats_list,
> > + struct vector *stats_vec,
> > struct ofputil_flow_stats *ofp_stats);
> >
> > void mac_binding_probe_stats_run(
> > struct rconn *swconn,
> > struct ovsdb_idl_index *sbrec_port_binding_by_name,
> > - struct ovs_list *stats_list, uint64_t *req_delay, void *data);
> > + struct vector *stats_vec, uint64_t *req_delay, void *data);
> >
> > #endif /* controller/mac-cache.h */
> > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > index f67a61758..a51957761 100644
> > --- a/controller/pinctrl.c
> > +++ b/controller/pinctrl.c
> > @@ -1569,8 +1569,7 @@ OVS_REQUIRES(pinctrl_mutex)
> > return;
> > }
> >
> > - struct bp_packet_data *pd = bp_packet_data_create(pin,
> continuation);
> > - buffered_packets_packet_data_enqueue(bp, pd);
> > + buffered_packets_packet_data_enqueue(bp, pin, continuation);
> >
> > /* There is a chance that the MAC binding was already created. */
> > notify_pinctrl_main();
> > @@ -4734,15 +4733,21 @@ send_mac_binding_buffered_pkts(struct rconn
> *swconn)
> > {
> > enum ofp_version version = rconn_get_version(swconn);
> > enum ofputil_protocol proto =
> ofputil_protocol_from_ofp_version(version);
> > + struct vector *rpd = &buffered_packets_ctx.ready_packets_data;
> >
> > struct bp_packet_data *pd;
> > - LIST_FOR_EACH_POP (pd, node,
> &buffered_packets_ctx.ready_packets_data) {
> > + VECTOR_FOR_EACH_PTR (rpd, pd) {
> > queue_msg(swconn, ofputil_encode_resume(&pd->pin,
> pd->continuation,
> > proto));
> > bp_packet_data_destroy(pd);
> > }
> >
> > - ovs_list_init(&buffered_packets_ctx.ready_packets_data);
> > + vector_clear(rpd);
> > + if (vector_capacity(rpd) >= MAC_CACHE_VEC_CAPACITY_THRESHOLD) {
> > + VLOG_DBG("The ready_packets_data vector capacity (%"PRIuSIZE") "
> > + "is over threshold.", vector_capacity(rpd));
> > + vector_shrink_to_fit(rpd);
> > + }
> > }
> >
> > /* Update or add an IP-MAC binding for 'logical_port'.
> > diff --git a/controller/statctrl.c b/controller/statctrl.c
> > index f0c5bd904..28c794df8 100644
> > --- a/controller/statctrl.c
> > +++ b/controller/statctrl.c
> > @@ -19,6 +19,7 @@
> > #include "dirs.h"
> > #include "latch.h"
> > #include "lflow.h"
> > +#include "lib/vec.h"
> > #include "mac-cache.h"
> > #include "openvswitch/ofp-errors.h"
> > #include "openvswitch/ofp-flow.h"
> > @@ -53,39 +54,33 @@ struct stats_node {
> > int64_t next_request_timestamp;
> > /* Request delay in ms. */
> > uint64_t request_delay;
> > - /* List of processed statistics. */
> > - struct ovs_list stats_list;
> > - /* Function to clean up the node.
> > - * This function runs in main thread. */
> > - void (*destroy)(struct ovs_list *stats_list);
> > + /* Vector of processed statistics. */
> > + struct vector stats;
> > /* Function to process the response and store it in the list.
> > * This function runs in statctrl thread locked behind mutex. */
> > - void (*process_flow_stats)(struct ovs_list *stats_list,
> > + void (*process_flow_stats)(struct vector *stats,
> > struct ofputil_flow_stats *ofp_stats);
> > /* Function to process the parsed stats.
> > * This function runs in main thread locked behind mutex. */
> > void (*run)(struct rconn *swconn,
> > struct ovsdb_idl_index *sbrec_port_binding_by_name,
> > - struct ovs_list *stats_list,
> > + struct vector *stats,
> > uint64_t *req_delay, void *data);
> > - /* Name of the stats node corresponding stopwatch. */
> > - const char *stopwatch_name;
> > + /* Name of the stats node. */
> > + const char *name;
> > };
> >
> > -#define STATS_NODE(NAME, REQUEST, DESTROY, PROCESS, RUN)
> \
> > +#define STATS_NODE(NAME, REQUEST, STAT_TYPE, PROCESS, RUN)
> \
> > do {
> \
> > statctrl_ctx.nodes[STATS_##NAME] = (struct stats_node) {
> \
> > .request = REQUEST,
> \
> > .xid = 0,
> \
> > .next_request_timestamp = INT64_MAX,
> \
> > .request_delay = 0,
> \
> > - .stats_list =
> \
> > - OVS_LIST_INITIALIZER(
> \
> > - &statctrl_ctx.nodes[STATS_##NAME].stats_list),
> \
> > - .destroy = DESTROY,
> \
> > + .stats = VECTOR_EMPTY_INITIALIZER(STAT_TYPE),
> \
> > .process_flow_stats = PROCESS,
> \
> > .run = RUN,
> \
> > - .stopwatch_name = OVS_STRINGIZE(stats_##NAME),
> \
> > + .name = OVS_STRINGIZE(stats_##NAME), \
> > };
> \
> > stopwatch_create(OVS_STRINGIZE(stats_##NAME), SW_MS);
> \
> > } while (0)
> > @@ -143,7 +138,7 @@ statctrl_init(void)
> > .out_group = OFPG_ANY,
> > .table_id = OFTABLE_MAC_CACHE_USE,
> > };
> > - STATS_NODE(MAC_BINDING, mac_binding_request,
> mac_cache_stats_destroy,
> > + STATS_NODE(MAC_BINDING, mac_binding_request, struct mac_cache_stats,
> > mac_binding_stats_process_flow_stats,
> mac_binding_stats_run);
> >
> > struct ofputil_flow_stats_request fdb_request = {
> > @@ -153,7 +148,7 @@ statctrl_init(void)
> > .out_group = OFPG_ANY,
> > .table_id = OFTABLE_LOOKUP_FDB,
> > };
> > - STATS_NODE(FDB, fdb_request, mac_cache_stats_destroy,
> > + STATS_NODE(FDB, fdb_request, struct mac_cache_stats,
> > fdb_stats_process_flow_stats, fdb_stats_run);
> >
> > struct ofputil_flow_stats_request mac_binding_probe_request = {
> > @@ -164,7 +159,7 @@ statctrl_init(void)
> > .table_id = OFTABLE_MAC_BINDING,
> > };
> > STATS_NODE(MAC_BINDING_PROBE, mac_binding_probe_request,
> > - mac_cache_stats_destroy,
> > + struct mac_cache_stats,
> > mac_binding_probe_stats_process_flow_stats,
> > mac_binding_probe_stats_run);
> >
> > @@ -196,11 +191,19 @@ statctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > struct stats_node *node = &statctrl_ctx.nodes[i];
> > uint64_t prev_delay = node->request_delay;
> >
> > - stopwatch_start(node->stopwatch_name, time_msec());
> > + stopwatch_start(node->name, time_msec());
> > node->run(statctrl_ctx.swconn,
> > - sbrec_port_binding_by_name, &node->stats_list,
> > + sbrec_port_binding_by_name, &node->stats,
> > &node->request_delay, node_data[i]);
> > - stopwatch_stop(node->stopwatch_name, time_msec());
> > + vector_clear(&node->stats);
> > + if (vector_capacity(&node->stats) >=
> > + MAC_CACHE_VEC_CAPACITY_THRESHOLD) {
>
> This strikes me as odd. Specifically, if all stats nodes have the same
> threshold, then the name should probably be changed away from
> MAC_CACHE_VEC_CAPACITY_THRESHOLD.
>
>
The whole module is called mac-cache.c so it seemed fitting to use that
name.
It is also used in pinctrl, I have changed the name of both to better
reflect
what's going on.
>
> > + VLOG_DBG("The statistics vector for node '%s' capacity "
> > + "(%"PRIuSIZE") is over threshold.", node->name,
> > + vector_capacity(&node->stats));
> > + vector_shrink_to_fit(&node->stats);
> > + }
> > + stopwatch_stop(node->name, time_msec());
> >
> > schedule_updated |=
> > statctrl_update_next_request_timestamp(node, now,
> prev_delay);
> > @@ -233,7 +236,7 @@ statctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn)
> > ovs_mutex_lock(&mutex);
> > for (size_t i = 0; i < STATS_MAX; i++) {
> > struct stats_node *node = &statctrl_ctx.nodes[i];
> > - if (!ovs_list_is_empty(&node->stats_list)) {
> > + if (!vector_is_empty(&node->stats)) {
> > poll_immediate_wake();
> > }
> > }
> > @@ -254,7 +257,7 @@ statctrl_destroy(void)
> >
> > for (size_t i = 0; i < STATS_MAX; i++) {
> > struct stats_node *node = &statctrl_ctx.nodes[i];
> > - node->destroy(&node->stats_list);
> > + vector_destroy(&node->stats);
> > }
> > }
> >
> > @@ -364,7 +367,7 @@ statctrl_decode_statistics_reply(struct stats_node
> *node, struct ofpbuf *msg)
> > break;
> > }
> >
> > - node->process_flow_stats(&node->stats_list, &fs);
> > + node->process_flow_stats(&node->stats, &fs);
> > }
> >
> > ofpbuf_uninit(&ofpacts);
> > @@ -399,7 +402,7 @@ static void
> > statctrl_notify_main_thread(struct statctrl_ctx *ctx)
> > {
> > for (size_t i = 0; i < STATS_MAX; i++) {
> > - if (!ovs_list_is_empty(&ctx->nodes[i].stats_list)) {
> > + if (!vector_is_empty(&ctx->nodes[i].stats)) {
> > seq_change(ctx->main_seq);
> > return;
> > }
>
>
Thanks,
Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
