Thanks for the review! I can do just about everything, see my comments about testing below
On Wed, Sep 9, 2026 at 1:47 AM Ales Musil <[email protected]> wrote: > > > On Tue, Sep 1, 2026 at 11:04 PM Jacob Tanenbaum via dev < > [email protected]> wrote: > >> Split ovn-northd's en_lflow engine node into a compute-only >> en_lflow node and a new en_dp_group_resolved node that >> handles datapath-group resolution and all SB database >> writes. This separation makes en_lflow independent of the >> SB database, preparing for future incremental processing >> improvements. >> >> en_lflow's handlers now track dirty lflow_refs in a hmapx >> instead of calling lflow_ref_sync_lflows directly. >> en_dp_group_resolved drains the dirty set on its lflow >> handler, falling back to a full lflow_table_sync_to_sb when >> en_lflow did a full recompute or when the IGMP/MLD handler >> set the needs_full_sync flag. >> >> A new lflow_ref_unlink_and_prune() function replaces >> lflow_ref_resync_flows() for the IGMP and IC-learned >> service monitor handlers. These handlers use a single >> shared lflow_ref whose flows are also built by the >> per-datapath pipeline (with lflow_ref = NULL). The >> per-datapath build's dp bits are not tracked by any >> lflow_ref, so lflow_ref_unlink_lflows alone cannot clear >> them via dp_refcnt. lflow_ref_unlink_and_prune destroys >> all lrns and orphaned lflows in-memory without SB writes, >> equivalent to lflow_ref_resync_flows' cleanup. >> >> The lflow_table_add_lflow__ upgrade-to-bitmap logic (for >> single lflow_refs that contribute multiple datapaths to the >> same lflow) is extended with per-datapath dp_refcnt >> accounting during mid-cycle bitmap upgrades, preventing >> premature dp bit release when multiple lflow_refs share a >> flow across overlapping datapaths. >> >> Reported-at: https://redhat.atlassian.net/browse/FDP-2747 >> Assisted-by: Claude Opus 4.8, Claude Code >> Signed-off-by: Jacob Tanenbaum <[email protected]> >> --- >> > > Hi Jacob, > > thank you for the patch. I have a few comments down below. > Also did you run ovn-heater tests to see if there are any potential > issues at scale? > > >> northd/automake.mk | 2 + >> northd/en-dp-group-resolved.c | 129 ++++++++++++++++ >> northd/en-dp-group-resolved.h | 30 ++++ >> northd/en-lflow.c | 140 +++++------------ >> northd/en-lflow.h | 3 + >> northd/en-northd-output.c | 4 +- >> northd/en-northd-output.h | 3 +- >> northd/inc-proc-northd.c | 29 +++- >> northd/lflow-mgr.c | 168 +++++++++++++++++---- >> northd/lflow-mgr.h | 8 +- >> northd/northd.c | 251 +++++++------------------------ >> northd/northd.h | 39 +++-- >> tests/ovn-inc-proc-graph-dump.at | 12 +- >> tests/ovn-northd.at | 81 +++++++--- >> 14 files changed, 506 insertions(+), 393 deletions(-) >> create mode 100644 northd/en-dp-group-resolved.c >> create mode 100644 northd/en-dp-group-resolved.h >> >> diff --git a/northd/automake.mk b/northd/automake.mk >> index 45ca0337f..d1439294d 100644 >> --- a/northd/automake.mk >> +++ b/northd/automake.mk >> @@ -16,6 +16,8 @@ northd_ovn_northd_SOURCES = \ >> northd/en-datapath-logical-router.h \ >> northd/en-datapath-sync.c \ >> northd/en-datapath-sync.h \ >> + northd/en-dp-group-resolved.c \ >> + northd/en-dp-group-resolved.h \ >> northd/en-ecmp-nexthop.c \ >> northd/en-ecmp-nexthop.h \ >> northd/en-global-config.c \ >> diff --git a/northd/en-dp-group-resolved.c b/northd/en-dp-group-resolved.c >> new file mode 100644 >> index 000000000..d5f9e7738 >> --- /dev/null >> +++ b/northd/en-dp-group-resolved.c >> > > nit: I would probably leave the resolved out of the name. > Just en-dp-group is better IMO. > will change for v2 > > >> @@ -0,0 +1,129 @@ >> +/* >> + * Copyright (c) 2026, Red Hat, Inc. >> + * >> + * Licensed under the Apache License, Version 2.0 (the "License"); >> + * you may not use this file except in compliance with the License. >> + * You may obtain a copy of the License at: >> + * >> + * http://www.apache.org/licenses/LICENSE-2.0 >> + * >> + * Unless required by applicable law or agreed to in writing, software >> + * distributed under the License is distributed on an "AS IS" BASIS, >> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >> implied. >> + * See the License for the specific language governing permissions and >> + * limitations under the License. >> + */ >> + >> +#include <config.h> >> + >> +#include "en-dp-group-resolved.h" >> +#include "en-datapath-sync.h" >> +#include "en-global-config.h" >> +#include "en-lflow.h" >> +#include "lflow-mgr.h" >> + >> +#include "lib/inc-proc-eng.h" >> +#include "northd.h" >> +#include "lib/stopwatch-names.h" >> +#include "stopwatch.h" >> +#include "timeval.h" >> +#include "openvswitch/vlog.h" >> > > nit: Not ordered according to coding style. > will change for v2 > > >> + >> +VLOG_DEFINE_THIS_MODULE(en_dp_group_resolved); >> + >> +void * >> +en_dp_group_resolved_init(struct engine_node *node OVS_UNUSED, >> + struct engine_arg *arg OVS_UNUSED) >> +{ >> + return NULL; >> +} >> + >> +static void >> +dp_group_resolved_sync_to_sb(struct engine_node *node, >> + struct lflow_data *lflow_data) >> +{ >> + const struct engine_context *eng_ctx = engine_get_context(); >> + struct all_synced_datapaths *all_dps = >> + engine_get_input_data("datapath_sync", node); >> + >> + const struct sbrec_logical_flow_table *sb_flow_table = >> + EN_OVSDB_GET(engine_get_input("SB_logical_flow", node)); >> + const struct sbrec_logical_dp_group_table *sb_dpgrp_table = >> + EN_OVSDB_GET(engine_get_input("SB_logical_dp_group", node)); >> + >> + struct ed_type_global_config *global_config = >> + engine_get_input_data("global_config", node); >> + >> + stopwatch_start(LFLOWS_TO_SB_STOPWATCH_NAME, time_msec()); >> + lflow_table_sync_to_sb(lflow_data->lflow_table, >> + eng_ctx->ovnsb_idl_txn, >> + all_dps->synced_dps, >> + global_config->ovn_internal_version_changed, >> + sb_flow_table, sb_dpgrp_table); >> + stopwatch_stop(LFLOWS_TO_SB_STOPWATCH_NAME, time_msec()); >> +} >> + >> +enum engine_node_state >> +en_dp_group_resolved_run(struct engine_node *node, >> + void *data OVS_UNUSED) >> +{ >> + struct lflow_data *lflow_data = engine_get_input_data("lflow", node); >> + >> + /* The full sync below covers every lflow in the table, so any >> + * per-ref dirty tracking left over from en_lflow's handlers is >> + * redundant. Clear it so the next incremental cycle starts >> + * clean. */ >> + hmapx_clear(&lflow_data->dirty_lflow_refs); >> + lflow_data->needs_full_sync = false; >> + >> + dp_group_resolved_sync_to_sb(node, lflow_data); >> + return EN_UPDATED; >> +} >> + >> +enum engine_input_handler_result >> +dp_group_resolved_lflow_handler(struct engine_node *node, >> + void *data OVS_UNUSED) >> +{ >> + struct lflow_data *lflow_data = engine_get_input_data("lflow", node); >> + >> + if (hmapx_is_empty(&lflow_data->dirty_lflow_refs) >> + || lflow_data->needs_full_sync) { >> + hmapx_clear(&lflow_data->dirty_lflow_refs); >> + lflow_data->needs_full_sync = false; >> + dp_group_resolved_sync_to_sb(node, lflow_data); >> + return EN_HANDLED_UPDATED; >> + } >> + >> + const struct engine_context *eng_ctx = engine_get_context(); >> + struct all_synced_datapaths *all_dps = >> + engine_get_input_data("datapath_sync", node); >> + >> + const struct sbrec_logical_flow_table *sb_flow_table = >> + EN_OVSDB_GET(engine_get_input("SB_logical_flow", node)); >> + const struct sbrec_logical_dp_group_table *sb_dpgrp_table = >> + EN_OVSDB_GET(engine_get_input("SB_logical_dp_group", node)); >> + >> + struct ed_type_global_config *global_config = >> + engine_get_input_data("global_config", node); >> > > nit: We usually get all the input data before doing any kind of operation. > will change for v2 > > >> + >> + struct hmapx_node *hmapx_node; >> + HMAPX_FOR_EACH (hmapx_node, &lflow_data->dirty_lflow_refs) { >> + struct lflow_ref *ref = hmapx_node->data; >> + if (!lflow_ref_sync_lflows( >> + ref, lflow_data->lflow_table, >> + eng_ctx->ovnsb_idl_txn, >> + all_dps->synced_dps, >> + global_config->ovn_internal_version_changed, >> + sb_flow_table, sb_dpgrp_table)) { >> + return EN_UNHANDLED; >> + } >> + } >> + hmapx_clear(&lflow_data->dirty_lflow_refs); >> + >> + return EN_HANDLED_UPDATED; >> +} >> + >> +void >> +en_dp_group_resolved_cleanup(void *data OVS_UNUSED) >> +{ >> +} >> diff --git a/northd/en-dp-group-resolved.h b/northd/en-dp-group-resolved.h >> new file mode 100644 >> index 000000000..9f25072c1 >> --- /dev/null >> +++ b/northd/en-dp-group-resolved.h >> @@ -0,0 +1,30 @@ >> +/* >> + * Copyright (c) 2026, Red Hat, Inc. >> + * >> + * Licensed under the Apache License, Version 2.0 (the "License"); >> + * you may not use this file except in compliance with the License. >> + * You may obtain a copy of the License at: >> + * >> + * http://www.apache.org/licenses/LICENSE-2.0 >> + * >> + * Unless required by applicable law or agreed to in writing, software >> + * distributed under the License is distributed on an "AS IS" BASIS, >> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >> implied. >> + * See the License for the specific language governing permissions and >> + * limitations under the License. >> + */ >> + >> +#ifndef EN_DP_GROUP_RESOLVED_H >> +#define EN_DP_GROUP_RESOLVED_H 1 >> + >> +#include "lib/inc-proc-eng.h" >> + >> +void *en_dp_group_resolved_init(struct engine_node *node, >> + struct engine_arg *arg); >> +enum engine_node_state en_dp_group_resolved_run(struct engine_node *node, >> + void *data); >> +void en_dp_group_resolved_cleanup(void *data); >> +enum engine_input_handler_result >> +dp_group_resolved_lflow_handler(struct engine_node *node, void *data); >> + >> +#endif /* EN_DP_GROUP_RESOLVED_H */ >> diff --git a/northd/en-lflow.c b/northd/en-lflow.c >> index 8cb987777..38b1639d6 100644 >> --- a/northd/en-lflow.c >> +++ b/northd/en-lflow.c >> @@ -68,10 +68,6 @@ lflow_get_input_data(struct engine_node *node, >> struct all_synced_datapaths *all_dps = >> engine_get_input_data("datapath_sync", node); >> >> - lflow_input->sbrec_logical_flow_table = >> - EN_OVSDB_GET(engine_get_input("SB_logical_flow", node)); >> - lflow_input->sbrec_logical_dp_group_table = >> - EN_OVSDB_GET(engine_get_input("SB_logical_dp_group", node)); >> lflow_input->sbrec_acl_id_table = >> EN_OVSDB_GET(engine_get_input("SB_acl_id", node)); >> >> @@ -119,8 +115,6 @@ lflow_get_input_data(struct engine_node *node, >> enum engine_node_state >> en_lflow_run(struct engine_node *node, void *data) >> { >> - const struct engine_context *eng_ctx = engine_get_context(); >> - >> struct lflow_input lflow_input; >> lflow_get_input_data(node, &lflow_input); >> >> @@ -129,9 +123,10 @@ en_lflow_run(struct engine_node *node, void *data) >> search_mode == LFLOW_TABLE_SEARCH_FIELDS); >> lflow_reset_northd_refs(&lflow_input); >> lflow_ref_clear(lflow_input.igmp_lflow_ref); >> + hmapx_clear(&lflow_data->dirty_lflow_refs); >> + lflow_data->needs_full_sync = false; >> >> - build_lflows(eng_ctx->ovnsb_idl_txn, &lflow_input, >> - lflow_data->lflow_table); >> + build_lflows(&lflow_input, lflow_data->lflow_table); >> >> return EN_UPDATED; >> } >> @@ -149,31 +144,25 @@ lflow_northd_handler(struct engine_node *node, >> return EN_UNHANDLED; >> } >> >> - const struct engine_context *eng_ctx = engine_get_context(); >> struct lflow_data *lflow_data = data; >> >> struct lflow_input lflow_input; >> lflow_get_input_data(node, &lflow_input); >> >> - if (!lflow_handle_northd_lr_changes(eng_ctx->ovnsb_idl_txn, >> - >> &northd_data->trk_data.trk_routers, >> - &lflow_input, >> - lflow_data->lflow_table)) { >> - return EN_UNHANDLED; >> - } >> + lflow_handle_northd_lr_changes(&northd_data->trk_data.trk_routers, >> + &lflow_input, >> + lflow_data->lflow_table, >> + &lflow_data->dirty_lflow_refs); >> >> - if (!lflow_handle_northd_port_changes(eng_ctx->ovnsb_idl_txn, >> - >> &northd_data->trk_data.trk_lsps, >> - &lflow_input, >> - lflow_data->lflow_table)) { >> - return EN_UNHANDLED; >> - } >> + lflow_handle_northd_port_changes(&northd_data->trk_data.trk_lsps, >> + &lflow_input, >> + lflow_data->lflow_table, >> + &lflow_data->dirty_lflow_refs); >> >> - if (!lflow_handle_northd_lb_changes( >> - eng_ctx->ovnsb_idl_txn, &northd_data->trk_data.trk_lbs, >> - &lflow_input, lflow_data->lflow_table)) { >> - return EN_UNHANDLED; >> - } >> + lflow_handle_northd_lb_changes(&northd_data->trk_data.trk_lbs, >> + &lflow_input, >> + lflow_data->lflow_table, >> + &lflow_data->dirty_lflow_refs); >> >> return EN_HANDLED_UPDATED; >> } >> @@ -189,17 +178,14 @@ lflow_lr_stateful_handler(struct engine_node *node, >> void *data) >> return EN_UNHANDLED; >> } >> >> - const struct engine_context *eng_ctx = engine_get_context(); >> struct lflow_data *lflow_data = data; >> struct lflow_input lflow_input; >> >> lflow_get_input_data(node, &lflow_input); >> - if (!lflow_handle_lr_stateful_changes(eng_ctx->ovnsb_idl_txn, >> - &lr_sful_data->trk_data, >> - &lflow_input, >> - lflow_data->lflow_table)) { >> - return EN_UNHANDLED; >> - } >> + lflow_handle_lr_stateful_changes(&lr_sful_data->trk_data, >> + &lflow_input, >> + lflow_data->lflow_table, >> + &lflow_data->dirty_lflow_refs); >> >> return EN_HANDLED_UPDATED; >> } >> @@ -214,17 +200,14 @@ lflow_ls_stateful_handler(struct engine_node *node, >> void *data) >> return EN_UNHANDLED; >> } >> >> - const struct engine_context *eng_ctx = engine_get_context(); >> struct lflow_data *lflow_data = data; >> struct lflow_input lflow_input; >> >> lflow_get_input_data(node, &lflow_input); >> - if (!lflow_handle_ls_stateful_changes(eng_ctx->ovnsb_idl_txn, >> - &ls_sful_data->trk_data, >> - &lflow_input, >> - lflow_data->lflow_table)) { >> - return EN_UNHANDLED; >> - } >> + lflow_handle_ls_stateful_changes(&ls_sful_data->trk_data, >> + &lflow_input, >> + lflow_data->lflow_table, >> + &lflow_data->dirty_lflow_refs); >> >> return EN_HANDLED_UPDATED; >> } >> @@ -235,35 +218,20 @@ lflow_multicast_igmp_handler(struct engine_node >> *node, void *data) >> struct multicast_igmp_data *mcast_igmp_data = >> engine_get_input_data("multicast_igmp", node); >> >> - const struct engine_context *eng_ctx = engine_get_context(); >> struct lflow_data *lflow_data = data; >> struct lflow_input lflow_input; >> lflow_get_input_data(node, &lflow_input); >> >> - if (!lflow_ref_resync_flows(mcast_igmp_data->lflow_ref, >> - lflow_data->lflow_table, >> - eng_ctx->ovnsb_idl_txn, >> - lflow_input.dps, >> - lflow_input.ovn_internal_version_changed, >> - lflow_input.sbrec_logical_flow_table, >> - >> lflow_input.sbrec_logical_dp_group_table)) { >> - return EN_UNHANDLED; >> - } >> + lflow_ref_unlink_and_prune(mcast_igmp_data->lflow_ref, >> + lflow_data->lflow_table); >> >> build_igmp_lflows(&mcast_igmp_data->igmp_groups, >> &lflow_input.ls_datapaths->datapaths, >> lflow_data->lflow_table, >> mcast_igmp_data->lflow_ref); >> >> - if (!lflow_ref_sync_lflows(mcast_igmp_data->lflow_ref, >> - lflow_data->lflow_table, >> - eng_ctx->ovnsb_idl_txn, >> - lflow_input.dps, >> - lflow_input.ovn_internal_version_changed, >> - lflow_input.sbrec_logical_flow_table, >> - >> lflow_input.sbrec_logical_dp_group_table)) { >> - return EN_UNHANDLED; >> - } >> + lflow_data->needs_full_sync = true; >> + hmapx_add(&lflow_data->dirty_lflow_refs, mcast_igmp_data->lflow_ref); >> >> return EN_HANDLED_UPDATED; >> } >> @@ -280,7 +248,6 @@ lflow_group_ecmp_route_change_handler(struct >> engine_node *node, >> return EN_UNHANDLED; >> } >> >> - const struct engine_context *eng_ctx = engine_get_context(); >> struct lflow_data *lflow_data = data; >> >> struct lflow_input lflow_input; >> @@ -289,25 +256,13 @@ lflow_group_ecmp_route_change_handler(struct >> engine_node *node, >> struct group_ecmp_datapath *route_node; >> struct hmapx_node *hmapx_node; >> >> - /* We need to handle deletions before additions as they could >> potentially >> - * overlap. */ >> HMAPX_FOR_EACH (hmapx_node, >> >> &group_ecmp_route_data->trk_data.deleted_datapath_routes) { >> route_node = hmapx_node->data; >> lflow_ref_unlink_lflows(route_node->lflow_ref); >> - >> - bool handled = lflow_ref_sync_lflows( >> - route_node->lflow_ref, lflow_data->lflow_table, >> - eng_ctx->ovnsb_idl_txn, lflow_input.dps, >> - lflow_input.ovn_internal_version_changed, >> - lflow_input.sbrec_logical_flow_table, >> - lflow_input.sbrec_logical_dp_group_table); >> - if (!handled) { >> - return EN_UNHANDLED; >> - } >> + hmapx_add(&lflow_data->dirty_lflow_refs, route_node->lflow_ref); >> } >> >> - /* Now we handle created or updated route nodes. */ >> struct hmapx *crupdated_datapath_routes = >> &group_ecmp_route_data->trk_data.crupdated_datapath_routes; >> HMAPX_FOR_EACH (hmapx_node, crupdated_datapath_routes) { >> @@ -316,16 +271,7 @@ lflow_group_ecmp_route_change_handler(struct >> engine_node *node, >> build_route_data_flows_for_lrouter( >> route_node->od, lflow_data->lflow_table, >> route_node, lflow_input.bfd_ports); >> - >> - bool handled = lflow_ref_sync_lflows( >> - route_node->lflow_ref, lflow_data->lflow_table, >> - eng_ctx->ovnsb_idl_txn, lflow_input.dps, >> - lflow_input.ovn_internal_version_changed, >> - lflow_input.sbrec_logical_flow_table, >> - lflow_input.sbrec_logical_dp_group_table); >> - if (!handled) { >> - return EN_UNHANDLED; >> - } >> + hmapx_add(&lflow_data->dirty_lflow_refs, route_node->lflow_ref); >> } >> >> return EN_HANDLED_UPDATED; >> @@ -338,7 +284,6 @@ lflow_ic_learned_svc_mons_handler(struct engine_node >> *node, >> struct ic_learned_svc_monitors_data *ic_learned_svc_monitors_data = >> engine_get_input_data("ic_learned_svcs", node); >> >> - const struct engine_context *eng_ctx = engine_get_context(); >> struct lflow_data *lflow_data = data; >> struct lflow_input lflow_input; >> lflow_get_input_data(node, &lflow_input); >> @@ -349,16 +294,8 @@ lflow_ic_learned_svc_mons_handler(struct engine_node >> *node, >> &ic_learned_svc_monitors_data->ic_learned_svc_monitors_map, >> ic_learned_svc_monitors_data->lflow_ref); >> >> - if (!lflow_ref_resync_flows( >> - ic_learned_svc_monitors_data->lflow_ref, >> - lflow_data->lflow_table, >> - eng_ctx->ovnsb_idl_txn, >> - lflow_input.dps, >> - lflow_input.ovn_internal_version_changed, >> - lflow_input.sbrec_logical_flow_table, >> - lflow_input.sbrec_logical_dp_group_table)) { >> - return EN_UNHANDLED; >> - } >> + lflow_ref_unlink_and_prune(ic_learned_svc_monitors_data->lflow_ref, >> + lflow_data->lflow_table); >> >> build_lswitch_arp_nd_ic_learned_svc_mon( >> &svc_mons_data, >> @@ -366,16 +303,9 @@ lflow_ic_learned_svc_mons_handler(struct engine_node >> *node, >> lflow_input.svc_monitor_mac, >> lflow_data->lflow_table); >> >> - if (!lflow_ref_sync_lflows( >> - ic_learned_svc_monitors_data->lflow_ref, >> - lflow_data->lflow_table, >> - eng_ctx->ovnsb_idl_txn, >> - lflow_input.dps, >> - lflow_input.ovn_internal_version_changed, >> - lflow_input.sbrec_logical_flow_table, >> - lflow_input.sbrec_logical_dp_group_table)) { >> - return EN_UNHANDLED; >> - } >> + lflow_data->needs_full_sync = true; >> + hmapx_add(&lflow_data->dirty_lflow_refs, >> + ic_learned_svc_monitors_data->lflow_ref); >> >> return EN_HANDLED_UPDATED; >> } >> @@ -386,6 +316,7 @@ void *en_lflow_init(struct engine_node *node >> OVS_UNUSED, >> struct lflow_data *data = xmalloc(sizeof *data); >> data->lflow_table = lflow_table_alloc(); >> lflow_table_init(data->lflow_table); >> + hmapx_init(&data->dirty_lflow_refs); >> return data; >> } >> >> @@ -393,4 +324,5 @@ void en_lflow_cleanup(void *data_) >> { >> struct lflow_data *data = data_; >> lflow_table_destroy(data->lflow_table); >> + hmapx_destroy(&data->dirty_lflow_refs); >> } >> diff --git a/northd/en-lflow.h b/northd/en-lflow.h >> index 99bcfda15..5dab6cc19 100644 >> --- a/northd/en-lflow.h >> +++ b/northd/en-lflow.h >> @@ -7,12 +7,15 @@ >> #include <stdlib.h> >> #include <stdio.h> >> >> +#include "lib/hmapx.h" >> #include "lib/inc-proc-eng.h" >> >> struct lflow_table; >> >> struct lflow_data { >> struct lflow_table *lflow_table; >> + struct hmapx dirty_lflow_refs; /* lflow_refs changed by handlers. */ >> + bool needs_full_sync; /* Full lflow_table_sync_to_sb >> needed. */ >> > > This has very strange relation, it is cleared by the sync node? > Shouldn't this be part of the tracked_data and cleared before > every engine run? That's why we have those. Same goes for the > hmapx, it should be modified only by lflow node and consumed > by the sync. > will add lflow_tracked_data{} > > }; >> >> enum engine_node_state en_lflow_run(struct engine_node *node, void >> *data); >> diff --git a/northd/en-northd-output.c b/northd/en-northd-output.c >> index b492a771c..ed8dec0ac 100644 >> --- a/northd/en-northd-output.c >> +++ b/northd/en-northd-output.c >> @@ -51,8 +51,8 @@ northd_output_sync_to_sb_handler(struct engine_node >> *node OVS_UNUSED, >> } >> >> enum engine_input_handler_result >> -northd_output_lflow_handler(struct engine_node *node OVS_UNUSED, >> - void *data OVS_UNUSED) >> +northd_output_dp_group_resolved_handler(struct engine_node *node >> OVS_UNUSED, >> + void *data OVS_UNUSED) >> { >> return EN_HANDLED_UPDATED; >> } >> diff --git a/northd/en-northd-output.h b/northd/en-northd-output.h >> index b7053e60c..bb5e35900 100644 >> --- a/northd/en-northd-output.h >> +++ b/northd/en-northd-output.h >> @@ -13,7 +13,8 @@ enum engine_input_handler_result >> northd_output_sync_to_sb_handler(struct engine_node *node, >> void *data OVS_UNUSED); >> enum engine_input_handler_result >> -northd_output_lflow_handler(struct engine_node *node, void *data >> OVS_UNUSED); >> +northd_output_dp_group_resolved_handler(struct engine_node *node, >> + void *data OVS_UNUSED); >> enum engine_input_handler_result >> northd_output_mac_binding_aging_handler(struct engine_node *node, >> void *data OVS_UNUSED); >> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c >> index d2f28666c..4c0c8acaa 100644 >> --- a/northd/inc-proc-northd.c >> +++ b/northd/inc-proc-northd.c >> @@ -37,6 +37,7 @@ >> #include "en-multicast.h" >> #include "en-northd.h" >> #include "en-lflow.h" >> +#include "en-dp-group-resolved.h" >> #include "en-northd-output.h" >> #include "en-meters.h" >> #include "en-sampling-app.h" >> @@ -160,7 +161,8 @@ enum sb_engine_node { >> static ENGINE_NODE(northd, CLEAR_TRACKED_DATA, SB_WRITE); >> static ENGINE_NODE(sync_from_sb, SB_WRITE); >> static ENGINE_NODE(sampling_app); >> -static ENGINE_NODE(lflow, SB_WRITE); >> +static ENGINE_NODE(lflow); >> +static ENGINE_NODE(dp_group_resolved, SB_WRITE); >> static ENGINE_NODE(mac_binding_aging, SB_WRITE); >> static ENGINE_NODE(mac_binding_aging_waker); >> static ENGINE_NODE(northd_output); >> @@ -399,9 +401,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >> engine_add_input(&en_multicast_igmp, &en_sb_igmp_group, NULL); >> >> engine_add_input(&en_lflow, &en_sync_meters, NULL); >> - engine_add_input(&en_lflow, &en_sb_logical_flow, NULL); >> engine_add_input(&en_lflow, &en_sb_multicast_group, NULL); >> - engine_add_input(&en_lflow, &en_sb_logical_dp_group, NULL); >> engine_add_input(&en_lflow, &en_bfd_sync, NULL); >> engine_add_input(&en_lflow, &en_route_policies, NULL); >> /* Route changes are propagated to en_lflow through the >> en_group_ecmp_route >> @@ -436,6 +436,25 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >> engine_add_input(&en_lflow, &en_ic_learned_svc_monitors, >> lflow_ic_learned_svc_mons_handler); >> >> + engine_add_input(&en_dp_group_resolved, &en_lflow, >> + dp_group_resolved_lflow_handler); >> + engine_add_input(&en_dp_group_resolved, &en_sb_logical_flow, NULL); >> + engine_add_input(&en_dp_group_resolved, &en_sb_logical_dp_group, >> NULL); >> + /* dp_group_resolved reads synced datapath arrays (dps[DP_MAX]) when >> + * calling lflow_table_sync_to_sb / lflow_ref_sync_lflows. A noop >> + * handler is sufficient because en_lflow already recomputes when >> + * datapath_sync changes; dp_group_resolved just needs access to >> + * the data. */ >> + engine_add_input(&en_dp_group_resolved, &en_datapath_sync, >> + engine_noop_handler); >> + /* dp_group_resolved reads ovn_internal_version_changed from >> + * global_config when syncing flows to SB. Changes to >> + * global_config propagate through en_lflow (which has its own >> + * global_config handler), so a noop handler here avoids a >> + * redundant recompute. */ >> + engine_add_input(&en_dp_group_resolved, &en_global_config, >> + engine_noop_handler); >> + >> engine_add_input(&en_sync_to_sb_addr_set, &en_northd, NULL); >> engine_add_input(&en_sync_to_sb_addr_set, &en_lr_stateful, NULL); >> engine_add_input(&en_sync_to_sb_addr_set, &en_sb_address_set, NULL); >> @@ -490,8 +509,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >> engine_add_input(&en_northd_output, &en_sync_from_sb, NULL); >> engine_add_input(&en_northd_output, &en_sync_to_sb, >> northd_output_sync_to_sb_handler); >> - engine_add_input(&en_northd_output, &en_lflow, >> - northd_output_lflow_handler); >> + engine_add_input(&en_northd_output, &en_dp_group_resolved, >> + northd_output_dp_group_resolved_handler); >> engine_add_input(&en_northd_output, &en_mac_binding_aging, >> northd_output_mac_binding_aging_handler); >> engine_add_input(&en_northd_output, &en_fdb_aging, >> diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c >> index ce9c4f854..7a61212bc 100644 >> --- a/northd/lflow-mgr.c >> +++ b/northd/lflow-mgr.c >> @@ -261,6 +261,28 @@ lflow_table_set_size(struct lflow_table >> *lflow_table, size_t size) >> lflow_table->entries.n = size; >> } >> >> +/* An lflow that applies to no datapaths (its dp group bitmap is empty) >> must >> + * not exist in the SB DB. Such lflows can be left in the table by >> en_lflow's >> + * incremental handlers, which unlink lflows (clearing their dp bits) >> and defer >> + * the teardown to the dp-group-resolved sync stage. Release the >> lflow's dp >> + * group and destroy it; its SB row (if any) is removed by the caller. >> Returns >> + * true if the lflow was destroyed. */ >> +static bool >> +lflow_prune_if_no_datapaths(struct lflow_table *lflow_table, >> + struct ovn_lflow *lflow) >> +{ >> + if (dynamic_bitmap_count1(&lflow->dpg_bitmap)) { >> + return false; >> + } >> + >> + enum ovn_datapath_type dp_type = >> ovn_stage_to_datapath_type(lflow->stage); >> + ovs_assert(dp_type < DP_MAX); >> + ovn_dp_group_release(&lflow_table->dp_groups[dp_type], lflow->dpg); >> + lflow->dpg = NULL; >> + ovn_lflow_destroy(lflow_table, lflow); >> + return true; >> +} >> + >> void >> lflow_table_sync_to_sb(struct lflow_table *lflow_table, >> struct ovsdb_idl_txn *ovnsb_txn, >> @@ -287,6 +309,12 @@ lflow_table_sync_to_sb(struct lflow_table >> *lflow_table, >> ovn_lflow_destroy(lflow_table, lflow); >> continue; >> } >> + /* An lflow with no datapaths must be removed from the SB DB. >> It is >> + * skipped here (not added to 'sb_uuid_set'), so its SB row, if >> any, is >> + * deleted by the reconciliation loop below. */ >> + if (lflow_prune_if_no_datapaths(lflow_table, lflow)) { >> + continue; >> + } >> sbflow = NULL; >> if (!uuid_is_zero(&lflow->sb_uuid)) { >> sbflow = sbrec_logical_flow_table_get_for_uuid(sb_flow_table, >> @@ -372,7 +400,7 @@ lflow_table_sync_to_sb(struct lflow_table >> *lflow_table, >> lflows, &stage, >> sbflow->priority, sbflow->match, sbflow->actions, >> sbflow->controller_meter, acl_ct_translation, sbflow->hash); >> - if (lflow) { >> + if (lflow && !lflow_prune_if_no_datapaths(lflow_table, lflow)) { >> const struct ovn_synced_datapaths *datapaths; >> struct hmap *dp_groups; >> dp_groups = &lflow_table->dp_groups[dp_type]; >> @@ -393,6 +421,9 @@ lflow_table_sync_to_sb(struct lflow_table >> *lflow_table, >> if (search_mode != LFLOW_TABLE_SEARCH_FIELDS) { >> break; >> } >> + if (lflow_prune_if_no_datapaths(lflow_table, lflow)) { >> + continue; >> + } >> const struct ovn_synced_datapaths *datapaths; >> struct hmap *dp_groups; >> enum ovn_datapath_type dp_type = >> @@ -681,20 +712,32 @@ lflow_ref_unlink_lflows(struct lflow_ref *lflow_ref) >> } >> } >> >> -bool >> -lflow_ref_resync_flows(struct lflow_ref *lflow_ref, >> - struct lflow_table *lflow_table, >> - struct ovsdb_idl_txn *ovnsb_txn, >> - const struct ovn_synced_datapaths dps[DP_MAX], >> - bool ovn_internal_version_changed, >> - const struct sbrec_logical_flow_table >> *sbflow_table, >> - const struct sbrec_logical_dp_group_table >> *dpgrp_table) >> +/* Unlinks and destroys all lrns in 'lflow_ref', then destroys any lflow >> + * whose referenced_by list is empty (no other lflow_ref references it). >> + * Unlike lflow_ref_unlink_lflows (which only clears dp bits and sets >> + * linked=false), this function removes the lrns and orphaned lflows >> + * from the in-memory table entirely, without writing to SB. */ >> +void >> +lflow_ref_unlink_and_prune(struct lflow_ref *lflow_ref, >> + struct lflow_table *lflow_table) >> { >> lflow_ref_unlink_lflows(lflow_ref); >> - return lflow_ref_sync_lflows__(lflow_ref, lflow_table, ovnsb_txn, >> - dps, >> - ovn_internal_version_changed, >> sbflow_table, >> - dpgrp_table); >> + >> + struct lflow_ref_node *lrn; >> + HMAP_FOR_EACH_SAFE (lrn, ref_node, &lflow_ref->lflow_ref_nodes) { >> + struct ovn_lflow *lflow = lrn->lflow; >> + lflow_ref_node_destroy(lrn); >> + >> + if (ovs_list_is_empty(&lflow->referenced_by)) { >> + enum ovn_datapath_type dp_type = >> + ovn_stage_to_datapath_type(lflow->stage); >> + ovs_assert(dp_type < DP_MAX); >> + ovn_dp_group_release(&lflow_table->dp_groups[dp_type], >> + lflow->dpg); >> + lflow->dpg = NULL; >> + ovn_lflow_destroy(lflow_table, lflow); >> + } >> + } >> } >> >> bool >> @@ -781,10 +824,78 @@ lflow_table_add_lflow__(struct lflow_table >> *lflow_table, >> } >> ovs_list_insert(&lflow->referenced_by, &lrn->ref_list_node); >> hmap_insert(&lflow_ref->lflow_ref_nodes, &lrn->ref_node, >> hash); >> + } else if (sdp) { >> + /* Single-datapath (re-)add of an existing node. */ >> + if (!lrn->dpgrp_lflow) { >> + if (!lrn->linked) { >> + /* First add of this (re-)link cycle establishes the >> base >> + * datapath this reference tracks for the lflow. */ >> + lrn->dp_index = sdp->index; >> + } else if (lrn->dp_index != sdp->index) { >> + /* The same 'lflow_ref' references this lflow L(M, >> A) for a >> + * second datapath in this cycle (e.g. the single >> lflow_ref >> + * shared by all IGMP flows). A single-datapath >> + * lflow_ref_node tracks only one datapath index, so >> + * lflow_ref_unlink_lflows() would leave the other >> + * datapaths set in the lflow's dp group bitmap. >> Upgrade >> + * the node to track a datapath bitmap instead, so >> that >> + * unlinking clears every datapath this reference >> + * contributed. */ >> + size_t len = sparse_array_len(&sdp->dps->dps_array); >> + lrn->dpgrp_bitmap = bitmap_allocate(len); >> + lrn->dpgrp_bitmap_len = len; >> + bitmap_set1(lrn->dpgrp_bitmap, lrn->dp_index); >> + bitmap_set1(lrn->dpgrp_bitmap, sdp->index); >> + lrn->dpgrp_lflow = true; >> + >> + /* This reference already accounted for >> 'lrn->dp_index' >> + * (the first datapath it added this cycle) in the >> block >> + * below. This add contributes a second datapath, >> which >> + * the block below will not see because it only runs >> on >> + * the first link of the cycle. Account for it here >> so >> + * that a datapath shared with another reference is >> not >> + * released prematurely. */ >> + if (dynamic_bitmap_is_set(&lflow->dpg_bitmap, >> + sdp->index)) { >> + dp_refcnt_use(&lflow->dp_refcnts_map, >> sdp->index); >> + } >> + } >> + } else { >> + /* A node previously upgraded to track a datapath bitmap >> is >> + * re-linked one datapath at a time. */ >> + if (!lrn->linked) { >> + /* First add of this (re-)link cycle: rebuild the >> tracked >> + * bitmap from scratch so it reflects the current >> datapath >> + * set and the current datapath array length (which >> may >> + * have grown or shrunk since the node was last >> linked). */ >> + size_t len = sparse_array_len(&sdp->dps->dps_array); >> + bitmap_free(lrn->dpgrp_bitmap); >> + lrn->dpgrp_bitmap = bitmap_allocate(len); >> + lrn->dpgrp_bitmap_len = len; >> + } else if (!bitmap_is_set(lrn->dpgrp_bitmap, >> sdp->index)) { >> + /* A second or later datapath this reference >> contributes in >> + * the same cycle. The first datapath was accounted >> for by >> + * the block below (which only runs on the first >> link of >> + * the cycle); account for this one too so that a >> datapath >> + * shared with another reference is not released >> + * prematurely. */ >> + if (dynamic_bitmap_is_set(&lflow->dpg_bitmap, >> + sdp->index)) { >> + dp_refcnt_use(&lflow->dp_refcnts_map, >> sdp->index); >> + } >> + } >> + bitmap_set1(lrn->dpgrp_bitmap, sdp->index); >> + } >> } >> >> if (!lrn->linked) { >> - if (lrn->dpgrp_lflow) { >> + /* Allocate a reference counter only if the datapath(s) >> added by >> + * this reference are already used by the lflow. */ >> + if (sdp) { >> + if (dynamic_bitmap_is_set(&lflow->dpg_bitmap, >> sdp->index)) { >> + dp_refcnt_use(&lflow->dp_refcnts_map, sdp->index); >> + } >> + } else { >> ovs_assert(lrn->dpgrp_bitmap_len == dp_bitmap_len); >> size_t index; >> BITMAP_FOR_EACH_1 (index, dp_bitmap_len, dp_bitmap) { >> @@ -793,11 +904,6 @@ lflow_table_add_lflow__(struct lflow_table >> *lflow_table, >> dp_refcnt_use(&lflow->dp_refcnts_map, index); >> } >> } >> - } else { >> - /* Allocate a reference counter only if already used. */ >> - if (dynamic_bitmap_is_set(&lflow->dpg_bitmap, >> lrn->dp_index)) { >> - dp_refcnt_use(&lflow->dp_refcnts_map, lrn->dp_index); >> - } >> } >> } >> lrn->linked = true; >> @@ -1231,17 +1337,12 @@ sync_lflow_to_sb(struct ovn_lflow *lflow, >> &lflow->dpg->dpg_uuid); >> >> if (!lflow->dpg->dp_group) { >> - /* Ideally this should not happen. But it can still >> happen >> - * due to 2 reasons: >> - * 1. There is a bug in the dp_group management. We >> should >> - * perhaps assert here. >> - * 2. A User or CMS may delete the logical_dp_groups in >> SB DB >> - * or clear the SB:Logical_flow.logical_dp_groups >> column >> - * (intentionally or accidentally) >> - * >> - * Because of (2) it is better to return false instead of >> - * assert,so that we recover from th inconsistent SB DB. >> - */ >> + /* The SB Logical_DP_Group row referenced by this >> in-memory dp >> + * group no longer exists. This can happen if a user or >> CMS >> + * deletes the SB:Logical_DP_Group rows or clears the >> + * SB:Logical_Flow.logical_dp_group column >> (intentionally or >> + * accidentally). Release the stale dp group and fall >> through >> + * to create a fresh one. */ >> static struct vlog_rate_limit rl = >> VLOG_RATE_LIMIT_INIT(1, 1); >> VLOG_WARN_RL(&rl, "SB Logical flow ["UUID_FMT"]'s " >> "logical_dp_group column is not set " >> @@ -1249,10 +1350,11 @@ sync_lflow_to_sb(struct ovn_lflow *lflow, >> "referencing the dp group ["UUID_FMT"]", >> UUID_ARGS(&sbflow->header_.uuid), >> UUID_ARGS(&lflow->dpg->dpg_uuid)); >> - lflow->sync_state = LFLOW_STALE; >> - return false; >> + ovn_dp_group_release(dp_groups, lflow->dpg); >> + lflow->dpg = NULL; >> > > Caught by AI: > > ovn_dp_group_get() can return an in-memory group shared by multiple > lflows. In that case ovn_dp_group_release() only drops this lflow's > reference, leaving the stale group in dp_groups, and > ovn_dp_group_create() inserts another group with the same bitmap. > Subsequent lflows can move to the new group, but sync_lflow_to_sb() only > increments the new group at the end; it does not release their > pre_sync_dpg references. The stale group and its reference counts can > therefore remain indefinitely. > > >> } >> - } else { >> + } >> + if (!lflow->dpg) { >> lflow->dpg = ovn_dp_group_create( >> ovnsb_txn, dp_groups, sbrec_dp_group, >> &lflow->dpg_bitmap, >> diff --git a/northd/lflow-mgr.h b/northd/lflow-mgr.h >> index 84d0b3e67..e924ffc22 100644 >> --- a/northd/lflow-mgr.h >> +++ b/northd/lflow-mgr.h >> @@ -58,13 +58,7 @@ struct lflow_ref *lflow_ref_create(void); >> void lflow_ref_destroy(struct lflow_ref *); >> void lflow_ref_clear(struct lflow_ref *lflow_ref); >> void lflow_ref_unlink_lflows(struct lflow_ref *); >> -bool lflow_ref_resync_flows(struct lflow_ref *, >> - struct lflow_table *lflow_table, >> - struct ovsdb_idl_txn *ovnsb_txn, >> - const struct ovn_synced_datapaths >> dps[DP_MAX], >> - bool ovn_internal_version_changed, >> - const struct sbrec_logical_flow_table *, >> - const struct sbrec_logical_dp_group_table *); >> +void lflow_ref_unlink_and_prune(struct lflow_ref *, struct lflow_table >> *); >> bool lflow_ref_sync_lflows(struct lflow_ref *, >> struct lflow_table *lflow_table, >> struct ovsdb_idl_txn *ovnsb_txn, >> diff --git a/northd/northd.c b/northd/northd.c >> index 47f76b108..fbc0068b7 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -20965,10 +20965,9 @@ void run_update_worker_pool(int n_threads) >> } >> } >> >> -/* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB >> database, >> - * constructing their contents based on the OVN_NB database. */ >> -void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, >> - struct lflow_input *input_data, >> +/* Builds the in-memory logical flow table from the OVN_NB database. >> + * The flows are synced to the SB database by en_dp_group_resolved. */ >> +void build_lflows(struct lflow_input *input_data, >> struct lflow_table *lflows) >> { >> struct svc_monitors_map_data svc_mons_data = >> @@ -21012,14 +21011,6 @@ void build_lflows(struct ovsdb_idl_txn >> *ovnsb_txn, >> /* Parallel build may result in a suboptimal hash. Resize the >> * lflow map to a correct size before doing lookups */ >> lflow_table_expand(lflows); >> - >> - stopwatch_start(LFLOWS_TO_SB_STOPWATCH_NAME, time_msec()); >> - lflow_table_sync_to_sb(lflows, ovnsb_txn, input_data->dps, >> - input_data->ovn_internal_version_changed, >> - input_data->sbrec_logical_flow_table, >> - input_data->sbrec_logical_dp_group_table); >> - >> - stopwatch_stop(LFLOWS_TO_SB_STOPWATCH_NAME, time_msec()); >> } >> >> void >> @@ -21064,24 +21055,17 @@ lflow_reset_northd_refs(struct lflow_input >> *lflow_input) >> } >> } >> >> -bool >> -lflow_handle_northd_lr_changes(struct ovsdb_idl_txn *ovnsb_txn, >> - struct tracked_dps *tracked_lrs, >> - struct lflow_input *lflow_input, >> - struct lflow_table *lflows) >> +void >> +lflow_handle_northd_lr_changes(struct tracked_dps *tracked_lrs, >> + struct lflow_input *lflow_input, >> + struct lflow_table *lflows, >> + struct hmapx *dirty_lflow_refs) >> { >> - bool handled = true; >> struct hmapx_node *hmapx_node; >> HMAPX_FOR_EACH (hmapx_node, &tracked_lrs->deleted) { >> struct ovn_datapath *od = hmapx_node->data; >> - handled = lflow_ref_resync_flows( >> - od->datapath_lflows, lflows, ovnsb_txn, lflow_input->dps, >> - lflow_input->ovn_internal_version_changed, >> - lflow_input->sbrec_logical_flow_table, >> - lflow_input->sbrec_logical_dp_group_table); >> - if (!handled) { >> - return handled; >> - } >> + lflow_ref_unlink_lflows(od->datapath_lflows); >> + hmapx_add(dirty_lflow_refs, od->datapath_lflows); >> } >> >> struct lswitch_flow_build_info lsi = { >> @@ -21099,34 +21083,18 @@ lflow_handle_northd_lr_changes(struct >> ovsdb_idl_txn *ovnsb_txn, >> >> lflow_ref_unlink_lflows(od->datapath_lflows); >> build_lswitch_and_lrouter_iterate_by_lr(od, &lsi); >> - } >> - >> - /* We need to make sure that all datapath groups are allocated before >> - * trying to sync logical flows. Otherwise, we would need to >> recompute >> - * those datapath groups within those flows over and over again. */ >> - HMAPX_FOR_EACH (hmapx_node, &tracked_lrs->crupdated) { >> - struct ovn_datapath *od = hmapx_node->data; >> - >> - handled = lflow_ref_sync_lflows( >> - od->datapath_lflows, lflows, ovnsb_txn, lflow_input->dps, >> - lflow_input->ovn_internal_version_changed, >> - lflow_input->sbrec_logical_flow_table, >> - lflow_input->sbrec_logical_dp_group_table); >> - if (!handled) { >> - break; >> - } >> + hmapx_add(dirty_lflow_refs, od->datapath_lflows); >> } >> >> ds_destroy(&lsi.actions); >> ds_destroy(&lsi.match); >> - return handled; >> } >> >> -bool >> -lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, >> - struct tracked_ovn_ports *trk_lsps, >> +void >> +lflow_handle_northd_port_changes(struct tracked_ovn_ports *trk_lsps, >> struct lflow_input *lflow_input, >> - struct lflow_table *lflows) >> + struct lflow_table *lflows, >> + struct hmapx *dirty_lflow_refs) >> { >> struct hmapx_node *hmapx_node; >> struct ovn_port *op; >> @@ -21135,14 +21103,8 @@ lflow_handle_northd_port_changes(struct >> ovsdb_idl_txn *ovnsb_txn, >> op = hmapx_node->data; >> /* Make sure 'op' is an lsp and not lrp. */ >> ovs_assert(op->nbsp); >> - bool handled = lflow_ref_resync_flows( >> - op->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, >> - lflow_input->ovn_internal_version_changed, >> - lflow_input->sbrec_logical_flow_table, >> - lflow_input->sbrec_logical_dp_group_table); >> - if (!handled) { >> - return false; >> - } >> + lflow_ref_unlink_lflows(op->lflow_ref); >> + hmapx_add(dirty_lflow_refs, op->lflow_ref); >> /* No need to update SB multicast groups, thanks to weak >> * references. */ >> } >> @@ -21163,33 +21125,17 @@ lflow_handle_northd_port_changes(struct >> ovsdb_idl_txn *ovnsb_txn, >> &match, &actions, >> >> lflow_input->svc_monitor_mac, >> lflows); >> - /* Sync the new flows to SB. */ >> - bool handled = lflow_ref_sync_lflows( >> - op->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, >> - lflow_input->ovn_internal_version_changed, >> - lflow_input->sbrec_logical_flow_table, >> - lflow_input->sbrec_logical_dp_group_table); >> - if (handled) { >> - /* Now regenerate the stateful lflows for 'op' */ >> - /* Clear old lflows. */ >> - lflow_ref_unlink_lflows(op->stateful_lflow_ref); >> - build_lbnat_lflows_iterate_by_lsp(op, >> - >> lflow_input->lr_stateful_table, >> - &match, &actions, lflows); >> - handled = lflow_ref_sync_lflows( >> - op->stateful_lflow_ref, lflows, ovnsb_txn, >> - lflow_input->dps, >> - lflow_input->ovn_internal_version_changed, >> - lflow_input->sbrec_logical_flow_table, >> - lflow_input->sbrec_logical_dp_group_table); >> - } >> + hmapx_add(dirty_lflow_refs, op->lflow_ref); >> + >> + /* Now regenerate the stateful lflows for 'op' */ >> + lflow_ref_unlink_lflows(op->stateful_lflow_ref); >> + build_lbnat_lflows_iterate_by_lsp(op, >> + lflow_input->lr_stateful_table, >> + &match, &actions, lflows); >> + hmapx_add(dirty_lflow_refs, op->stateful_lflow_ref); >> >> ds_destroy(&match); >> ds_destroy(&actions); >> - >> - if (!handled) { >> - return false; >> - } >> } >> >> HMAPX_FOR_EACH (hmapx_node, &trk_lsps->created) { >> @@ -21205,42 +21151,24 @@ lflow_handle_northd_port_changes(struct >> ovsdb_idl_txn *ovnsb_txn, >> &match, &actions, >> >> lflow_input->svc_monitor_mac, >> lflows); >> + hmapx_add(dirty_lflow_refs, op->lflow_ref); >> >> - /* Sync the newly added flows to SB. */ >> - bool handled = lflow_ref_sync_lflows( >> - op->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, >> - lflow_input->ovn_internal_version_changed, >> - lflow_input->sbrec_logical_flow_table, >> - lflow_input->sbrec_logical_dp_group_table); >> - if (handled) { >> - /* Now generate the stateful lflows for 'op' */ >> - build_lbnat_lflows_iterate_by_lsp(op, >> - >> lflow_input->lr_stateful_table, >> - &match, &actions, lflows); >> - handled = lflow_ref_sync_lflows( >> - op->stateful_lflow_ref, lflows, ovnsb_txn, >> - lflow_input->dps, >> - lflow_input->ovn_internal_version_changed, >> - lflow_input->sbrec_logical_flow_table, >> - lflow_input->sbrec_logical_dp_group_table); >> - } >> + /* Now generate the stateful lflows for 'op' */ >> + build_lbnat_lflows_iterate_by_lsp(op, >> + lflow_input->lr_stateful_table, >> + &match, &actions, lflows); >> + hmapx_add(dirty_lflow_refs, op->stateful_lflow_ref); >> >> ds_destroy(&match); >> ds_destroy(&actions); >> - >> - if (!handled) { >> - return false; >> - } >> } >> - >> - return true; >> } >> >> -bool >> -lflow_handle_northd_lb_changes(struct ovsdb_idl_txn *ovnsb_txn, >> - struct tracked_lbs *trk_lbs, >> +void >> +lflow_handle_northd_lb_changes(struct tracked_lbs *trk_lbs, >> struct lflow_input *lflow_input, >> - struct lflow_table *lflows) >> + struct lflow_table *lflows, >> + struct hmapx *dirty_lflow_refs) >> { >> struct ovn_lb_datapaths *lb_dps; >> struct hmapx_node *hmapx_node; >> @@ -21252,12 +21180,8 @@ lflow_handle_northd_lb_changes(struct >> ovsdb_idl_txn *ovnsb_txn, >> >> HMAPX_FOR_EACH (hmapx_node, &trk_lbs->deleted) { >> lb_dps = hmapx_node->data; >> - >> - lflow_ref_resync_flows( >> - lb_dps->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, >> - lflow_input->ovn_internal_version_changed, >> - lflow_input->sbrec_logical_flow_table, >> - lflow_input->sbrec_logical_dp_group_table); >> + lflow_ref_unlink_lflows(lb_dps->lflow_ref); >> + hmapx_add(dirty_lflow_refs, lb_dps->lflow_ref); >> } >> >> HMAPX_FOR_EACH (hmapx_node, &trk_lbs->crupdated) { >> @@ -21291,31 +21215,20 @@ lflow_handle_northd_lb_changes(struct >> ovsdb_idl_txn *ovnsb_txn, >> ds_destroy(&match); >> ds_destroy(&actions); >> >> - /* Sync the new flows to SB. */ >> - bool handled = lflow_ref_sync_lflows( >> - lb_dps->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, >> - lflow_input->ovn_internal_version_changed, >> - lflow_input->sbrec_logical_flow_table, >> - lflow_input->sbrec_logical_dp_group_table); >> - if (!handled) { >> - return false; >> - } >> + hmapx_add(dirty_lflow_refs, lb_dps->lflow_ref); >> } >> - >> - return true; >> } >> >> -bool >> -lflow_handle_lr_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn, >> - struct lr_stateful_tracked_data >> *trk_data, >> - struct lflow_input *lflow_input, >> - struct lflow_table *lflows) >> +void >> +lflow_handle_lr_stateful_changes(struct lr_stateful_tracked_data >> *trk_data, >> + struct lflow_input *lflow_input, >> + struct lflow_table *lflows, >> + struct hmapx *dirty_lflow_refs) >> { >> struct lr_stateful_record *lr_stateful_rec; >> struct ds actions = DS_EMPTY_INITIALIZER; >> struct ds match = DS_EMPTY_INITIALIZER; >> struct hmapx_node *hmapx_node; >> - bool handled = true; >> >> HMAPX_FOR_EACH (hmapx_node, &trk_data->crupdated) { >> lr_stateful_rec = hmapx_node->data; >> @@ -21328,17 +21241,7 @@ lflow_handle_lr_stateful_changes(struct >> ovsdb_idl_txn *ovnsb_txn, >> &match, &actions, >> lflow_input->meter_groups, >> lflow_input->features); >> - >> - /* Sync the new flows to SB. */ >> - handled = lflow_ref_sync_lflows( >> - lr_stateful_rec->lflow_ref, lflows, ovnsb_txn, >> - lflow_input->dps, >> - lflow_input->ovn_internal_version_changed, >> - lflow_input->sbrec_logical_flow_table, >> - lflow_input->sbrec_logical_dp_group_table); >> - if (!handled) { >> - goto exit; >> - } >> + hmapx_add(dirty_lflow_refs, lr_stateful_rec->lflow_ref); >> >> const struct ovn_datapath *od = >> ovn_datapaths_find_by_index(lflow_input->lr_datapaths, >> @@ -21353,16 +21256,7 @@ lflow_handle_lr_stateful_changes(struct >> ovsdb_idl_txn *ovnsb_txn, >> lflow_input->bfd_ports, >> &match, &actions, >> lflows); >> - >> - handled = lflow_ref_sync_lflows( >> - op->stateful_lflow_ref, lflows, ovnsb_txn, >> - lflow_input->dps, >> - lflow_input->ovn_internal_version_changed, >> - lflow_input->sbrec_logical_flow_table, >> - lflow_input->sbrec_logical_dp_group_table); >> - if (!handled) { >> - goto exit; >> - } >> + hmapx_add(dirty_lflow_refs, op->stateful_lflow_ref); >> >> if (op->peer && op->peer->nbsp) { >> lflow_ref_unlink_lflows(op->peer->stateful_lflow_ref); >> @@ -21370,32 +21264,20 @@ lflow_handle_lr_stateful_changes(struct >> ovsdb_idl_txn *ovnsb_txn, >> build_lbnat_lflows_iterate_by_lsp( >> op->peer, lflow_input->lr_stateful_table, &match, >> &actions, >> lflows); >> - >> - handled = lflow_ref_sync_lflows( >> - op->peer->stateful_lflow_ref, lflows, ovnsb_txn, >> - lflow_input->dps, >> - lflow_input->ovn_internal_version_changed, >> - lflow_input->sbrec_logical_flow_table, >> - lflow_input->sbrec_logical_dp_group_table); >> - if (!handled) { >> - goto exit; >> - } >> + hmapx_add(dirty_lflow_refs, >> op->peer->stateful_lflow_ref); >> } >> } >> } >> >> -exit: >> ds_destroy(&match); >> ds_destroy(&actions); >> - >> - return handled; >> } >> >> -bool >> -lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn, >> - struct ls_stateful_tracked_data >> *trk_data, >> - struct lflow_input *lflow_input, >> - struct lflow_table *lflows) >> +void >> +lflow_handle_ls_stateful_changes(struct ls_stateful_tracked_data >> *trk_data, >> + struct lflow_input *lflow_input, >> + struct lflow_table *lflows, >> + struct hmapx *dirty_lflow_refs) >> { >> struct hmapx_node *hmapx_node; >> >> @@ -21420,39 +21302,14 @@ lflow_handle_ls_stateful_changes(struct >> ovsdb_idl_txn *ovnsb_txn, >> build_network_function(od, lflows, >> lflow_input->ls_port_groups, >> ls_stateful_rec->lflow_ref); >> - } >> - >> - /* We need to make sure that all datapath groups are allocated before >> - * trying to sync logical flows. Otherwise, we would need to >> recompute >> - * those datapath groups within those flows over and over again. */ >> - HMAPX_FOR_EACH (hmapx_node, &trk_data->crupdated) { >> - struct ls_stateful_record *ls_stateful_rec = hmapx_node->data; >> - /* Sync the new flows to SB. */ >> - bool handled = lflow_ref_sync_lflows( >> - ls_stateful_rec->lflow_ref, lflows, ovnsb_txn, >> - lflow_input->dps, >> - lflow_input->ovn_internal_version_changed, >> - lflow_input->sbrec_logical_flow_table, >> - lflow_input->sbrec_logical_dp_group_table); >> - if (!handled) { >> - return false; >> - } >> + hmapx_add(dirty_lflow_refs, ls_stateful_rec->lflow_ref); >> } >> >> HMAPX_FOR_EACH (hmapx_node, &trk_data->deleted) { >> struct ls_stateful_record *ls_stateful_rec = hmapx_node->data; >> - >> - if (!lflow_ref_resync_flows( >> - ls_stateful_rec->lflow_ref, lflows, ovnsb_txn, >> - lflow_input->dps, >> - lflow_input->ovn_internal_version_changed, >> - lflow_input->sbrec_logical_flow_table, >> - lflow_input->sbrec_logical_dp_group_table)) { >> - return false; >> - } >> + lflow_ref_unlink_lflows(ls_stateful_rec->lflow_ref); >> + hmapx_add(dirty_lflow_refs, ls_stateful_rec->lflow_ref); >> } >> - >> - return true; >> } >> >> static bool >> diff --git a/northd/northd.h b/northd/northd.h >> index 2e3a9e00d..df2189c97 100644 >> --- a/northd/northd.h >> +++ b/northd/northd.h >> @@ -260,9 +260,7 @@ struct lflow_ref; >> struct lr_nat_table; >> >> struct lflow_input { >> - /* Southbound table references */ >> - const struct sbrec_logical_flow_table *sbrec_logical_flow_table; >> - const struct sbrec_logical_dp_group_table >> *sbrec_logical_dp_group_table; >> + /* Southbound table references (used during lflow computation). */ >> const struct sbrec_acl_id_table *sbrec_acl_id_table; >> >> /* Indexes */ >> @@ -979,8 +977,7 @@ struct lr_stateful_tracked_data; >> struct ls_stateful_tracked_data; >> struct group_ecmp_datapath; >> >> -void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, >> - struct lflow_input *input_data, >> +void build_lflows(struct lflow_input *input_data, >> struct lflow_table *); >> void lflow_reset_northd_refs(struct lflow_input *); >> void build_route_data_flows_for_lrouter( >> @@ -988,26 +985,26 @@ void build_route_data_flows_for_lrouter( >> const struct group_ecmp_datapath *route_node, >> const struct sset *bfd_ports); >> >> -bool lflow_handle_northd_lr_changes(struct ovsdb_idl_txn *ovnsh_txn, >> - struct tracked_dps *, >> - struct lflow_input *, >> - struct lflow_table *lflows); >> -bool lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, >> - struct tracked_ovn_ports *, >> +void lflow_handle_northd_lr_changes(struct tracked_dps *, >> + struct lflow_input *, >> + struct lflow_table *lflows, >> + struct hmapx *dirty_lflow_refs); >> +void lflow_handle_northd_port_changes(struct tracked_ovn_ports *, >> struct lflow_input *, >> - struct lflow_table *lflows); >> -bool lflow_handle_northd_lb_changes(struct ovsdb_idl_txn *ovnsb_txn, >> - struct tracked_lbs *, >> + struct lflow_table *lflows, >> + struct hmapx *dirty_lflow_refs); >> +void lflow_handle_northd_lb_changes(struct tracked_lbs *, >> struct lflow_input *, >> - struct lflow_table *lflows); >> -bool lflow_handle_lr_stateful_changes(struct ovsdb_idl_txn *, >> - struct lr_stateful_tracked_data *, >> + struct lflow_table *lflows, >> + struct hmapx *dirty_lflow_refs); >> +void lflow_handle_lr_stateful_changes(struct lr_stateful_tracked_data *, >> struct lflow_input *, >> - struct lflow_table *lflows); >> -bool lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *, >> - struct ls_stateful_tracked_data *, >> + struct lflow_table *lflows, >> + struct hmapx *dirty_lflow_refs); >> +void lflow_handle_ls_stateful_changes(struct ls_stateful_tracked_data *, >> struct lflow_input *, >> - struct lflow_table *lflows); >> + struct lflow_table *lflows, >> + struct hmapx *dirty_lflow_refs); >> bool northd_handle_sb_port_binding_changes( >> const struct sbrec_port_binding_table *, struct hmap *ls_ports, >> struct hmap *lr_ports); >> diff --git a/tests/ovn-inc-proc-graph-dump.at b/tests/ >> ovn-inc-proc-graph-dump.at >> index 44bf5689e..af951da8d 100644 >> --- a/tests/ovn-inc-proc-graph-dump.at >> +++ b/tests/ovn-inc-proc-graph-dump.at >> @@ -143,7 +143,6 @@ digraph "Incremental-Processing-Engine" { >> sync_meters -> sync_to_sb [[label=""]]; >> sync_to_sb_lb -> sync_to_sb [[label=""]]; >> sync_to_sb_pb -> sync_to_sb [[label=""]]; >> - SB_logical_flow [[style=filled, shape=box, fillcolor=white, >> label="SB_logical_flow"]]; >> SB_multicast_group [[style=filled, shape=box, fillcolor=white, >> label="SB_multicast_group"]]; >> NB_bfd [[style=filled, shape=box, fillcolor=white, >> label="NB_bfd"]]; >> SB_bfd [[style=filled, shape=box, fillcolor=white, >> label="SB_bfd"]]; >> @@ -186,9 +185,7 @@ digraph "Incremental-Processing-Engine" { >> SB_igmp_group -> multicast_igmp [[label=""]]; >> lflow [[style=filled, shape=box, fillcolor=white, label="lflow"]]; >> sync_meters -> lflow [[label=""]]; >> - SB_logical_flow -> lflow [[label=""]]; >> SB_multicast_group -> lflow [[label=""]]; >> - SB_logical_dp_group -> lflow [[label=""]]; >> bfd_sync -> lflow [[label=""]]; >> route_policies -> lflow [[label=""]]; >> routes -> lflow [[label="engine_noop_handler"]]; >> @@ -203,6 +200,13 @@ digraph "Incremental-Processing-Engine" { >> multicast_igmp -> lflow [[label="lflow_multicast_igmp_handler"]]; >> SB_acl_id -> lflow [[label=""]]; >> ic_learned_svc_monitors -> lflow >> [[label="lflow_ic_learned_svc_mons_handler"]]; >> + SB_logical_flow [[style=filled, shape=box, fillcolor=white, >> label="SB_logical_flow"]]; >> + dp_group_resolved [[style=filled, shape=box, fillcolor=white, >> label="dp_group_resolved"]]; >> + lflow -> dp_group_resolved >> [[label="dp_group_resolved_lflow_handler"]]; >> + SB_logical_flow -> dp_group_resolved [[label=""]]; >> + SB_logical_dp_group -> dp_group_resolved [[label=""]]; >> + datapath_sync -> dp_group_resolved >> [[label="engine_noop_handler"]]; >> + global_config -> dp_group_resolved >> [[label="engine_noop_handler"]]; >> mac_binding_aging_waker [[style=filled, shape=box, >> fillcolor=white, label="mac_binding_aging_waker"]]; >> mac_binding_aging [[style=filled, shape=box, fillcolor=white, >> label="mac_binding_aging"]]; >> SB_mac_binding -> mac_binding_aging [[label=""]]; >> @@ -237,7 +241,7 @@ digraph "Incremental-Processing-Engine" { >> acl_id -> northd_output [[label="northd_output_acl_id_handler"]]; >> sync_from_sb -> northd_output [[label=""]]; >> sync_to_sb -> northd_output >> [[label="northd_output_sync_to_sb_handler"]]; >> - lflow -> northd_output [[label="northd_output_lflow_handler"]]; >> + dp_group_resolved -> northd_output >> [[label="northd_output_dp_group_resolved_handler"]]; >> mac_binding_aging -> northd_output >> [[label="northd_output_mac_binding_aging_handler"]]; >> fdb_aging -> northd_output >> [[label="northd_output_fdb_aging_handler"]]; >> ecmp_nexthop -> northd_output >> [[label="northd_output_ecmp_nexthop_handler"]]; >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index d19978a62..2a070901b 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -13266,17 +13266,7 @@ check ovn-nbctl --wait=sb set load_balancer lb1 >> options:bar=foo >> check_engine_stats lflow norecompute compute >> CHECK_NO_CHANGE_AFTER_RECOMPUTE >> >> -# Clear the SB:Logical_Flow.logical_dp_groups column of all the >> -# logical flows and then modify the NB:Load_balancer. ovn-northd >> -# should resync the logical flows. >> -for l in $(ovn-sbctl --bare --columns _uuid list logical_flow) >> -do >> - check ovn-sbctl clear logical_flow $l logical_dp_group >> -done >> - >> -check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> > > Why was this removed? We should still check if the resync happens > in those cases. > The resync will not happen in these cases. sync_lflow_to_sb() no longer returns false for stale dp_groups. It releases the stale dpg, creates a new one, and returns true. So only the dirty lflow_refs are synced. All the rest are left alone. Before this patch sync_lflow_to_sb() returns false which triggered the full recompute and repairs the rest of the lflows. > > check ovn-nbctl --wait=sb set load_balancer lb1 options:foo=bar >> -check_engine_stats lflow recompute nocompute >> CHECK_NO_CHANGE_AFTER_RECOMPUTE >> >> lb_lflow_uuid=$(fetch_column Logical_flow _uuid match='"ct.new && >> ip4.dst == 10.0.0.10 && reg1[[16..23]] == 6 && reg1[[0..15]] == 80"') >> @@ -13445,16 +13435,7 @@ AT_CHECK([echo $dpgrp_dps | grep $sw3_uuid], >> [0], [ignore]) >> AT_CHECK([echo $dpgrp_dps | grep $sw4_uuid], [0], [ignore]) >> AT_CHECK([echo $dpgrp_dps | grep $sw5_uuid], [0], [ignore]) >> >> -# Clear the SB:Logical_Flow.logical_dp_groups column of all the >> -# logical flows and then modify the NB:Load_balancer. ovn-northd >> -# should resync the logical flows. >> -for l in $(ovn-sbctl --bare --columns _uuid list logical_flow) >> -do >> - check ovn-sbctl clear logical_flow $l logical_dp_group >> -done >> - >> check ovn-nbctl --wait=sb set load_balancer lb2 vips='{"10.0.0.10:80"=" >> 10.0.0.3:80,10.0.0.4:80"}' >> -check_engine_stats lflow recompute compute > > > Same here. > > >> CHECK_NO_CHANGE_AFTER_RECOMPUTE >> >> lb_lflow_uuid=$(fetch_column Logical_flow _uuid match='"ct.new && >> ip4.dst == 10.0.0.10 && reg1[[16..23]] == 6 && reg1[[0..15]] == 80"') >> @@ -23883,3 +23864,65 @@ AT_CHECK([as northd ovn-appctl -t ovn-northd >> inc-engine/enable-stopwatch nonexis >> OVN_CLEANUP_NORTHD >> AT_CLEANUP >> ]) >> + >> +AT_SETUP([Datapath group SB writes only when bitmap changes]) >> +ovn_start >> + >> +# Create three switches, each with an ACL so they share the >> +# "has ACLs" lflows in the same datapath group {sw0, sw1, sw2}. >> +check ovn-nbctl ls-add sw0 >> +check ovn-nbctl ls-add sw1 >> +check ovn-nbctl ls-add sw2 >> +check ovn-nbctl lsp-add sw0 sw0-p0 >> +check ovn-nbctl lsp-add sw1 sw1-p0 >> +check ovn-nbctl lsp-add sw2 sw2-p0 >> +check ovn-nbctl acl-add sw0 to-lport 1000 ip4 allow >> +check ovn-nbctl acl-add sw1 to-lport 1000 ip6 allow >> +check ovn-nbctl --wait=sb acl-add sw2 to-lport 1000 tcp allow >> + >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +# Record dp_group state (UUIDs + content). >> +ovn-sbctl --bare --columns=_uuid,datapaths list Logical_DP_Group \ >> + | sort > dpg_before >> + >> +# ---------------------------------------------------------- >> +# Case 1: Change that triggers incremental lflow compute but >> +# does NOT alter the dp-group bitmap. >> +# >> +# Setting a port address rebuilds the port's lflows but the >> +# set of datapaths that share the "has ACLs" lflows is still >> +# {sw0, sw1, sw2}. The dp_group rows should not change. >> +# ---------------------------------------------------------- >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +check ovn-nbctl --wait=sb lsp-set-addresses sw0-p0 "00:00:00:00:00:01" >> +check_engine_stats lflow norecompute compute >> + >> +ovn-sbctl --bare --columns=_uuid,datapaths list Logical_DP_Group \ >> + | sort > dpg_after >> +AT_CHECK([diff dpg_before dpg_after]) >> + >> +# ---------------------------------------------------------- >> +# Case 2: Change that removes a datapath from a shared flow, >> +# altering the dp-group bitmap. >> +# >> +# Move sw0's ACL to tier 1. The egress "acl action" flows >> +# now differ for sw0, so the {sw0, sw1, sw2} group loses sw0 >> +# and a {sw1, sw2} group appears (or the existing one is >> +# updated). The dp_group rows must change. >> +# ---------------------------------------------------------- >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> +acl0=$(fetch_column nb:ACL _uuid match=ip4) >> +check ovn-nbctl --wait=sb set ACL $acl0 tier=1 >> +check_engine_stats lflow norecompute compute >> + >> +ovn-sbctl --bare --columns=_uuid,datapaths list Logical_DP_Group \ >> + | sort > dpg_changed >> +AT_CHECK([diff dpg_before dpg_changed], [1], [ignore]) >> + >> +check ovn-nbctl --wait=sb ls-del sw0 >> + >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> + >> +OVN_CLEANUP_NORTHD >> +AT_CLEANUP >> -- >> 2.55.0 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Regards, > Ales > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
