On Mon, Mar 25, 2024 at 5:59 PM Jacob Tanenbaum 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
> ---
> 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();
> +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(,
> + "set_meter(%"PRId64", %"PRId64"); ",
> + rate, burst);
> +} else {
> +ds_put_format(,
> + "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(, "next;");
> -ovn_lflow_add_with_hint(lflows, od, stage, qos->priority,
> -qos->match, ds_cstr(),
> ->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];
> -}
> -}
> -
