Hi Mark, thank you for the review.
On Wed, Jun 11, 2025 at 12:10 AM Mark Michelson <[email protected]> wrote: > On 5/21/25 08:59, Ales Musil via dev wrote: > > Any change to ACL would result in full lflow recompute. Instead of > > that reprocess only the relevant logical switch. In order to get > > track the relation between ACL and LS from the ACL side we need to > > store all related ACL uuids in the ls_stateful_record. > > > > Reported-at: https://issues.redhat.com/browse/FDP-755 > > Signed-off-by: Ales Musil <[email protected]> > > --- > > v3: Rebase on top of latest main. > > v2: Rebase on top of latest main. > > Optimize the related ACLs and flags computation further. > > --- > > northd/en-ls-stateful.c | 178 +++++++++++++++------------------------ > > northd/en-ls-stateful.h | 6 ++ > > northd/inc-proc-northd.c | 2 +- > > tests/ovn-northd.at | 24 +++--- > > 4 files changed, 85 insertions(+), 125 deletions(-) > > > > diff --git a/northd/en-ls-stateful.c b/northd/en-ls-stateful.c > > index b713b5bce..88c850916 100644 > > --- a/northd/en-ls-stateful.c > > +++ b/northd/en-ls-stateful.c > > @@ -65,20 +65,13 @@ static void ls_stateful_record_destroy(struct > ls_stateful_record *); > > static void ls_stateful_record_init( > > struct ls_stateful_record *, > > const struct ovn_datapath *, > > - const struct ls_port_group *, > > - const struct ls_port_group_table *); > > -static void ls_stateful_record_reinit( > > - struct ls_stateful_record *, > > - const struct ovn_datapath *, > > - const struct ls_port_group *, > > const struct ls_port_group_table *); > > static bool ls_has_lb_vip(const struct ovn_datapath *); > > -static void ls_stateful_record_set_acl_flags( > > +static void ls_stateful_record_set_acls( > > struct ls_stateful_record *, const struct ovn_datapath *, > > - const struct ls_port_group *, const struct ls_port_group_table *); > > -static bool ls_stateful_record_set_acl_flags_(struct ls_stateful_record > *, > > - struct nbrec_acl **, > > - size_t n_acls); > > + const struct ls_port_group_table *); > > +static void ls_stateful_record_set_acls_(struct ls_stateful_record *, > > + struct nbrec_acl **, size_t > n_acls); > > static struct ls_stateful_input ls_stateful_get_input_data( > > struct engine_node *); > > > > @@ -148,30 +141,31 @@ ls_stateful_northd_handler(struct engine_node > *node, void *data_) > > struct ed_type_ls_stateful *data = data_; > > struct hmapx_node *hmapx_node; > > > > - struct hmapx changed_stateful_od = > HMAPX_INITIALIZER(&changed_stateful_od); > > HMAPX_FOR_EACH (hmapx_node, &nd_changes->ls_with_changed_lbs) { > > - hmapx_add(&changed_stateful_od, hmapx_node->data); > > - } > > + const struct ovn_datapath *od = hmapx_node->data; > > > > - HMAPX_FOR_EACH (hmapx_node, &nd_changes->ls_with_changed_acls) { > > - hmapx_add(&changed_stateful_od, hmapx_node->data); > > + struct ls_stateful_record *ls_stateful_rec = > > + ls_stateful_table_find_(&data->table, od->nbs); > > + ovs_assert(ls_stateful_rec); > > + ls_stateful_rec->has_lb_vip = ls_has_lb_vip(od); > > + > > + /* Add the ls_stateful_rec to the tracking data. */ > > + hmapx_add(&data->trk_data.crupdated, ls_stateful_rec); > > } > > > > - HMAPX_FOR_EACH (hmapx_node, &changed_stateful_od) { > > + HMAPX_FOR_EACH (hmapx_node, &nd_changes->ls_with_changed_acls) { > > const struct ovn_datapath *od = hmapx_node->data; > > > > - struct ls_stateful_record *ls_stateful_rec = > ls_stateful_table_find_( > > - &data->table, od->nbs); > > + struct ls_stateful_record *ls_stateful_rec = > > + ls_stateful_table_find_(&data->table, od->nbs); > > ovs_assert(ls_stateful_rec); > > - ls_stateful_record_reinit(ls_stateful_rec, od, NULL, > > - input_data.ls_port_groups); > > + ls_stateful_record_set_acls(ls_stateful_rec, od, > > + input_data.ls_port_groups); > > > > /* Add the ls_stateful_rec to the tracking data. */ > > hmapx_add(&data->trk_data.crupdated, ls_stateful_rec); > > } > > > > - hmapx_destroy(&changed_stateful_od); > > - > > if (ls_stateful_has_tracked_data(&data->trk_data)) { > > return EN_HANDLED_UPDATED; > > } > > @@ -189,49 +183,47 @@ ls_stateful_port_group_handler(struct engine_node > *node, void *data_) > > return EN_UNHANDLED; > > } > > > > - /* port_group engine node doesn't provide the tracking data yet. > > - * Loop through all the ls port groups and update the > ls_stateful_rec. > > - * This is still better than returning false. */ > > - struct ls_stateful_input input_data = > ls_stateful_get_input_data(node); > > struct ed_type_ls_stateful *data = data_; > > - const struct ls_port_group *ls_pg; > > + if (ls_stateful_has_tracked_data(&data->trk_data)) { > > + return EN_HANDLED_UPDATED; > > + } > > + return EN_HANDLED_UNCHANGED; > > +} > > > > - LS_PORT_GROUP_TABLE_FOR_EACH (ls_pg, input_data.ls_port_groups) { > > - struct ls_stateful_record *ls_stateful_rec = > > - ls_stateful_table_find_(&data->table, ls_pg->nbs); > > - ovs_assert(ls_stateful_rec); > > - const struct ovn_datapath *od = > > - ovn_datapaths_find_by_index(input_data.ls_datapaths, > > - ls_stateful_rec->ls_index); > > - bool had_stateful_acl = ls_stateful_rec->has_stateful_acl; > > - struct acl_tier old_max = ls_stateful_rec->max_acl_tier; > > - bool had_acls = ls_stateful_rec->has_acls; > > - bool modified = false; > > - > > - ls_stateful_record_reinit(ls_stateful_rec, od, ls_pg, > > - input_data.ls_port_groups); > > - > > - struct acl_tier new_max = ls_stateful_rec->max_acl_tier; > > - > > - /* Using memcmp for struct acl_tier is fine since there is no > padding > > - * in the struct. However, if the structure is changed, the > memcmp > > - * may need to be updated to compare individual struct fields. > > - */ > > - if ((had_stateful_acl != ls_stateful_rec->has_stateful_acl) > > - || (had_acls != ls_stateful_rec->has_acls) > > - || memcmp(&old_max, &new_max, sizeof(old_max))) { > > - modified = true; > > +enum engine_input_handler_result > > +ls_stateful_acl_handler(struct engine_node *node, void *data_) > > +{ > > + struct ed_type_ls_stateful *data = data_; > > + const struct nbrec_acl_table *nbrec_acl_table = > > + EN_OVSDB_GET(engine_get_input("NB_acl", node)); > > + > > + const struct nbrec_acl *acl; > > + NBREC_ACL_TABLE_FOR_EACH_TRACKED (acl, nbrec_acl_table) { > > + /* The creation and deletion is handled in relation to LS/PG > rather > > + * than the ACL itself. */ > > + if (nbrec_acl_is_new(acl) || nbrec_acl_is_deleted(acl)) { > > + continue; > > } > > > > - if (modified) { > > - /* Add the ls_stateful_rec to the tracking data. */ > > - hmapx_add(&data->trk_data.crupdated, ls_stateful_rec); > > + /* Meter changes are handled separately in the en_sync_meters > node. */ > > + if (nbrec_acl_is_updated(acl, NBREC_ACL_COL_LOG) || > > + nbrec_acl_is_updated(acl, NBREC_ACL_COL_METER)) { > > + continue; > > + } > > I think the if statement above should be removed or modified. > > Currently, en_lflow evaluates en_sync_meters before en_ls_stateful, so > if an ACL's meters are updated, then en_sync_meters will recompute, > causing en_lflow to recompute, too. This code will never be reached. > Therefore, there's not much point in checking this. > > However, consider a future where en_sync_meters is able to incrementally > handle changes to ACL meters. Therefore, this code does get reached when > the ACL meter is updated. If an ACL's meter and match are updated in the > same transaction, then this if statement will cause the loop to > continue, and the ACL will not be added to the trk_data->crupdated > hmapx. The ACL's lflows will not get resynced. > > Based on the current code, it makes most sense to remove the if > statement entirely. If we want to be prepared for that possible future > where en_meters_sync does not force a recompute of en_lflow, then we > will need to discern which columns have been updated. In order to get > the logic correct, though, we would need to abandon processing the ACL > if the log and meter columns are the only ones updated, not just if they > are updated at all. > That's a fair point, I have removed that check in v4. > > > + > > + struct ls_stateful_record *ls_stateful_rec; > > + LS_STATEFUL_TABLE_FOR_EACH (ls_stateful_rec, &data->table) { > > + if (uuidset_contains(&ls_stateful_rec->related_acls, > > + &acl->header_.uuid)) { > > + hmapx_add(&data->trk_data.crupdated, ls_stateful_rec); > > + } > > } > > } > > > > if (ls_stateful_has_tracked_data(&data->trk_data)) { > > return EN_HANDLED_UPDATED; > > } > > + > > return EN_HANDLED_UNCHANGED; > > } > > > > @@ -295,7 +287,8 @@ ls_stateful_record_create(struct ls_stateful_table > *table, > > xzalloc(sizeof *ls_stateful_rec); > > ls_stateful_rec->ls_index = od->index; > > ls_stateful_rec->nbs_uuid = od->nbs->header_.uuid; > > - ls_stateful_record_init(ls_stateful_rec, od, NULL, ls_pgs); > > + uuidset_init(&ls_stateful_rec->related_acls); > > + ls_stateful_record_init(ls_stateful_rec, od, ls_pgs); > > ls_stateful_rec->lflow_ref = lflow_ref_create(); > > > > hmap_insert(&table->entries, &ls_stateful_rec->key_node, > > @@ -307,6 +300,7 @@ ls_stateful_record_create(struct ls_stateful_table > *table, > > static void > > ls_stateful_record_destroy(struct ls_stateful_record *ls_stateful_rec) > > { > > + uuidset_destroy(&ls_stateful_rec->related_acls); > > lflow_ref_destroy(ls_stateful_rec->lflow_ref); > > free(ls_stateful_rec); > > } > > @@ -314,20 +308,10 @@ ls_stateful_record_destroy(struct > ls_stateful_record *ls_stateful_rec) > > static void > > ls_stateful_record_init(struct ls_stateful_record *ls_stateful_rec, > > const struct ovn_datapath *od, > > - const struct ls_port_group *ls_pg, > > const struct ls_port_group_table *ls_pgs) > > { > > ls_stateful_rec->has_lb_vip = ls_has_lb_vip(od); > > - ls_stateful_record_set_acl_flags(ls_stateful_rec, od, ls_pg, > ls_pgs); > > -} > > - > > -static void > > -ls_stateful_record_reinit(struct ls_stateful_record *ls_stateful_rec, > > - const struct ovn_datapath *od, > > - const struct ls_port_group *ls_pg, > > - const struct ls_port_group_table *ls_pgs) > > -{ > > - ls_stateful_record_init(ls_stateful_rec, od, ls_pg, ls_pgs); > > + ls_stateful_record_set_acls(ls_stateful_rec, od, ls_pgs); > > } > > > > static bool > > @@ -365,36 +349,28 @@ ls_has_lb_vip(const struct ovn_datapath *od) > > } > > > > static void > > -ls_stateful_record_set_acl_flags(struct ls_stateful_record > *ls_stateful_rec, > > - const struct ovn_datapath *od, > > - const struct ls_port_group *ls_pg, > > - const struct ls_port_group_table > *ls_pgs) > > +ls_stateful_record_set_acls(struct ls_stateful_record *ls_stateful_rec, > > + const struct ovn_datapath *od, > > + const struct ls_port_group_table *ls_pgs) > > { > > ls_stateful_rec->has_stateful_acl = false; > > memset(&ls_stateful_rec->max_acl_tier, 0, > > sizeof ls_stateful_rec->max_acl_tier); > > ls_stateful_rec->has_acls = false; > > + uuidset_clear(&ls_stateful_rec->related_acls); > > > > - if (ls_stateful_record_set_acl_flags_(ls_stateful_rec, > od->nbs->acls, > > - od->nbs->n_acls)) { > > - return; > > - } > > - > > - if (!ls_pg) { > > - ls_pg = ls_port_group_table_find(ls_pgs, od->nbs); > > - } > > + ls_stateful_record_set_acls_(ls_stateful_rec, od->nbs->acls, > > + od->nbs->n_acls); > > > > + struct ls_port_group *ls_pg = ls_port_group_table_find(ls_pgs, > od->nbs); > > if (!ls_pg) { > > return; > > } > > > > const struct ls_port_group_record *ls_pg_rec; > > HMAP_FOR_EACH (ls_pg_rec, key_node, &ls_pg->nb_pgs) { > > - if (ls_stateful_record_set_acl_flags_(ls_stateful_rec, > > - ls_pg_rec->nb_pg->acls, > > - > ls_pg_rec->nb_pg->n_acls)) { > > - return; > > - } > > + ls_stateful_record_set_acls_(ls_stateful_rec, > ls_pg_rec->nb_pg->acls, > > + ls_pg_rec->nb_pg->n_acls); > > } > > } > > > > @@ -421,46 +397,24 @@ update_ls_max_acl_tier(struct ls_stateful_record > *ls_stateful_rec, > > *tier = MAX(*tier, acl->tier); > > } > > > > -static bool > > -ls_acl_tiers_are_maxed_out(struct acl_tier *acl_tier, > > - uint64_t max_allowed_acl_tier) > > -{ > > - return acl_tier->ingress_post_lb == max_allowed_acl_tier && > > - acl_tier->ingress_pre_lb == max_allowed_acl_tier && > > - acl_tier->egress == max_allowed_acl_tier; > > -} > > - > > -static bool > > -ls_stateful_record_set_acl_flags_(struct ls_stateful_record > *ls_stateful_rec, > > - struct nbrec_acl **acls, > > - size_t n_acls) > > +static void > > +ls_stateful_record_set_acls_(struct ls_stateful_record *ls_stateful_rec, > > + struct nbrec_acl **acls, size_t n_acls) > > { > > - /* A true return indicates that there are no possible ACL flags > > - * left to set on ls_stateful record. A false return indicates that > > - * further ACLs should be explored in case more flags need to be > > - * set on ls_stateful record. > > - */ > > if (!n_acls) { > > - return false; > > + return; > > } > > > > ls_stateful_rec->has_acls = true; > > for (size_t i = 0; i < n_acls; i++) { > > const struct nbrec_acl *acl = acls[i]; > > update_ls_max_acl_tier(ls_stateful_rec, acl); > > + uuidset_insert(&ls_stateful_rec->related_acls, > &acl->header_.uuid); > > if (!ls_stateful_rec->has_stateful_acl > > && !strcmp(acl->action, "allow-related")) { > > ls_stateful_rec->has_stateful_acl = true; > > } > > - if (ls_stateful_rec->has_stateful_acl && > > - ls_acl_tiers_are_maxed_out( > > - &ls_stateful_rec->max_acl_tier, > > - nbrec_acl_col_tier.type.key.integer.max)) { > > - return true; > > - } > > } > > - > > - return false; > > } > > > > static struct ls_stateful_input > > diff --git a/northd/en-ls-stateful.h b/northd/en-ls-stateful.h > > index 1d98b3695..217adf696 100644 > > --- a/northd/en-ls-stateful.h > > +++ b/northd/en-ls-stateful.h > > @@ -20,6 +20,7 @@ > > > > /* OVS includes. */ > > #include "lib/hmapx.h" > > +#include "lib/uuidset.h" > > #include "openvswitch/hmap.h" > > #include "sset.h" > > > > @@ -54,6 +55,9 @@ struct ls_stateful_record { > > bool has_acls; > > struct acl_tier max_acl_tier; > > > > + /* Set of ACLs that are related to this LS. */ > > + struct uuidset related_acls; > > + > > /* 'lflow_ref' is used to reference logical flows generated for > > * this ls_stateful record. > > * > > @@ -109,6 +113,8 @@ enum engine_input_handler_result > > ls_stateful_northd_handler(struct engine_node *, void *data); > > enum engine_input_handler_result > > ls_stateful_port_group_handler(struct engine_node *, void *data); > > +enum engine_input_handler_result > > +ls_stateful_acl_handler(struct engine_node *node, void *data); > > > > const struct ls_stateful_record *ls_stateful_table_find( > > const struct ls_stateful_table *, const struct > nbrec_logical_switch *); > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > > index be2f36e33..d43bfc16c 100644 > > --- a/northd/inc-proc-northd.c > > +++ b/northd/inc-proc-northd.c > > @@ -259,6 +259,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > engine_add_input(&en_ls_stateful, &en_northd, > ls_stateful_northd_handler); > > engine_add_input(&en_ls_stateful, &en_port_group, > > ls_stateful_port_group_handler); > > + engine_add_input(&en_ls_stateful, &en_nb_acl, > ls_stateful_acl_handler); > > > > engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL); > > engine_add_input(&en_mac_binding_aging, &en_northd, NULL); > > @@ -330,7 +331,6 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > engine_add_input(&en_multicast_igmp, &en_sb_multicast_group, NULL); > > engine_add_input(&en_multicast_igmp, &en_sb_igmp_group, NULL); > > > > - engine_add_input(&en_lflow, &en_nb_acl, 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); > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 945727a3f..95119016f 100644 > > --- a/tests/ovn-northd.at > > +++ b/tests/ovn-northd.at > > @@ -11352,14 +11352,14 @@ check as northd ovn-appctl -t ovn-northd > inc-engine/clear-stats > > check ovn-nbctl --wait=sb meter-add m drop 1 pktps > > check ovn-nbctl --wait=sb acl-add ls from-lport 1 1 allow > > dnl Only triggers recompute of the sync_meters and lflow nodes. > > -check_recompute_counter 0 2 1 > > +check_recompute_counter 0 1 1 > > CHECK_NO_CHANGE_AFTER_RECOMPUTE(1) > > > > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > check ovn-nbctl --wait=sb meter-del m > > check ovn-nbctl --wait=sb acl-del ls > > dnl Only triggers recompute of the sync_meters and lflow nodes. > > -check_recompute_counter 0 2 1 > > +check_recompute_counter 0 1 1 > > CHECK_NO_CHANGE_AFTER_RECOMPUTE(1) > > > > AT_CLEANUP > > @@ -17542,7 +17542,7 @@ check as northd ovn-appctl -t ovn-northd > inc-engine/clear-stats > > > > check ovn-nbctl --wait=sb set acl $acl_id match=udp > > check_engine_stats northd norecompute compute > > -check_engine_stats lflow recompute nocompute > > +check_engine_stats lflow norecompute compute > > check_engine_stats sync_meters norecompute compute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > @@ -17573,7 +17573,7 @@ check as northd ovn-appctl -t ovn-northd > inc-engine/clear-stats > > > > check ovn-nbctl --wait=sb set acl $acl_id match=udp > > check_engine_stats northd norecompute compute > > -check_engine_stats lflow recompute nocompute > > +check_engine_stats lflow norecompute compute > > check_engine_stats sync_meters norecompute compute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > @@ -17596,28 +17596,28 @@ AS_BOX([ACLs attached to LS]) > > check ovn-nbctl --wait=sb acl-add ls from-lport 100 tcp drop > > acl_id=$(fetch_column nb:Acl _uuid match=tcp action=drop) > > check_engine_stats northd norecompute compute > > -check_engine_stats lflow recompute nocompute > > +check_engine_stats lflow norecompute compute > > check_engine_stats sync_meters norecompute compute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > > > check ovn-nbctl --wait=sb acl-add ls from-lport 101 ip4 drop > > check_engine_stats northd norecompute compute > > -check_engine_stats lflow recompute nocompute > > +check_engine_stats lflow norecompute compute > > check_engine_stats sync_meters norecompute compute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > > > check ovn-nbctl --wait=sb set acl $acl_id match=udp > > check_engine_stats northd norecompute compute > > -check_engine_stats lflow recompute nocompute > > +check_engine_stats lflow norecompute compute > > check_engine_stats sync_meters norecompute compute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > > > check ovn-nbctl --wait=sb acl-del ls > > check_engine_stats northd norecompute compute > > -check_engine_stats lflow recompute nocompute > > +check_engine_stats lflow norecompute compute > > check_engine_stats sync_meters norecompute compute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > @@ -17626,28 +17626,28 @@ AS_BOX([ACLs attached to PG]) > > check ovn-nbctl --wait=sb acl-add pg from-lport 100 tcp drop > > acl_id=$(fetch_column nb:Acl _uuid match=tcp action=drop) > > check_engine_stats northd norecompute compute > > -check_engine_stats lflow recompute nocompute > > +check_engine_stats lflow norecompute compute > > check_engine_stats sync_meters norecompute compute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > > > check ovn-nbctl --wait=sb acl-add pg from-lport 101 ip4 drop > > check_engine_stats northd norecompute compute > > -check_engine_stats lflow recompute nocompute > > +check_engine_stats lflow norecompute compute > > check_engine_stats sync_meters norecompute compute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > > > check ovn-nbctl --wait=sb set acl $acl_id match=udp > > check_engine_stats northd norecompute compute > > -check_engine_stats lflow recompute nocompute > > +check_engine_stats lflow norecompute compute > > check_engine_stats sync_meters norecompute compute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > > > check ovn-nbctl --wait=sb acl-del pg > > check_engine_stats northd norecompute compute > > -check_engine_stats lflow recompute nocompute > > +check_engine_stats lflow norecompute compute > > check_engine_stats sync_meters norecompute compute > > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
