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
