On Wed, Sep 30, 2026 at 4:27 PM Dumitru Ceara <[email protected]> wrote:
> Hi Ales, > > Thanks for the fix! > > On 9/25/26 2:04 PM, Ales Musil wrote: > > On Fri, Sep 25, 2026 at 1:29 PM Ales Musil <[email protected]> wrote: > > > >> Large MAC binding and FDB timestamp transactions can block both the > >> controller main thread and the Southbound database server. Spread these > >> updates across the statistics request interval to reduce individual > >> transaction costs while preserving the existing eligibility checks. > >> > >> Pending UUID sets also deduplicate repeated statistics replies and allow > >> rows deleted before a drain to be skipped safely. > >> > >> Reported-at: https://issues.redhat.com/browse/FDP-4385 > > > > > > Small mistake, this should have been > > https://redhat.atlassian.net/browse/FDP-4388 instead. > > > > I can fix this up, no worries. > > > > >> > >> Assisted-by: GPT-5.6 Sol, OpenCode > >> Signed-off-by: Ales Musil <[email protected]> > >> --- > >> controller/mac-cache.c | 93 ++++++++++++++++++++++++++++++++++++------ > >> controller/mac-cache.h | 15 +++++-- > >> controller/statctrl.c | 70 +++++++++++++++++++++++++++---- > >> 3 files changed, 155 insertions(+), 23 deletions(-) > >> > >> diff --git a/controller/mac-cache.c b/controller/mac-cache.c > >> index fad44401a..5869604a9 100644 > >> --- a/controller/mac-cache.c > >> +++ b/controller/mac-cache.c > >> @@ -25,6 +25,7 @@ > >> #include "openvswitch/hmap.h" > >> #include "openvswitch/vlog.h" > >> #include "ovn/logical-fields.h" > >> +#include "uuidset.h" > >> #include "ovn-sb-idl.h" > >> #include "pinctrl.h" > >> > >> @@ -403,7 +404,8 @@ mac_binding_update_log(const char *action, > >> > >> void > >> mac_binding_stats_run(struct vector *stats_vec, uint64_t *req_delay, > >> - void *data, long long timewall_now) > >> + void *data, struct uuidset *pending, > >> + long long timewall_now) > >> { > >> struct mac_cache_data *cache_data = data; > >> > >> @@ -432,10 +434,10 @@ mac_binding_stats_run(struct vector *stats_vec, > >> uint64_t *req_delay, > >> * used on this chassis. */ > >> if (stats->idle_age_ms < threshold->value) { > >> if (since_updated_ms >= threshold->cooldown_period) { > >> - mac_binding_update_log("Updating active", &mb->data, > true, > >> - threshold, stats->idle_age_ms, > >> - since_updated_ms); > >> - sbrec_mac_binding_set_timestamp(mb->sbrec, > timewall_now); > >> + mac_binding_update_log("Queuing timestamp update for > >> active", > >> + &mb->data, true, threshold, > >> + stats->idle_age_ms, > >> since_updated_ms); > >> + uuidset_insert(pending, &mb->sbrec->header_.uuid); > >> } else { > >> /* Postponing the update to avoid sending database > >> transactions > >> * too frequently. */ > >> @@ -456,6 +458,38 @@ mac_binding_stats_run(struct vector *stats_vec, > >> uint64_t *req_delay, > >> } > >> } > >> > >> +void > >> +mac_binding_stats_drain(struct ovsdb_idl_txn *txn, struct uuidset > >> *pending, > >> + size_t batch_size, long long timewall_now) > >> +{ > >> + struct ovsdb_idl *idl = ovsdb_idl_txn_get_idl(txn); > >> + size_t drained = 0; > >> + > >> + struct uuidset_node *node; > >> + UUIDSET_FOR_EACH_SAFE (node, pending) { > >> + if (drained == batch_size) { > >> + break; > >> + } > >> + > >> + const struct sbrec_mac_binding *mb = > >> + sbrec_mac_binding_get_for_uuid(idl, &node->uuid); > >> + if (mb) { > >> + sbrec_mac_binding_set_timestamp(mb, timewall_now); > >> + > >> + if (VLOG_IS_DBG_ENABLED()) { > >> + struct mac_binding_data data; > >> + if (mac_binding_data_from_sbrec(&data, mb)) { > >> + mac_binding_update_log("Updating active", &data, > >> false, > >> + NULL, 0, > >> + mb->timestamp - > timewall_now); > > Nit: Here "mb->timestamp - timewall_now" is always 0, we might as well > pass 0 directly. > > >> + } > >> + } > >> + } > >> + uuidset_delete(pending, node); > >> + drained++; > >> + } > >> +} > >> + > >> /* FDB stat processing. */ > >> void > >> fdb_stats_process_flow_stats(struct vector *stats_vec, > >> @@ -508,7 +542,7 @@ fdb_update_log(const char *action, > >> > >> void > >> fdb_stats_run(struct vector *stats_vec, uint64_t *req_delay, void > *data, > >> - long long timewall_now) > >> + struct uuidset *pending, long long timewall_now) > >> { > >> struct mac_cache_data *cache_data = data; > >> > >> @@ -536,10 +570,10 @@ fdb_stats_run(struct vector *stats_vec, uint64_t > >> *req_delay, void *data, > >> * used on this chassis. */ > >> if (stats->idle_age_ms < threshold->value) { > >> if (since_updated_ms >= threshold->cooldown_period) { > >> - fdb_update_log("Updating active", &fdb->data, true, > >> - threshold, stats->idle_age_ms, > >> - since_updated_ms); > >> - sbrec_fdb_set_timestamp(fdb->sbrec_fdb, timewall_now); > >> + fdb_update_log("Queuing timestamp update for active", > >> + &fdb->data, true, threshold, > >> + stats->idle_age_ms, since_updated_ms); > >> + uuidset_insert(pending, &fdb->sbrec_fdb->header_.uuid); > >> } else { > >> /* Postponing the update to avoid sending database > >> transactions > >> * too frequently. */ > >> @@ -559,6 +593,36 @@ fdb_stats_run(struct vector *stats_vec, uint64_t > >> *req_delay, void *data, > >> } > >> } > >> > >> +void > >> +fdb_stats_drain(struct ovsdb_idl_txn *txn, struct uuidset *pending, > >> + size_t batch_size, long long timewall_now) > >> +{ > >> + struct ovsdb_idl *idl = ovsdb_idl_txn_get_idl(txn); > >> + size_t drained = 0; > >> + > >> + struct uuidset_node *node; > >> + UUIDSET_FOR_EACH_SAFE (node, pending) { > >> + if (drained == batch_size) { > >> + break; > >> + } > >> + > >> + const struct sbrec_fdb *fdb = sbrec_fdb_get_for_uuid(idl, > >> &node->uuid); > >> + if (fdb) { > >> + sbrec_fdb_set_timestamp(fdb, timewall_now); > >> + > >> + if (VLOG_IS_DBG_ENABLED()) { > >> + struct fdb_data data; > >> + if (fdb_data_from_sbrec(&data, fdb)) { > >> + fdb_update_log("Updating active", &data, false, > >> + NULL, 0, fdb->timestamp - > >> timewall_now); > > Nit: Here "fdb->timestamp - timewall_now" is always 0, we might as well > pass 0 directly. > > >> + } > >> + } > >> + } > >> + uuidset_delete(pending, node); > >> + drained++; > >> + } > >> +} > >> + > >> /* Packet buffering. */ > >> void > >> bp_packet_data_destroy(struct bp_packet_data *pd) { > >> @@ -901,7 +965,8 @@ mac_binding_probe_stats_process_flow_stats( > >> > >> void > >> mac_binding_probe_stats_run(struct vector *stats_vec, uint64_t > *req_delay, > >> - void *data, long long timewall_now) > >> + void *data, struct uuidset *pending > >> OVS_UNUSED, > >> + long long timewall_now) > >> { > >> struct mac_binding_probe_data *probe_data = data; > >> struct mac_cache_data *cache_data = probe_data->cache_data; > >> @@ -936,9 +1001,11 @@ mac_binding_probe_stats_run(struct vector > >> *stats_vec, uint64_t *req_delay, > >> continue; > >> } > >> > >> - if (since_updated_ms < threshold->cooldown_period) { > >> + if (since_updated_ms < threshold->cooldown_period || > >> + uuidset_contains(probe_data->mac_binding_pending, > >> + &sbrec->header_.uuid)) { > >> mac_binding_update_log( > >> - "Not sending ARP/ND request for recently updated", > >> + "Not sending ARP/ND request for recently/to be > >> updated", > >> &mb->data, true, threshold, stats->idle_age_ms, > >> since_updated_ms); > >> mb->arp_attempts = 0; > >> diff --git a/controller/mac-cache.h b/controller/mac-cache.h > >> index bf9afaf3a..f27998e40 100644 > >> --- a/controller/mac-cache.h > >> +++ b/controller/mac-cache.h > >> @@ -29,6 +29,7 @@ > >> > >> struct ovsdb_idl_index; > >> struct vector; > >> +struct uuidset; > >> > >> struct mac_cache_data { > >> /* 'struct mac_cache_threshold' by datapath's tunnel_key. */ > >> @@ -65,6 +66,7 @@ struct mac_binding_data { > >> > >> struct mac_binding_probe_data { > >> struct mac_cache_data *cache_data; > >> + struct uuidset *mac_binding_pending; > >> struct rconn *swconn; > >> struct ovsdb_idl_index *sbrec_port_binding_by_name; > >> const struct sbrec_chassis *chassis; > >> @@ -201,14 +203,20 @@ mac_binding_stats_process_flow_stats(struct vector > >> *stats_vec, > >> struct ofputil_flow_stats > >> *ofp_stats); > >> > >> void mac_binding_stats_run(struct vector *stats_vec, uint64_t > *req_delay, > >> - void *data, long long timewall_now); > >> + void *data, struct uuidset *pending, > >> + long long timewall_now); > >> +void mac_binding_stats_drain(struct ovsdb_idl_txn *txn, > >> + struct uuidset *pending, size_t > batch_size, > >> + long long timewall_now); > >> > >> /* FDB stat processing. */ > >> void fdb_stats_process_flow_stats(struct vector *stats_vec, > >> struct ofputil_flow_stats > *ofp_stats); > >> > >> void fdb_stats_run(struct vector *stats_vec, uint64_t *req_delay, void > >> *data, > >> - long long timewall_now); > >> + struct uuidset *pending, long long timewall_now); > >> +void fdb_stats_drain(struct ovsdb_idl_txn *txn, struct uuidset > *pending, > >> + size_t batch_size, long long timewall_now); > >> > >> /* Packet buffering. */ > >> void bp_packet_data_destroy(struct bp_packet_data *pd); > >> @@ -237,6 +245,7 @@ void mac_binding_probe_stats_process_flow_stats( > >> struct ofputil_flow_stats *ofp_stats); > >> > >> void mac_binding_probe_stats_run(struct vector *stats_vec, uint64_t > >> *req_delay, > >> - void *data, long long timewall_now); > >> + void *data, struct uuidset *pending, > >> + long long timewall_now); > >> > >> #endif /* controller/mac-cache.h */ > >> diff --git a/controller/statctrl.c b/controller/statctrl.c > >> index 00c0a4450..77453319c 100644 > >> --- a/controller/statctrl.c > >> +++ b/controller/statctrl.c > >> @@ -35,10 +35,13 @@ > >> #include "socket-util.h" > >> #include "statctrl.h" > >> #include "stopwatch.h" > >> +#include "uuidset.h" > >> > >> VLOG_DEFINE_THIS_MODULE(statctrl); > >> > >> #define STATS_VEC_CAPACITY_THRESHOLD 1024 > >> +#define STATS_DRAIN_INTERVAL 3000 > >> +#define STATS_MIN_BATCH_SIZE 500 > >> > >> enum stat_type { > >> STATS_MAC_BINDING = 0, > >> @@ -58,6 +61,10 @@ struct stats_node { > >> uint64_t request_delay; > >> /* Vector of processed statistics. */ > >> struct vector stats; > >> + /* UUIDs whose timestamps should be updated. */ > >> + struct uuidset pending; > >> + /* Timestamp when the next batch should be drained. */ > >> + int64_t next_drain_timestamp; > >> /* 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 vector *stats, > >> @@ -65,12 +72,15 @@ struct stats_node { > >> /* Function to process the parsed stats. > >> * This function runs in main thread locked behind mutex. */ > >> void (*run)(struct vector *stats, uint64_t *req_delay, void *data, > >> - long long timewall_now); > >> + struct uuidset *pending, long long timewall_now); > >> + /* Function to drain a batch of pending timestamp updates. */ > >> + void (*drain)(struct ovsdb_idl_txn *txn, struct uuidset *pending, > >> + size_t batch_size, long long timewall_now); > >> /* Name of the stats node. */ > >> const char *name; > >> }; > >> > >> -#define STATS_NODE(NAME, REQUEST, STAT_TYPE, PROCESS, RUN) > >> \ > >> +#define STATS_NODE(NAME, REQUEST, STAT_TYPE, PROCESS, RUN, DRAIN) > >> \ > >> do { > >> \ > >> statctrl_ctx.nodes[STATS_##NAME] = (struct stats_node) { > >> \ > >> .request = REQUEST, > >> \ > >> @@ -78,10 +88,13 @@ struct stats_node { > >> .next_request_timestamp = INT64_MAX, > >> \ > >> .request_delay = 0, > >> \ > >> .stats = VECTOR_EMPTY_INITIALIZER(STAT_TYPE), > >> \ > >> + .next_drain_timestamp = INT64_MAX, > >> \ > >> .process_flow_stats = PROCESS, > >> \ > >> .run = RUN, > >> \ > >> - .name = OVS_STRINGIZE(stats_##NAME), \ > >> + .drain = DRAIN, > >> \ > >> + .name = OVS_STRINGIZE(stats_##NAME), > >> \ > >> }; > >> \ > >> + uuidset_init(&statctrl_ctx.nodes[STATS_##NAME].pending); > >> \ > >> stopwatch_create(OVS_STRINGIZE(stats_##NAME), SW_MS); > >> \ > >> } while (0) > >> > >> @@ -141,7 +154,8 @@ statctrl_init(void) > >> .table_id = OFTABLE_MAC_CACHE_USE, > >> }; > >> STATS_NODE(MAC_BINDING, mac_binding_request, struct > mac_cache_stats, > >> - mac_binding_stats_process_flow_stats, > >> mac_binding_stats_run); > >> + mac_binding_stats_process_flow_stats, > >> mac_binding_stats_run, > >> + mac_binding_stats_drain); > >> > >> struct ofputil_flow_stats_request fdb_request = { > >> .cookie = htonll(0), > >> @@ -151,7 +165,8 @@ statctrl_init(void) > >> .table_id = OFTABLE_LOOKUP_FDB, > >> }; > >> STATS_NODE(FDB, fdb_request, struct mac_cache_stats, > >> - fdb_stats_process_flow_stats, fdb_stats_run); > >> + fdb_stats_process_flow_stats, fdb_stats_run, > >> + fdb_stats_drain); > >> > >> struct ofputil_flow_stats_request mac_binding_probe_request = { > >> .cookie = htonll(0), > >> @@ -163,7 +178,7 @@ statctrl_init(void) > >> STATS_NODE(MAC_BINDING_PROBE, mac_binding_probe_request, > >> struct mac_cache_stats, > >> mac_binding_probe_stats_process_flow_stats, > >> - mac_binding_probe_stats_run); > >> + mac_binding_probe_stats_run, NULL); > >> > >> statctrl_ctx.thread = ovs_thread_create("ovn_statctrl", > >> statctrl_thread_handler, > >> @@ -182,6 +197,7 @@ statctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > >> > >> struct mac_binding_probe_data mac_binding_probe_data = { > >> .cache_data = mac_cache_data, > >> + .mac_binding_pending = > >> &statctrl_ctx.nodes[STATS_MAC_BINDING].pending, > >> .sbrec_port_binding_by_name = sbrec_port_binding_by_name, > >> .swconn = statctrl_ctx.swconn, > >> .chassis = chassis, > >> @@ -202,10 +218,12 @@ statctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > >> for (size_t i = 0; i < STATS_MAX; i++) { > >> struct stats_node *node = &statctrl_ctx.nodes[i]; > >> uint64_t prev_delay = node->request_delay; > >> + size_t prev_pending_count = uuidset_count(&node->pending); > >> > >> stopwatch_start(node->name, time_msec()); > >> node->run(&node->stats, &node->request_delay, node_data[i], > >> - timewall_now); > >> + &node->pending, timewall_now); > >> + now = time_msec(); > >> vector_clear(&node->stats); > >> if (vector_capacity(&node->stats) >= > >> STATS_VEC_CAPACITY_THRESHOLD) { > >> VLOG_DBG("The statistics vector for node '%s' capacity " > >> @@ -217,6 +235,40 @@ statctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, > >> > >> schedule_updated |= > >> statctrl_update_next_request_timestamp(node, now, > >> prev_delay); > >> + > >> + if (!node->drain) { > >> + continue; > >> + } > >> + > >> + size_t pending_count = uuidset_count(&node->pending); > >> + if (pending_count != prev_pending_count && pending_count && > >> + node->next_drain_timestamp == INT64_MAX) { > >> + node->next_drain_timestamp = now; > >> + } > >> + > >> + if (now < node->next_drain_timestamp) { > >> + continue; > >> + } > >> + > >> + /* Distribute pending updates over the remaining fixed drain > >> intervals > >> + * before the next statistics request, subject to the minimum > >> batch > >> + * size. */ > >> + uint64_t slots = 1; > >> + if (node->next_request_timestamp != INT64_MAX && > >> + node->next_request_timestamp > now) { > >> + slots = MAX((node->next_request_timestamp - now) > >> + / STATS_DRAIN_INTERVAL, 1); > >> + } > >> + size_t batch_size = MAX(STATS_MIN_BATCH_SIZE, > >> + (pending_count / slots) + 1); > >> + > >> + stopwatch_start(node->name, time_msec()); > >> + node->drain(ovnsb_idl_txn, &node->pending, batch_size, > >> + timewall_now); > >> + node->next_drain_timestamp = uuidset_is_empty(&node->pending) > >> + ? INT64_MAX > >> + : now + STATS_DRAIN_INTERVAL; > >> + stopwatch_stop(node->name, time_msec()); > >> } > >> ovs_mutex_unlock(&mutex); > >> > >> @@ -249,6 +301,9 @@ statctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn) > >> if (!vector_is_empty(&node->stats)) { > >> poll_immediate_wake(); > >> } > >> + if (node->next_drain_timestamp != INT64_MAX) { > >> + poll_timer_wait_until(node->next_drain_timestamp); > >> + } > >> } > >> seq_wait(statctrl_ctx.main_seq, statctrl_ctx.new_main_seq); > >> ovs_mutex_unlock(&mutex); > >> @@ -267,6 +322,7 @@ statctrl_destroy(void) > >> for (size_t i = 0; i < STATS_MAX; i++) { > >> struct stats_node *node = &statctrl_ctx.nodes[i]; > >> vector_destroy(&node->stats); > >> + uuidset_destroy(&node->pending); > >> } > >> } > >> > >> -- > >> 2.55.0 > >> > >> > > > > The patch looks good to me but I asked some of our OpenShift colleagues > to run more tests with it at scale to make sure it works good in their > scenario too. We should be getting that information these days. > > I can merge the patch then or if you want to merge it before: > > Acked-by: Dumitru Ceara <[email protected]> > > Thanks, > Dumitru > > Thank you Dumitru, I saw that the testing went well. I took care of the commit message adjustment, and the two nits. With that applied to main and backported down to 26.03. Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
