| Message ID | 20240325165842.509044-1-jtanenba@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,1/2] Merge QoS logical pipelines | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Mon, Mar 25, 2024 at 5:59 PM Jacob Tanenbaum <jtanenba@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 <jtanenba@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > With those two above fixed: Acked-by: Ales Musil <amusil@redhat.com>
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
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 Signed-off-by: Jacob Tanenbaum <jtanenba@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(-)