On Wed, Jun 11, 2025 at 12:10 AM Mark Michelson <[email protected]> wrote:
> Hi Ales, > Hi Mark, thank you for the review. > > On 5/21/25 08:59, Ales Musil via dev wrote: > > The lflow has dependency on sync_meters node so any change in > > sync_meters will cause full lflow recompute. Add handler for ACLs > > into sync_meters node that will check if the ACL actually requires > > any meters. In case it does it will trigger recompute. However, in > > most cases the ACLs are without metering which should avoid expensive > > recomopute of lflow node. > > I would reword this commit message slightly. The commit message is > implying that this change prevents en-lflow from recomputing when a > non-metered ACL is updated. In reality, this change is only affecting > whether en-sync-meters needs to be recomputed. > > I know that patch 3 removes en-lflow's dependency on en-nb-acl, and so > this patch helps contribute towards incremental processing in en-lflow. > I think the commit message needs to make it clear that this change alone > does not allow for en-lflow to run incrementally. > I have changed the commit message to make it clear. > > As far as the code itself is concerned, > > Acked-by: Mark Michelson <[email protected]> > > > > > Signed-off-by: Ales Musil <[email protected]> > > --- > > v3: Rebase on top of latest main. > > v2: Rebase on top of latest main. > > --- > > northd/en-meters.c | 24 ++++++++++++ > > northd/en-meters.h | 2 + > > northd/inc-proc-northd.c | 2 +- > > tests/ovn-northd.at | 79 +++++++++++++++++++++++++++++++++++++++- > > 4 files changed, 104 insertions(+), 3 deletions(-) > > > > diff --git a/northd/en-meters.c b/northd/en-meters.c > > index a0352c34b..288134108 100644 > > --- a/northd/en-meters.c > > +++ b/northd/en-meters.c > > @@ -79,6 +79,30 @@ en_sync_meters_run(struct engine_node *node, void > *data_) > > return EN_UPDATED; > > } > > > > +enum engine_input_handler_result > > +sync_meters_nb_acl_handler(struct engine_node *node, void *data > OVS_UNUSED) > > +{ > > + const struct nbrec_acl_table *acl_table = > > + EN_OVSDB_GET(engine_get_input("NB_acl", node)); > > + > > + const struct nbrec_acl *nb_acl; > > + NBREC_ACL_TABLE_FOR_EACH_TRACKED (nb_acl, acl_table) { > > + /* New or deleted ACL with meter needs to be recomputed. */ > > + if ((nbrec_acl_is_new(nb_acl) || nbrec_acl_is_deleted(nb_acl)) > && > > + (nb_acl->log || nb_acl->meter)) { > > + return EN_UNHANDLED; > > + } > > + > > + /* Addition or removal of meter requires recompute. */ > > + if (nbrec_acl_is_updated(nb_acl, NBREC_ACL_COL_LOG) || > > + nbrec_acl_is_updated(nb_acl, NBREC_ACL_COL_METER)) { > > + return EN_UNHANDLED; > > + } > > + } > > + > > + return EN_HANDLED_UNCHANGED; > > +} > > + > > const struct nbrec_meter* > > fair_meter_lookup_by_name(const struct shash *meter_groups, > > const char *meter_name) > > diff --git a/northd/en-meters.h b/northd/en-meters.h > > index e0ef07fad..e1deb5daf 100644 > > --- a/northd/en-meters.h > > +++ b/northd/en-meters.h > > @@ -29,6 +29,8 @@ struct sync_meters_data { > > void *en_sync_meters_init(struct engine_node *, struct engine_arg *); > > void en_sync_meters_cleanup(void *data); > > enum engine_node_state en_sync_meters_run(struct engine_node *, void > *data); > > +enum engine_input_handler_result > > +sync_meters_nb_acl_handler(struct engine_node *, void *data); > > > > const struct nbrec_meter *fair_meter_lookup_by_name( > > const struct shash *meter_groups, > > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > > index b1e4994a4..5905462ec 100644 > > --- a/northd/inc-proc-northd.c > > +++ b/northd/inc-proc-northd.c > > @@ -319,7 +319,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > > engine_add_input(&en_group_ecmp_route, &en_learned_route_sync, > > group_ecmp_route_learned_route_change_handler); > > > > - engine_add_input(&en_sync_meters, &en_nb_acl, NULL); > > + engine_add_input(&en_sync_meters, &en_nb_acl, > sync_meters_nb_acl_handler); > > engine_add_input(&en_sync_meters, &en_nb_meter, NULL); > > engine_add_input(&en_sync_meters, &en_sb_meter, NULL); > > > > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > > index 4a676e5e4..28b568663 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 2 > > +check_recompute_counter 0 2 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 2 > > +check_recompute_counter 0 2 1 > > CHECK_NO_CHANGE_AFTER_RECOMPUTE(1) > > > > AT_CLEANUP > > @@ -17516,3 +17516,78 @@ AT_CHECK([grep -E 'ls_in_pre_stateful' sw1flows > | ovn_strip_lflows], [0], [dnl > > AT_CLEANUP > > ]) > > > > +OVN_FOR_EACH_NORTHD_NO_HV([ > > +AT_SETUP([ACL incremental processing]) > > +ovn_start > > + > > +check ovn-nbctl ls-add ls > > +check ovn-nbctl meter-add meter1 drop 10 kbps > > +check ovn-nbctl meter-add meter2 drop 20 kbps > > + > > +# Wait for sb to be connected before clearing stats. > > +check ovn-nbctl --wait=sb sync > > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > + > > +AS_BOX([ACL with log]) > > +check ovn-nbctl --wait=sb --log acl-add ls from-lport 100 tcp drop > > +acl_id=$(fetch_column nb:Acl _uuid action=drop) > > +check_engine_stats northd norecompute compute > > +check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_meters recompute nocompute > > +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 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 log=false > > +check_engine_stats northd norecompute compute > > +check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_meters recompute nocompute > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > +check ovn-nbctl --wait=sb set acl $acl_id log=true > > +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 sync_meters recompute nocompute > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > + > > +AS_BOX([ACL with meter]) > > +check ovn-nbctl --wait=sb --meter=meter1 acl-add ls from-lport 100 tcp > drop > > +acl_id=$(fetch_column nb:Acl _uuid action=drop) > > +check_engine_stats northd norecompute compute > > +check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_meters recompute nocompute > > +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 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 meter=meter2 > > +check_engine_stats northd norecompute compute > > +check_engine_stats lflow recompute nocompute > > +check_engine_stats sync_meters recompute nocompute > > +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 sync_meters recompute nocompute > > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > > + > > +AT_CLEANUP > > +]) > > Thanks, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
