On 2/1/24 17:51, Lorenzo Bianconi wrote: > Add the capbility to mark (through pkt.mark) incoming/outgoing packets > in logical_switch datapath according to user configured QoS rule. > > Co-developed-by: Dumitru Ceara <[email protected]> > Reported-at: https://issues.redhat.com/browse/FDP-42 > Signed-off-by: Lorenzo Bianconi <[email protected]> > --- > - move qos packet mark action in QOS_MARK ls {ingress/egress} stage > --- > NEWS | 2 + > northd/northd.c | 33 +++++++++++++--- > northd/ovn-northd.8.xml | 6 +++ > ovn-nb.ovsschema | 9 ++--- > ovn-nb.xml | 12 +++++- > tests/ovn-nbctl.at | 8 +++- > tests/ovn-northd.at | 35 +++++++++++++++++ > tests/ovn.at | 83 +++++++++++++++++++++++++++++++++++++++ > utilities/ovn-nbctl.8.xml | 5 ++- > utilities/ovn-nbctl.c | 27 ++++++++++--- > 10 files changed, 199 insertions(+), 21 deletions(-) > > diff --git a/NEWS b/NEWS > index 6553bd078..a8beb09fb 100644 > --- a/NEWS > +++ b/NEWS > @@ -18,6 +18,8 @@ Post v23.09.0 > - Support selecting encapsulation IP based on the source/destination VIF's > settting. See ovn-controller(8) 'external_ids:ovn-encap-ip' for more > details. > + - Add the capability to mark (through pkt.mark) incoming/outgoing packets > + in the logical switch datapath according to user configured QoS rule. > > OVN v23.09.0 - 15 Sep 2023 > -------------------------- > diff --git a/northd/northd.c b/northd/northd.c > index d2091d4bc..a77919af3 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -8363,6 +8363,8 @@ build_acls(struct ovn_datapath *od, const struct > chassis_features *features, > ds_destroy(&actions); > } > > +#define QOS_MAX_DSCP 63 > + > static void > build_qos(struct ovn_datapath *od, struct hmap *lflows) { > struct ds action = DS_EMPTY_INITIALIZER; > @@ -8376,21 +8378,40 @@ build_qos(struct ovn_datapath *od, struct hmap > *lflows) { > 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; > + 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 j = 0; j < qos->n_action; j++) { > + if (strcmp(qos->key_action[j], "dscp") && > + strcmp(qos->key_action[j], "mark")) { > + continue; > + } > + > if (!strcmp(qos->key_action[j], "dscp")) { > - ds_clear(&action); > - ds_put_format(&action, "ip.dscp = %"PRId64"; next;", > + if (qos->value_action[j] > QOS_MAX_DSCP) { > + VLOG_WARN_RL(&rl, "Bad 'dscp' value %"PRId64" in qos " > + UUID_FMT, qos->value_action[j], > + UUID_ARGS(&qos->header_.uuid)); > + continue; > + } > + > + ds_put_format(&action, "ip.dscp = %"PRId64"; ", > + qos->value_action[j]); > + } else if (!strcmp(qos->key_action[j], "mark")) { > + ds_put_format(&action, "pkt.mark = %"PRId64"; ", > qos->value_action[j]); > - ovn_lflow_add_with_hint(lflows, od, stage, > - qos->priority, > - qos->match, ds_cstr(&action), > - &qos->header_); > } > } > > + 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_); > + } > + > for (size_t n = 0; n < qos->n_bandwidth; n++) { > if (!strcmp(qos->key_bandwidth[n], "rate")) { > rate = qos->value_bandwidth[n]; > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml > index 068d47e1a..9583abeff 100644 > --- a/northd/ovn-northd.8.xml > +++ b/northd/ovn-northd.8.xml > @@ -915,6 +915,12 @@ > QoS table. > </li> > > + <li> > + For every qos_rules entry in a logical switch with packet marking > + enabled, a flow will be added at the priority mentioned in the > + QoS table. > + </li> > + > <li> > One priority-0 fallback flow that matches all packets and advances to > the next table. > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema > index b2e0993e0..7bb8f36c0 100644 > --- a/ovn-nb.ovsschema > +++ b/ovn-nb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Northbound", > - "version": "7.2.0", > - "cksum": "1069338687 34162", > + "version": "7.3.0", > + "cksum": "2786772995 34106", > "tables": { > "NB_Global": { > "columns": { > @@ -293,10 +293,9 @@ > "enum": ["set", ["from-lport", > "to-lport"]]}}}, > "match": {"type": "string"}, > "action": {"type": {"key": {"type": "string", > - "enum": ["set", ["dscp"]]}, > + "enum": ["set", ["dscp", > "mark"]]}, > "value": {"type": "integer", > - "minInteger": 0, > - "maxInteger": 63}, > + "minInteger": 0},
Not a full review, but database integer type allows signed 64bit values, while the packet mark is an unsigned 32bit field. The type restriction here is removed but the parsing code doesn't check the value to be within 32 bits. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
