On 8/6/24 20:47, Mark Michelson wrote:
> On 8/6/24 05:44, Dumitru Ceara wrote:
>> From: Ales Musil <[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
>> Signed-off-by: Ales Musil <[email protected]>
>> ---
>> V6:
>> - Rebased.
>> - Removed Dumitru's ack.
>> - Store (newly created) Sample_Collector.id in ct state - instead of the
>> actual set-id to avoid ambiguity when multiple probabilities are used
>> with the same collector set id.
>> - Fixed bug with stateful to-lport ACLs on router ports.
>> - Reduced number of ct mark bits used for storing the collector id to 4.
>> V5:
>> - Address Ilya's comments:
>> - Explicitly set acl_observation_stage enum values.
>> - Added Dumitru's ack
>> ---
>> include/ovn/logical-fields.h | 2 +
>> lib/logical-fields.c | 8 +
>> northd/northd.c | 273 ++++++++++++++++++------
>> tests/ovn-northd.at | 388 +++++++++++++++++++++++++++++------
>> tests/ovn.at | 2 +
>> tests/system-ovn.at | 10 +-
>> 6 files changed, 553 insertions(+), 130 deletions(-)
>>
>> 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..00bbc4a1c4 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..19]", 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 13f9faba31..b1201bcc86 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 = 0,
>> + ACL_OBS_FROM_LPORT_AFTER_LB = 1,
>> + ACL_OBS_TO_LPORT = 2,
>> + ACL_OBS_STAGE_MAX
>> +};
>> +
>> +/* enum acl_observation_stage_t values must fit in the 2 bits of
>> + * REGBIT_ACL_OBS_STAGE .*/
>> +BUILD_ASSERT_DECL(ACL_OBS_STAGE_MAX < (1 << 2));
>> +
>> /* Indicate that this packet has been recirculated using egress
>> * loopback. This allows certain checks to be bypassed, such as a
>> * logical router dropping packets with source IP address equals
>> @@ -189,6 +201,8 @@ static bool vxlan_mode;
>> * domain and point ID. */
>> #define REG_OBS_POINT_ID_NEW "reg3"
>> #define REG_OBS_POINT_ID_EST "reg9"
>> +#define REG_OBS_COLLECTOR_ID_NEW "reg8[0..3]"
>> +#define REG_OBS_COLLECTOR_ID_EST "reg8[4..7]"
>> /* Register used for temporarily store ECMP eth.src to avoid
>> masked ct_label
>> * access. It doesn't really occupy registers because the content of
>> the
>> @@ -228,12 +242,13 @@ static bool vxlan_mode;
>> * +----+----------------------------------------------+ G
>> | |
>> * | R7 | UNUSED | 1
>> | |
>> *
>> +----+----------------------------------------------+---+-----------------------------------+
>> - * | | LB_AFF_MATCH_PORT |
>> - * | | (>= IN_LB_AFF_CHECK && <= IN_LB_AFF_LEARN) |
>> - * +----+----------------------------------------------+
>> - * | R9 | OBS_POINT_ID_EST |
>> - * | | (>= ACL_EVAL* && <= ACL_ACTION*) |
>> - * +----+----------------------------------------------+
>> + * | R8 | LB_AFF_MATCH_PORT | X |
>> REG_OBS_COLLECTOR_ID_NEW |
>> + * | | (>= IN_LB_AFF_CHECK && <= IN_LB_AFF_LEARN) | R |
>> REG_OBS_COLLECTOR_ID_EST |
>> + * | | | E | (>=
>> ACL_EVAL* && <= ACL_ACTION*) |
>> + * +----+----------------------------------------------+ G
>> +-----------------------------------+
>> + * | R9 | OBS_POINT_ID_EST | 4
>> | |
>> + * | | (>= ACL_EVAL* && <= ACL_ACTION*) |
>> | |
>> + *
>> +----+----------------------------------------------+---+-----------------------------------+
>> *
>> * Logical Router pipeline:
>> *
>> +-----+---------------------------+---+-----------------+---+------------------------------------+
>> @@ -6532,7 +6547,8 @@ build_acl_sample_action(struct ds *actions,
>> const struct nbrec_acl *acl,
>> static void
>> build_acl_sample_label_action(struct ds *actions, const struct
>> nbrec_acl *acl,
>> const struct nbrec_sample *sample_new,
>> - const struct nbrec_sample *sample_est)
>> + const struct nbrec_sample *sample_est,
>> + enum acl_observation_stage obs_stage)
>> {
>> if (!acl->label && !sample_new && !sample_est) {
>> return;
>> @@ -6540,6 +6556,8 @@ build_acl_sample_label_action(struct ds
>> *actions, const struct nbrec_acl *acl,
>> uint32_t point_id_new = 0;
>> uint32_t point_id_est = 0;
>> + uint8_t collector_id_new = 0;
>> + uint8_t collector_id_est = 0;
>> if (acl->label) {
>> point_id_new = acl->label;
>> @@ -6547,16 +6565,27 @@ build_acl_sample_label_action(struct ds
>> *actions, const struct nbrec_acl *acl,
>> } else {
>> if (sample_new) {
>> point_id_new = sample_new->metadata;
>> + if (sample_new->n_collectors == 1) {
>> + collector_id_new = sample_new->collectors[0]->id;
>> + }
>> }
>> if (sample_est) {
>> point_id_est = sample_est->metadata;
>> + if (sample_est->n_collectors == 1) {
>> + collector_id_est = sample_est->collectors[0]->id;
>> + }
>> }
>> }
>> ds_put_format(actions, REGBIT_ACL_LABEL" = 1; "
>> REG_OBS_POINT_ID_NEW " = %"PRIu32"; "
>> - REG_OBS_POINT_ID_EST " = %"PRIu32"; ",
>> - point_id_new, point_id_est);
>> + REG_OBS_POINT_ID_EST " = %"PRIu32"; "
>> + REG_OBS_COLLECTOR_ID_NEW " = %"PRIu8"; "
>> + REG_OBS_COLLECTOR_ID_EST " = %"PRIu8"; "
>> + REGBIT_ACL_OBS_STAGE " = %"PRIu8"; ",
>> + point_id_new, point_id_est,
>> + collector_id_new, collector_id_est,
>> + (uint8_t) obs_stage);
>> }
>> /* This builds an ACL logical flow specific match that selects
>> traffic
>> @@ -6604,46 +6633,16 @@ build_acl_sample_label_match(struct ds *match,
>> const struct nbrec_acl *acl,
>> }
>> /* This builds a logical flow that samples and forwards/drops traffic
>> - * that hit a stateless ACL ("pass" or "allow-stateless") that has
>> sampling
>> - * enabled.
>> + * that hit a stateless/stateful ACL that has sampling enabled.
>> */
>> static void
>> -build_acl_sample_new_stateless_flows(const struct ovn_datapath *od,
>> - struct lflow_table *lflows,
>> - enum ovn_stage stage,
>> - struct ds *match, struct ds
>> *actions,
>> - const struct nbrec_acl *acl,
>> - uint8_t sample_domain_id,
>> - struct lflow_ref *lflow_ref)
>> -{
>> - if (!acl->sample_new) {
>> - return;
>> - }
>> -
>> - ds_clear(actions);
>> - ds_clear(match);
>> -
>> - ds_put_cstr(match, "ip && ");
>> - build_acl_sample_register_match(match, acl, acl->sample_new);
>> -
>> - build_acl_sample_action(actions, acl, acl->sample_new,
>> sample_domain_id);
>> -
>> - ovn_lflow_add(lflows, od, stage, 1100, ds_cstr(match),
>> - ds_cstr(actions), lflow_ref);
>> -}
>> -
>> -/* This builds a logical flow that samples and forwards/drops traffic
>> - * that created a new conntrack entry and hit a stateful ACL that has
>> sampling
>> - * enabled.
>> - */
>> -static void
>> -build_acl_sample_new_stateful_flows(const struct ovn_datapath *od,
>> - struct lflow_table *lflows,
>> - enum ovn_stage stage,
>> - struct ds *match, struct ds
>> *actions,
>> - const struct nbrec_acl *acl,
>> - uint8_t sample_domain_id,
>> - struct lflow_ref *lflow_ref)
>> +build_acl_sample_new_flows(const struct ovn_datapath *od,
>> + struct lflow_table *lflows,
>> + enum ovn_stage stage,
>> + struct ds *match, struct ds *actions,
>> + const struct nbrec_acl *acl,
>> + uint8_t sample_domain_id, bool stateful,
>> + struct lflow_ref *lflow_ref)
>> {
>> if (!acl->sample_new) {
>> return;
>> @@ -6652,12 +6651,16 @@ build_acl_sample_new_stateful_flows(const
>> struct ovn_datapath *od,
>> ds_clear(actions);
>> ds_clear(match);
>> - /* Match on new connections. However, for to-lport ACLs, due to
>> + /* Match on new connections. However, for stateful to-lport
>> ACLs, due to
>> * skip_port_from_conntrack() conntrack state might be cleared, so
>> * take that into account too. */
>> - ds_put_format(match, "ip && %s && ",
>> - stage != S_SWITCH_OUT_ACL_SAMPLE
>> - ? "ct.new" : "(ct.new || !ct.trk)");
>> + if (!stateful) {
>> + ds_put_format(match, "ip && ");
>> + } else if (stage != S_SWITCH_OUT_ACL_SAMPLE) {
>> + ds_put_format(match, "ip && ct.new && ");
>> + } else {
>> + ds_put_format(match, "ip && (ct.new || !ct.trk) && ");
>> + }
>> build_acl_sample_register_match(match, acl, acl->sample_new);
>> build_acl_sample_action(actions, acl, acl->sample_new,
>> sample_domain_id);
>> @@ -6758,6 +6761,112 @@ build_acl_sample_est_stateful_flows(const
>> struct ovn_datapath *od,
>> static void build_acl_reject_action(struct ds *actions, bool
>> is_ingress);
>> +/* This builds a generic logical flow that samples traffic
>> + * that hit a stateless/stateful ACL that has sampling enabled with
>> + * single collector and all chassis supporting the sample with match
>> action.
>> + */
>> +static void
>> +build_acl_sample_generic_new_flows(const struct ovn_datapath *od,
>> + struct lflow_table *lflows,
>> + enum ovn_stage stage,
>> + enum acl_observation_stage obs_stage,
>> + struct ds *match, struct ds *actions,
>> + const struct
>> nbrec_sample_collector *coll,
>> + uint8_t sample_domain_id, bool
>> stateful,
>> + struct lflow_ref *lflow_ref)
>> +{
>> + ds_clear(match);
>> + ds_clear(actions);
>> +
>> + /* Match on new connections. However, for stateful to-lport
>> ACLs, due to
>> + * skip_port_from_conntrack() conntrack state might be cleared, so
>> + * take that into account too. */
>> + const char *new_conn_match = "";
>> + if (stateful) {
>> + if (stage != S_SWITCH_OUT_ACL_SAMPLE) {
>> + new_conn_match = "&& ct.new ";
>> + } else {
>> + new_conn_match = "&& (ct.new || !ct.trk) ";
>> + }
>> + }
>> +
>> + ds_put_format(match, "ip %s&& "REG_OBS_COLLECTOR_ID_NEW" ==
>> %"PRIu8" && "
>
> To me, it seems really strange to include "&& " and the trailing space
> as part of new_conn_match. I think formatting the match string looks
> more natural like this:
>
> ds_put_format(match, "ip && %s && "REG_OBS_COLLECTOR_ID_NEW" == ...
>
Thanks, Mark, for the review.
You're right, "%s&&" is quite ugly. We do need to account for the fact
that if "!stateful" new_conn_match is "".
What if I change this to:
const char *new_conn_match = "ip";
if (stateful) {
if (stage != S_SWITCH_OUT_ACL_SAMPLE) {
new_conn_match = "ip && ct.new";
} else {
new_conn_match = "ip && (ct.new || !ct.trk)";
}
}
ds_put_format(match, "%s && "REG_OBS_COLLECTOR_ID_NEW" == %"PRIu8" && "
REGBIT_ACL_OBS_STAGE " == %"PRIu8, new_conn_match,
(uint8_t) coll->id,
(uint8_t) obs_stage);
Does this look better? If so, I can include it in v7.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
