On Mon, Mar 25, 2024 at 5:59 PM Jacob Tanenbaum <jtane...@redhat.com> wrote:

Hi Jacob,

thank you for he patch, the subject should end with a dot, and there is one
more thing
down below.

currently there are 2 QoS pipelines for ingress (ls_in_qos_mark,
> ls_in_qos_meter) and egress (ls_out_qos_mark, ls_out_qos_meter). This is
> not necessary as there are no actions across the two pipelines that
> depend on each other. The two pipelines can be merged.
>
> https://issues.redhat.com/browse/FDP-397


Should be:
Reported-at: https://issues.redhat.com/browse/FDP-397


>
>
> Signed-off-by: Jacob Tanenbaum <jtane...@redhat.com>
> ---
>  northd/northd.c     | 65 +++++++++++++++++----------------------------
>  northd/northd.h     | 46 +++++++++++++++-----------------
>  tests/ovn-northd.at | 24 ++++++++---------
>  tests/ovn.at        |  9 +++----
>  4 files changed, 62 insertions(+), 82 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 1839b7d8b..d5aab756f 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3484,7 +3484,7 @@ build_lb_vip_actions(const struct ovn_northd_lb *lb,
>      }
>
>      if (reject) {
> -        int stage = ls_dp ? ovn_stage_get_table(S_SWITCH_OUT_QOS_MARK)
> +        int stage = ls_dp ? ovn_stage_get_table(S_SWITCH_OUT_QOS)
>                            : ovn_stage_get_table(S_ROUTER_OUT_SNAT);
>          ds_clear(action);
>          ds_put_format(action, "reg0 = 0; reject { outport <-> inport; "
> @@ -6705,7 +6705,7 @@ build_acl_action_lflows(const struct
> ls_stateful_record *ls_stateful_rec,
>              "/* eth.dst <-> eth.src; ip.dst <-> ip.src; is implicit. */ "
>              "outport <-> inport; next(pipeline=%s,table=%d); };",
>              ingress ? "egress" : "ingress",
> -            ingress ? ovn_stage_get_table(S_SWITCH_OUT_QOS_MARK)
> +            ingress ? ovn_stage_get_table(S_SWITCH_OUT_QOS)
>                  : ovn_stage_get_table(S_SWITCH_IN_L2_LKUP));
>
>          ovn_lflow_metered(lflows, od, stage, 1000,
> @@ -7081,24 +7081,39 @@ build_qos(struct ovn_datapath *od, struct
> lflow_table *lflows,
>            struct lflow_ref *lflow_ref) {
>      struct ds action = DS_EMPTY_INITIALIZER;
>
> -    ovn_lflow_add(lflows, od, S_SWITCH_IN_QOS_MARK, 0, "1", "next;",
> +    ovn_lflow_add(lflows, od, S_SWITCH_IN_QOS, 0, "1", "next;",
>                    lflow_ref);
> -    ovn_lflow_add(lflows, od, S_SWITCH_OUT_QOS_MARK, 0, "1", "next;",
> -                  lflow_ref);
> -    ovn_lflow_add(lflows, od, S_SWITCH_IN_QOS_METER, 0, "1", "next;",
> -                  lflow_ref);
> -    ovn_lflow_add(lflows, od, S_SWITCH_OUT_QOS_METER, 0, "1", "next;",
> +    ovn_lflow_add(lflows, od, S_SWITCH_OUT_QOS, 0, "1", "next;",
>                    lflow_ref);
>
>      for (size_t i = 0; i < od->nbs->n_qos_rules; i++) {
>          struct nbrec_qos *qos = od->nbs->qos_rules[i];
>          bool ingress = !strcmp(qos->direction, "from-lport") ? true
> :false;
> -        enum ovn_stage stage = ingress ? S_SWITCH_IN_QOS_MARK :
> S_SWITCH_OUT_QOS_MARK;
> +        enum ovn_stage stage = ingress ? S_SWITCH_IN_QOS :
> S_SWITCH_OUT_QOS;
>          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>          int64_t rate = 0;
>          int64_t burst = 0;
>
>          ds_clear(&action);
> +        for (size_t n = 0; n < qos->n_bandwidth; n++) {
> +            if (!strcmp(qos->key_bandwidth[n], "rate")) {
> +                rate = qos->value_bandwidth[n];
> +            } else if (!strcmp(qos->key_bandwidth[n], "burst")) {
> +                burst = qos->value_bandwidth[n];
> +            }
> +        }
> +        if (rate) {
> +            stage = ingress ? S_SWITCH_IN_QOS : S_SWITCH_OUT_QOS;
> +            if (burst) {
> +                ds_put_format(&action,
> +                              "set_meter(%"PRId64", %"PRId64"); ",
> +                              rate, burst);
> +            } else {
> +                ds_put_format(&action,
> +                              "set_meter(%"PRId64"); ",
> +                              rate);
> +            }
> +        }
>          for (size_t j = 0; j < qos->n_action; j++) {
>              if (!strcmp(qos->key_action[j], "dscp")) {
>                  if (qos->value_action[j] > QOS_MAX_DSCP) {
> @@ -7115,43 +7130,11 @@ build_qos(struct ovn_datapath *od, struct
> lflow_table *lflows,
>                                qos->value_action[j]);
>              }
>          }
> -
> -        if (action.length) {
>              ds_put_cstr(&action, "next;");
> -            ovn_lflow_add_with_hint(lflows, od, stage, qos->priority,
> -                                    qos->match, ds_cstr(&action),
> -                                    &qos->header_, lflow_ref);
> -        }
> -
> -        for (size_t n = 0; n < qos->n_bandwidth; n++) {
> -            if (!strcmp(qos->key_bandwidth[n], "rate")) {
> -                rate = qos->value_bandwidth[n];
> -            } else if (!strcmp(qos->key_bandwidth[n], "burst")) {
> -                burst = qos->value_bandwidth[n];
> -            }
> -        }
> -        if (rate) {
> -            stage = ingress ? S_SWITCH_IN_QOS_METER :
> S_SWITCH_OUT_QOS_METER;
> -            ds_clear(&action);
> -            if (burst) {
> -                ds_put_format(&action,
> -                              "set_meter(%"PRId64", %"PRId64"); next;",
> -                              rate, burst);
> -            } else {
> -                ds_put_format(&action,
> -                              "set_meter(%"PRId64"); next;",
> -                              rate);
> -            }
> -
> -            /* Ingress and Egress QoS Meter Table.
> -             *
> -             * We limit the bandwidth of this flow by adding a meter
> table.
> -             */
>              ovn_lflow_add_with_hint(lflows, od, stage,
>                                      qos->priority,
>                                      qos->match, ds_cstr(&action),
>                                      &qos->header_, lflow_ref);
> -        }
>      }
>      ds_destroy(&action);
>  }
> diff --git a/northd/northd.h b/northd/northd.h
> index 3f1cd8341..d5161d17e 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -397,27 +397,26 @@ enum ovn_stage {
>      PIPELINE_STAGE(SWITCH, IN,  ACL_HINT,       7, "ls_in_acl_hint")
> \
>      PIPELINE_STAGE(SWITCH, IN,  ACL_EVAL,       8, "ls_in_acl_eval")
> \
>      PIPELINE_STAGE(SWITCH, IN,  ACL_ACTION,     9, "ls_in_acl_action")
> \
> -    PIPELINE_STAGE(SWITCH, IN,  QOS_MARK,      10, "ls_in_qos_mark")
> \
> -    PIPELINE_STAGE(SWITCH, IN,  QOS_METER,     11, "ls_in_qos_meter")
>  \
> -    PIPELINE_STAGE(SWITCH, IN,  LB_AFF_CHECK,  12, "ls_in_lb_aff_check")
> \
> -    PIPELINE_STAGE(SWITCH, IN,  LB,            13, "ls_in_lb")
> \
> -    PIPELINE_STAGE(SWITCH, IN,  LB_AFF_LEARN,  14, "ls_in_lb_aff_learn")
> \
> -    PIPELINE_STAGE(SWITCH, IN,  PRE_HAIRPIN,   15, "ls_in_pre_hairpin")
>  \
> -    PIPELINE_STAGE(SWITCH, IN,  NAT_HAIRPIN,   16, "ls_in_nat_hairpin")
>  \
> -    PIPELINE_STAGE(SWITCH, IN,  HAIRPIN,       17, "ls_in_hairpin")
>  \
> -    PIPELINE_STAGE(SWITCH, IN,  ACL_AFTER_LB_EVAL,  18, \
> +    PIPELINE_STAGE(SWITCH, IN,  QOS,           10, "ls_in_qos")    \
> +    PIPELINE_STAGE(SWITCH, IN,  LB_AFF_CHECK,  11, "ls_in_lb_aff_check")
> \
> +    PIPELINE_STAGE(SWITCH, IN,  LB,            12, "ls_in_lb")
> \
> +    PIPELINE_STAGE(SWITCH, IN,  LB_AFF_LEARN,  13, "ls_in_lb_aff_learn")
> \
> +    PIPELINE_STAGE(SWITCH, IN,  PRE_HAIRPIN,   14, "ls_in_pre_hairpin")
>  \
> +    PIPELINE_STAGE(SWITCH, IN,  NAT_HAIRPIN,   15, "ls_in_nat_hairpin")
>  \
> +    PIPELINE_STAGE(SWITCH, IN,  HAIRPIN,       16, "ls_in_hairpin")
>  \
> +    PIPELINE_STAGE(SWITCH, IN,  ACL_AFTER_LB_EVAL,  17, \
>                     "ls_in_acl_after_lb_eval")  \
> -    PIPELINE_STAGE(SWITCH, IN,  ACL_AFTER_LB_ACTION,  19, \
> +    PIPELINE_STAGE(SWITCH, IN,  ACL_AFTER_LB_ACTION,  18, \
>                     "ls_in_acl_after_lb_action")  \
> -    PIPELINE_STAGE(SWITCH, IN,  STATEFUL,      20, "ls_in_stateful")
> \
> -    PIPELINE_STAGE(SWITCH, IN,  ARP_ND_RSP,    21, "ls_in_arp_rsp")
>  \
> -    PIPELINE_STAGE(SWITCH, IN,  DHCP_OPTIONS,  22, "ls_in_dhcp_options")
> \
> -    PIPELINE_STAGE(SWITCH, IN,  DHCP_RESPONSE, 23, "ls_in_dhcp_response")
> \
> -    PIPELINE_STAGE(SWITCH, IN,  DNS_LOOKUP,    24, "ls_in_dns_lookup")
> \
> -    PIPELINE_STAGE(SWITCH, IN,  DNS_RESPONSE,  25, "ls_in_dns_response")
> \
> -    PIPELINE_STAGE(SWITCH, IN,  EXTERNAL_PORT, 26, "ls_in_external_port")
> \
> -    PIPELINE_STAGE(SWITCH, IN,  L2_LKUP,       27, "ls_in_l2_lkup")
>  \
> -    PIPELINE_STAGE(SWITCH, IN,  L2_UNKNOWN,    28, "ls_in_l2_unknown")
> \
> +    PIPELINE_STAGE(SWITCH, IN,  STATEFUL,      19, "ls_in_stateful")
> \
> +    PIPELINE_STAGE(SWITCH, IN,  ARP_ND_RSP,    20, "ls_in_arp_rsp")
>  \
> +    PIPELINE_STAGE(SWITCH, IN,  DHCP_OPTIONS,  21, "ls_in_dhcp_options")
> \
> +    PIPELINE_STAGE(SWITCH, IN,  DHCP_RESPONSE, 22, "ls_in_dhcp_response")
> \
> +    PIPELINE_STAGE(SWITCH, IN,  DNS_LOOKUP,    23, "ls_in_dns_lookup")
> \
> +    PIPELINE_STAGE(SWITCH, IN,  DNS_RESPONSE,  24, "ls_in_dns_response")
> \
> +    PIPELINE_STAGE(SWITCH, IN,  EXTERNAL_PORT, 25, "ls_in_external_port")
> \
> +    PIPELINE_STAGE(SWITCH, IN,  L2_LKUP,       26, "ls_in_l2_lkup")
>  \
> +    PIPELINE_STAGE(SWITCH, IN,  L2_UNKNOWN,    27, "ls_in_l2_unknown")
> \
>
>  \
>      /* Logical switch egress stages. */
>  \
>      PIPELINE_STAGE(SWITCH, OUT, PRE_ACL,      0, "ls_out_pre_acl")
> \
> @@ -426,11 +425,10 @@ enum ovn_stage {
>      PIPELINE_STAGE(SWITCH, OUT, ACL_HINT,     3, "ls_out_acl_hint")
>  \
>      PIPELINE_STAGE(SWITCH, OUT, ACL_EVAL,     4, "ls_out_acl_eval")
>  \
>      PIPELINE_STAGE(SWITCH, OUT, ACL_ACTION,   5, "ls_out_acl_action")
>  \
> -    PIPELINE_STAGE(SWITCH, OUT, QOS_MARK,     6, "ls_out_qos_mark")
>  \
> -    PIPELINE_STAGE(SWITCH, OUT, QOS_METER,    7, "ls_out_qos_meter")
> \
> -    PIPELINE_STAGE(SWITCH, OUT, STATEFUL,     8, "ls_out_stateful")
>  \
> -    PIPELINE_STAGE(SWITCH, OUT, CHECK_PORT_SEC,  9,
> "ls_out_check_port_sec") \
> -    PIPELINE_STAGE(SWITCH, OUT, APPLY_PORT_SEC, 10,
> "ls_out_apply_port_sec") \
> +    PIPELINE_STAGE(SWITCH, OUT, QOS,          6, "ls_out_qos")       \
> +    PIPELINE_STAGE(SWITCH, OUT, STATEFUL,     7, "ls_out_stateful")
>  \
> +    PIPELINE_STAGE(SWITCH, OUT, CHECK_PORT_SEC,  8,
> "ls_out_check_port_sec") \
> +    PIPELINE_STAGE(SWITCH, OUT, APPLY_PORT_SEC, 9,
> "ls_out_apply_port_sec") \
>                                                                        \
>      /* Logical router ingress stages. */                              \
>      PIPELINE_STAGE(ROUTER, IN,  ADMISSION,       0, "lr_in_admission")
> \
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 7893b0540..87c7e49a6 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -10080,6 +10080,7 @@ check ovn-nbctl qos-add ls from-lport 100 ip4
> rate=100 burst=1000
>  check ovn-nbctl qos-add ls from-lport 101 ip4 mark=15
>  check ovn-nbctl qos-add ls from-lport 102 ip4 dscp=16
>  check ovn-nbctl qos-add ls from-lport 103 ip4 dscp=17 mark=18
> +check ovn-nbctl qos-add ls from-lport 104 ip4 dscp=18 mark=19 rate=100
>
>  check ovn-nbctl qos-add ls to-lport 100 ip4 rate=100 burst=1000
>  check ovn-nbctl qos-add ls to-lport 101 ip4 mark=15
> @@ -10088,18 +10089,17 @@ check ovn-nbctl qos-add ls to-lport 103 ip4
> dscp=17 mark=18
>  check ovn-nbctl --wait=sb sync
>
>  AT_CHECK([ovn-sbctl lflow-list ls | grep qos | ovn_strip_lflows], [0],
> [dnl
> -  table=??(ls_in_qos_mark     ), priority=0    , match=(1), action=(next;)
> -  table=??(ls_in_qos_mark     ), priority=101  , match=(ip4),
> action=(pkt.mark = 15; next;)
> -  table=??(ls_in_qos_mark     ), priority=102  , match=(ip4),
> action=(ip.dscp = 16; next;)
> -  table=??(ls_in_qos_mark     ), priority=103  , match=(ip4),
> action=(ip.dscp = 17; pkt.mark = 18; next;)
> -  table=??(ls_in_qos_meter    ), priority=0    , match=(1), action=(next;)
> -  table=??(ls_in_qos_meter    ), priority=100  , match=(ip4),
> action=(set_meter(100, 1000); next;)
> -  table=??(ls_out_qos_mark    ), priority=0    , match=(1), action=(next;)
> -  table=??(ls_out_qos_mark    ), priority=101  , match=(ip4),
> action=(pkt.mark = 15; next;)
> -  table=??(ls_out_qos_mark    ), priority=102  , match=(ip4),
> action=(ip.dscp = 16; next;)
> -  table=??(ls_out_qos_mark    ), priority=103  , match=(ip4),
> action=(ip.dscp = 17; pkt.mark = 18; next;)
> -  table=??(ls_out_qos_meter   ), priority=0    , match=(1), action=(next;)
> -  table=??(ls_out_qos_meter   ), priority=100  , match=(ip4),
> action=(set_meter(100, 1000); next;)
> +  table=??(ls_in_qos          ), priority=0    , match=(1), action=(next;)
> +  table=??(ls_in_qos          ), priority=100  , match=(ip4),
> action=(set_meter(100, 1000); next;)
> +  table=??(ls_in_qos          ), priority=101  , match=(ip4),
> action=(pkt.mark = 15; next;)
> +  table=??(ls_in_qos          ), priority=102  , match=(ip4),
> action=(ip.dscp = 16; next;)
> +  table=??(ls_in_qos          ), priority=103  , match=(ip4),
> action=(ip.dscp = 17; pkt.mark = 18; next;)
> +  table=??(ls_in_qos          ), priority=104  , match=(ip4),
> action=(set_meter(100); ip.dscp = 18; pkt.mark = 19; next;)
> +  table=??(ls_out_qos         ), priority=0    , match=(1), action=(next;)
> +  table=??(ls_out_qos         ), priority=100  , match=(ip4),
> action=(set_meter(100, 1000); next;)
> +  table=??(ls_out_qos         ), priority=101  , match=(ip4),
> action=(pkt.mark = 15; next;)
> +  table=??(ls_out_qos         ), priority=102  , match=(ip4),
> action=(ip.dscp = 16; next;)
> +  table=??(ls_out_qos         ), priority=103  , match=(ip4),
> action=(ip.dscp = 17; pkt.mark = 18; next;)
>  ])
>
>  AT_CLEANUP
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 4d0c7ad53..f04b74210 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -10205,9 +10205,8 @@ check ovn-nbctl --wait=hv set QoS $qos_id
> bandwidth=rate=100
>  # check at hv with a qos meter table
>  AT_CHECK([as hv ovs-ofctl dump-meters br-int -O OpenFlow13 | grep
> rate=100 | wc -l], [0], [1
>  ])
> -AT_CHECK([as hv ovs-ofctl dump-flows br-int -O OpenFlow13 | grep meter |
> wc -l], [0], [1
> +AT_CHECK([as hv ovs-ofctl dump-flows br-int -O OpenFlow13 | grep meter |
> wc -l], [0], [2
>  ])
> -
>  # Update the DSCP marking
>  check ovn-nbctl --wait=hv set QoS $qos_id action=dscp=63
>  check_tos 63
> @@ -10218,7 +10217,7 @@ check ovn-nbctl --wait=hv set QoS $qos_id
> bandwidth=rate=4294967295,burst=429496
>  # check at hv with a qos meter table
>  AT_CHECK([as hv ovs-ofctl dump-meters br-int -O OpenFlow13 | grep
> burst_size=4294967295 | wc -l], [0], [1
>  ])
> -AT_CHECK([as hv ovs-ofctl dump-flows br-int -O OpenFlow13 | grep meter |
> wc -l], [0], [1
> +AT_CHECK([as hv ovs-ofctl dump-flows br-int -O OpenFlow13 | grep meter |
> wc -l], [0], [2
>  ])
>
>  check ovn-nbctl --wait=hv set QoS $qos_id match="outport\=\=\"lp2\""
> direction="to-lport"
> @@ -22071,7 +22070,7 @@ check_row_count Port_Binding 1
> logical_port=sw0-vir virtual_parent=sw0-p1
>  wait_for_ports_up sw0-vir
>  check ovn-nbctl --wait=hv sync
>  AT_CHECK([test 2 = `cat hv1/ovn-controller.log | grep "pinctrl received
> packet-in" | \
> -grep opcode=BIND_VPORT | grep OF_Table_ID=29 | wc -l`])
> +grep opcode=BIND_VPORT | grep OF_Table_ID=28 | wc -l`])
>
>  wait_row_count Port_Binding 1 logical_port=sw0-vir6 chassis=$hv1_ch_uuid
>  check_row_count Port_Binding 1 logical_port=sw0-vir6 virtual_parent=sw0-p1
> @@ -37685,7 +37684,7 @@ tpa=$(ip_to_hex 10 0 0 10)
>  send_garp 1 1 $eth_src $eth_dst $spa $tpa
>
>  OVS_WAIT_UNTIL([test 1 = `cat hv1/ovn-controller.log | grep "pinctrl
> received  packet-in" | \
> -grep opcode=BIND_VPORT | grep OF_Table_ID=29 | wc -l`])
> +grep opcode=BIND_VPORT | grep OF_Table_ID=28 | wc -l`])
>
>  sleep_controller hv1
>
> --
> 2.42.0
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
With those two above fixed:

Acked-by: Ales Musil <amu...@redhat.com>

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA <https://www.redhat.com>

amu...@redhat.com
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to