On 7/31/24 15:25, Ales Musil wrote: > > > On Wed, Jul 31, 2024 at 3:03 PM Ilya Maximets <[email protected] > <mailto:[email protected]>> wrote: > > On 7/31/24 11:05, Dumitru Ceara wrote: > > From: Ales Musil <[email protected] <mailto:[email protected]>> > > > > Currently, OVN would generate up to 2 flows per sample, depending > > on the configuration. Add optimization that can reduce the number > > of flows added into the ACL pipeline down to 3 per collector. This > > optimization can be achieved only when the sample action with > > registers is supported in OvS and the sample has only single > > collector. The single collector per sample should be the case > > in most configurations, usually even the same collector > > for all samples which greatly reduces the number of flows per > > ACL with sampling. > > > > If there are more collectors per sample or the OvS feature is not > > supported, the implementation will fall back to flows per sample. > > > > Reported-at: https://issues.redhat.com/browse/FDP-709 > <https://issues.redhat.com/browse/FDP-709> > > Signed-off-by: Ales Musil <[email protected] <mailto:[email protected]>> > > --- > > include/ovn/logical-fields.h | 2 + > > lib/logical-fields.c | 8 + > > northd/northd.c | 256 +++++++++++++++++++------- > > tests/ovn-northd.at <http://ovn-northd.at> | 341 > ++++++++++++++++++++++++++++++----- > > tests/ovn.at <http://ovn.at> | 2 + > > tests/system-ovn.at <http://system-ovn.at> | 10 +- > > 6 files changed, 509 insertions(+), 110 deletions(-) > > Not a full review, but a couple comments inline. > > > > > diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h > > index ce79b501cf..d6c4a9b6b3 100644 > > --- a/include/ovn/logical-fields.h > > +++ b/include/ovn/logical-fields.h > > @@ -197,6 +197,8 @@ const struct ovn_field *ovn_field_from_name(const > char *name); > > #define OVN_CT_NATTED_BIT 1 > > #define OVN_CT_LB_SKIP_SNAT_BIT 2 > > #define OVN_CT_LB_FORCE_SNAT_BIT 3 > > +#define OVN_CT_OBS_STAGE_1ST_BIT 4 > > +#define OVN_CT_OBS_STAGE_END_BIT 5 > > > > #define OVN_CT_BLOCKED 1 > > #define OVN_CT_NATTED 2 > > diff --git a/lib/logical-fields.c b/lib/logical-fields.c > > index 0c187e1c84..134d2674fd 100644 > > --- a/lib/logical-fields.c > > +++ b/lib/logical-fields.c > > @@ -165,6 +165,14 @@ ovn_init_symtab(struct shash *symtab) > > > OVN_CT_STR(OVN_CT_LB_FORCE_SNAT_BIT) > > "]", > > WR_CT_COMMIT); > > + expr_symtab_add_subfield_scoped(symtab, "ct_mark.obs_stage", NULL, > > + "ct_mark[" > > + > OVN_CT_STR(OVN_CT_OBS_STAGE_1ST_BIT) ".." > > + > OVN_CT_STR(OVN_CT_OBS_STAGE_END_BIT) > > + "]", > > + WR_CT_COMMIT); > > + expr_symtab_add_subfield_scoped(symtab, > "ct_mark.obs_collector_id", NULL, > > + "ct_mark[16..23]", WR_CT_COMMIT); > > > > expr_symtab_add_field_scoped(symtab, "ct_label", MFF_CT_LABEL, > NULL, > > false, WR_CT_COMMIT); > > diff --git a/northd/northd.c b/northd/northd.c > > index 6bf3ca4d8c..fd9cfe82db 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -144,8 +144,20 @@ static bool vxlan_mode; > > #define REGBIT_ACL_VERDICT_ALLOW "reg8[16]" > > #define REGBIT_ACL_VERDICT_DROP "reg8[17]" > > #define REGBIT_ACL_VERDICT_REJECT "reg8[18]" > > +#define REGBIT_ACL_OBS_STAGE "reg8[19..20]" > > #define REG_ACL_TIER "reg8[30..31]" > > > > +enum acl_observation_stage { > > + ACL_OBS_FROM_LPORT, > > + ACL_OBS_FROM_LPORT_AFTER_LB, > > + ACL_OBS_TO_LPORT, > > + ACL_OBS_STAGE_MAX > > +}; > > Values of this enum end up in the database as part of actions. > It's better to define exact values for them. > > > > Fair point. > > > > <snip> > > > diff --git a/tests/system-ovn.at <http://system-ovn.at> > b/tests/system-ovn.at <http://system-ovn.at> > > index 52b2b84d7b..a9cb54b452 100644 > > --- a/tests/system-ovn.at <http://system-ovn.at> > > +++ b/tests/system-ovn.at <http://system-ovn.at> > > @@ -7724,7 +7724,7 @@ NS_CHECK_EXEC([sw0-p3], [ping -q -c 10 -i 0.3 -w > 15 10.0.0.2 | FORMAT_PING], \ > > AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.0.0.2) | \ > > sed -e 's/zone=[[0-9]]*/zone=<cleared>/' | \ > > sed -e > 's/labels=0x4d3[[0-9a-f]]*/labels=0x4d3000000000000000000000000/'], [0], [dnl > > What's the point of replacing with 24 zeroes? > Maybe just 0x4d3 ? > > Would be nice to check that we matched 24 characters, but > I'm not sure if there is a nice portable way to do that. > > > Well this sed is not part of this patch actually we can change but rather as > follow up change.
Ah, true. Missed that it's not introduced here. > > > > Best regards, Ilya Maximets. > > > Thanks, > Ales > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > [email protected] <mailto:[email protected]> > > <https://red.ht/sig> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
