diff mbox series

[ovs-dev,v2] northd: Add qos packet marking.

Message ID 4b7bde002f6006088e5d41d9728b811a54579ce8.1706806148.git.lorenzo.bianconi@redhat.com
State Changes Requested
Headers show
Series [ovs-dev,v2] northd: Add qos packet marking. | expand

Checks

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

Commit Message

Lorenzo Bianconi Feb. 1, 2024, 4:51 p.m. UTC
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 <dceara@redhat.com>
Reported-at: https://issues.redhat.com/browse/FDP-42
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
- 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(-)

Comments

Ilya Maximets Feb. 1, 2024, 5:48 p.m. UTC | #1
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 <dceara@redhat.com>
> Reported-at: https://issues.redhat.com/browse/FDP-42
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
> - 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.
diff mbox series

Patch

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},
                                     "min": 0, "max": "unlimited"}},
                 "bandwidth": {"type": {"key": {"type": "string",
                                                "enum": ["set", ["rate",
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 765ffcf2e..5b593f6a7 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -2808,12 +2808,22 @@  or
     </column>
 
     <column name="action">
-      <p>When specified, matching flows will have DSCP marking applied.</p>
+      <p>
+        When <code>dscp</code> action is specified, matching flows will have
+        have DSCP marking applied.
+        When <code>mark</code> action is specified, matching flows will have
+        packet marking applied.
+      </p>
+
       <ul>
         <li>
           <code>dscp</code>: The value of this action should be in the
           range of 0 to 63 (inclusive).
         </li>
+        <li>
+           <code>mark</code>: The value of this action should be a positive
+           integer.
+        </li>
       </ul>
     </column>
 
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 7f37d7716..b11e4ec6b 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -316,6 +316,8 @@  AT_CHECK([ovn-nbctl qos-add ls0 from-lport 400 tcp dscp=0 rate=300 burst=3000])
 AT_CHECK([ovn-nbctl qos-add ls0 to-lport 300 tcp dscp=48])
 AT_CHECK([ovn-nbctl qos-add ls0 to-lport 200 ip rate=101])
 AT_CHECK([ovn-nbctl qos-add ls0 to-lport 100 ip4 dscp=13 rate=301 burst=30000])
+AT_CHECK([ovn-nbctl qos-add ls0 to-lport 101 ip4 mark=15])
+AT_CHECK([ovn-nbctl qos-add ls0 to-lport 102 ip4 dscp=16 mark=17])
 
 dnl Add duplicated qos
 AT_CHECK([ovn-nbctl qos-add ls0 to-lport 100 ip4 dscp=11 rate=302 burst=30002], [1], [], [stderr])
@@ -328,6 +330,8 @@  from-lport   500 (udp) rate=100 burst=1000
 from-lport   400 (tcp) rate=300 burst=3000 dscp=0
   to-lport   300 (tcp) dscp=48
   to-lport   200 (ip) rate=101
+  to-lport   102 (ip4) dscp=16 mark=17
+  to-lport   101 (ip4) mark=15
   to-lport   100 (ip4) rate=301 burst=30000 dscp=13
 ])
 
@@ -371,11 +375,11 @@  AT_CHECK([ovn-nbctl qos-add ls0 from-lport 600 ip dscp=-1], [1], [],
 ])
 
 AT_CHECK([ovn-nbctl qos-add ls0 from-lport 600 ip dscpa=-1], [1], [],
-[ovn-nbctl: dscpa=-1: supported arguments are "dscp=", "rate=", and "burst="
+[ovn-nbctl: dscpa=-1: supported arguments are "dscp=", "rate=", "burst=" and "mark="
 ])
 
 AT_CHECK([ovn-nbctl qos-add ls0 from-lport 600 ip burst=123], [1], [],
-[ovn-nbctl: Either "rate" and/or "dscp" must be specified
+[ovn-nbctl: Either "mark", "rate" and/or "dscp" must be specified
 ])])
 
 dnl ---------------------------------------------------------------------
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 67e81ddba..8d2e81041 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -9860,6 +9860,41 @@  AT_CHECK([fetch_column sb:Port_Binding options logical_port=public |grep -q 'qos
 AT_CLEANUP
 ])
 
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([check OVN QoS mark/meter logical flows])
+AT_KEYWORDS([OVN-QoS])
+ovn_start
+
+check ovn-nbctl ls-add ls
+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 to-lport 100 ip4 rate=100 burst=1000
+check ovn-nbctl qos-add ls to-lport 101 ip4 mark=15
+check ovn-nbctl qos-add ls to-lport 102 ip4 dscp=16
+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 | sed 's/table=../table=??/'], [0], [dnl
+  table=??(ls_in_qos_mark     ), priority=103  , match=(ip4), action=(ip.dscp = 17; pkt.mark = 18; next;)
+  table=??(ls_in_qos_mark     ), priority=102  , match=(ip4), action=(ip.dscp = 16; next;)
+  table=??(ls_in_qos_mark     ), priority=101  , match=(ip4), action=(pkt.mark = 15; next;)
+  table=??(ls_in_qos_mark     ), priority=0    , match=(1), action=(next;)
+  table=??(ls_in_qos_meter    ), priority=100  , match=(ip4), action=(set_meter(100, 1000); next;)
+  table=??(ls_in_qos_meter    ), priority=0    , match=(1), action=(next;)
+  table=??(ls_out_qos_mark    ), priority=103  , match=(ip4), action=(ip.dscp = 17; pkt.mark = 18; next;)
+  table=??(ls_out_qos_mark    ), priority=102  , match=(ip4), action=(ip.dscp = 16; next;)
+  table=??(ls_out_qos_mark    ), priority=101  , match=(ip4), action=(pkt.mark = 15; next;)
+  table=??(ls_out_qos_mark    ), priority=0    , match=(1), action=(next;)
+  table=??(ls_out_qos_meter   ), priority=100  , match=(ip4), action=(set_meter(100, 1000); next;)
+  table=??(ls_out_qos_meter   ), priority=0    , match=(1), action=(next;)
+])
+
+AT_CLEANUP
+])
+
 AT_SETUP([Tiered ACL logical flows])
 AT_KEYWORDS([acl])
 
diff --git a/tests/ovn.at b/tests/ovn.at
index 28c6b6c34..ee7c8cb1e 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -37929,3 +37929,86 @@  AT_CHECK([ovn-sbctl --db=ssl:127.0.0.1:$TCP_PORT \
 
 OVS_APP_EXIT_AND_WAIT([ovsdb-server])
 AT_CLEANUP
+
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([QoS packet marking])
+AT_KEYWORDS([ovn-qos-pkt-marking])
+AT_SKIP_IF([test $HAVE_SCAPY = no])
+ovn_start
+
+check ovn-nbctl lr-add lr0
+check ovn-nbctl ls-add ls0
+check ovn-nbctl ls-add ls1
+
+check ovn-nbctl lrp-add lr0 lrp0 f0:00:00:00:00:10 10.0.0.254/24
+check ovn-nbctl lsp-add ls0 lrp-s0 -- \
+    set Logical_Switch_Port lrp-s0 type=router \
+    options:router-port=lrp0 addresses='"f0:00:00:00:00:10"'
+check ovn-nbctl lrp-add lr0 lrp1 f0:00:00:00:00:20 20.0.0.254/24
+check ovn-nbctl lsp-add ls1 lrp-s1 -- \
+    set Logical_Switch_Port lrp-s1 type=router \
+    options:router-port=lrp1 addresses='"f0:00:00:00:00:20"'
+
+check ovn-nbctl --wait=sb lsp-add ls0 lp0
+check ovn-nbctl lsp-set-addresses lp0 f0:00:00:00:00:01
+check ovn-nbctl --wait=sb lsp-add ls0 lp1
+check ovn-nbctl lsp-set-addresses lp1 f0:00:00:00:00:02
+
+check ovn-nbctl --wait=sb lsp-add ls1 lp2
+check ovn-nbctl lsp-set-addresses lp2 f0:00:00:00:00:11
+check ovn-nbctl --wait=sb sync
+
+net_add n1
+sim_add hv
+
+as hv
+check ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+
+check ovs-vsctl add-port br-int vif0 -- \
+    set Interface vif0 external-ids:iface-id=lp0 \
+    options:tx_pcap=hv/vif0-tx.pcap \
+    options:rxq_pcap=hv/vif0-rx.pcap \
+    ofport-request=1
+check ovs-vsctl add-port br-int vif1 -- \
+    set Interface vif1 external-ids:iface-id=lp1 \
+    options:tx_pcap=hv/vif1-tx.pcap \
+    options:rxq_pcap=hv/vif1-rx.pcap \
+    ofport-request=2
+check ovs-vsctl add-port br-int vif2 -- \
+    set Interface vif2 external-ids:iface-id=lp2 \
+    options:tx_pcap=hv/vif2-tx.pcap \
+    options:rxq_pcap=hv/vif2-rx.pcap \
+    ofport-request=3
+
+# Create QoS rules for packet marking.
+check ovn-nbctl qos-add ls0 from-lport 100 "inport==\"lp0\" && udp" mark=48
+check ovn-nbctl qos-add ls0 to-lport 100 "outport==\"lrp-s0\" && udp" mark=49
+check_row_count nb:QoS 2
+
+# Create some flows to count pkt marking.
+check ovn-nbctl acl-add ls0 to-lport 1002 "outport==\"lp1\" && pkt.mark == 48" drop
+check ovn-nbctl lr-policy-add lr0 200 "pkt.mark == 49" drop
+wait_for_ports_up
+check ovn-nbctl --wait=hv sync
+
+send_udp_packet() {
+    local inport=$1 eth_src=$2 eth_dst=$3 ipv4_src=$4 ipv4_dst=$5
+    local packet=$(fmt_pkt "Ether(dst='${eth_dst}', src='${eth_src}')/ \
+                            IP(src='${ipv4_src}', dst='${ipv4_dst}', ttl=0x40)/ \
+                            UDP(sport=4242, dport=4343)")
+    as hv ovs-appctl netdev-dummy/receive vif$inport $packet
+}
+
+ls0_dp_key=$(printf "0x%x" $(fetch_column Datapath_Binding tunnel_key external_ids:name=ls0))
+lr0_dp_key=$(printf "0x%x" $(fetch_column Datapath_Binding tunnel_key external_ids:name=lr0))
+
+send_udp_packet 0 f0:00:00:00:00:01 f0:00:00:00:00:02 10.0.0.1 10.0.0.2
+OVS_WAIT_UNTIL([test 1 = $(as hv ovs-ofctl dump-flows br-int | grep -E "pkt_mark=0x30.*metadata=$ls0_dp_key" | grep -c n_packets=1)])
+
+send_udp_packet 0 f0:00:00:00:00:01 f0:00:00:00:00:10 10.0.0.1 10.0.0.10
+OVS_WAIT_UNTIL([test 1 = $(as hv ovs-ofctl dump-flows br-int | grep -E "pkt_mark=0x31.*metadata=$lr0_dp_key" | grep -c n_packets=1)])
+
+OVN_CLEANUP([hv])
+AT_CLEANUP
+])
diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
index 6f74bd557..c1a280191 100644
--- a/utilities/ovn-nbctl.8.xml
+++ b/utilities/ovn-nbctl.8.xml
@@ -463,7 +463,7 @@ 
 
     <h2>Logical Switch QoS Rule Commands</h2>
     <dl>
-      <dt>[<code>--may-exist</code>] <code>qos-add</code> <var>switch</var> <var>direction</var> <var>priority</var> <var>match</var> [<code>dscp=</code><var>dscp</var>] [<code>rate=</code><var>rate</var> [<code>burst=</code><var>burst</var>]]</dt>
+        <dt>[<code>--may-exist</code>] <code>qos-add</code> <var>switch</var> <var>direction</var> <var>priority</var> <var>match</var> [<code>mark=</code><var>mark</var>] [<code>dscp=</code><var>dscp</var>] [<code>rate=</code><var>rate</var> [<code>burst=</code><var>burst</var>]]</dt>
       <dd>
         <p>
           Adds QoS marking and metering rules to <var>switch</var>.
@@ -482,6 +482,9 @@ 
           <code>burst=</code><var>burst</var> specifies the burst rate
           limit in kilobits.  <code>dscp</code> and/or <code>rate</code>
           are required arguments.
+          If <code>mark=</code><var>mark</var> is specified, then matching
+          packets will be marked (through <code>pkt.mark</code>).
+          <var>mark</var> must be a positive integer.
         </p>
 
         <p>
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index 526369b68..14460991f 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -283,7 +283,7 @@  ACL commands:\n\
                             print ACLs for SWITCH\n\
 \n\
 QoS commands:\n\
-  qos-add SWITCH DIRECTION PRIORITY MATCH [rate=RATE [burst=BURST]] [dscp=DSCP]\n\
+  qos-add SWITCH DIRECTION PRIORITY MATCH [rate=RATE [burst=BURST]] [dscp=DSCP] [mark=MARK]\n\
                             add an QoS rule to SWITCH\n\
   qos-del SWITCH [{DIRECTION | UUID} [PRIORITY MATCH]]\n\
                             remove QoS rules from SWITCH\n\
@@ -2614,6 +2614,9 @@  nbctl_qos_list(struct ctl_context *ctx)
             if (!strcmp(qos_rule->key_action[j], "dscp")) {
                 ds_put_format(&ctx->output, " dscp=%"PRId64"",
                               qos_rule->value_action[j]);
+            } else if (!strcmp(qos_rule->key_action[j], "mark")) {
+                ds_put_format(&ctx->output, " mark=%"PRId64"",
+                              qos_rule->value_action[j]);
             }
         }
         ds_put_cstr(&ctx->output, "\n");
@@ -2631,6 +2634,7 @@  nbctl_pre_qos_add(struct ctl_context *ctx)
     ovsdb_idl_add_column(ctx->idl, &nbrec_qos_col_direction);
     ovsdb_idl_add_column(ctx->idl, &nbrec_qos_col_priority);
     ovsdb_idl_add_column(ctx->idl, &nbrec_qos_col_match);
+    ovsdb_idl_add_column(ctx->idl, &nbrec_qos_col_action);
 }
 
 static void
@@ -2640,6 +2644,7 @@  nbctl_qos_add(struct ctl_context *ctx)
     const char *direction;
     int64_t priority;
     int64_t dscp = -1;
+    int64_t mark = 0;
     int64_t rate = 0;
     int64_t burst = 0;
     char *error;
@@ -2669,6 +2674,13 @@  nbctl_qos_add(struct ctl_context *ctx)
                 return;
             }
         }
+        else if (!strncmp(ctx->argv[i], "mark=", 5)) {
+            if (!ovs_scan(ctx->argv[i] + 5, "%"SCNd64, &mark) || mark < 0) {
+                ctl_error(ctx, "%s: mark must be a positive integer",
+                          ctx->argv[i] + 5);
+                return;
+            }
+        }
         else if (!strncmp(ctx->argv[i], "rate=", 5)) {
             if (!ovs_scan(ctx->argv[i] + 5, "%"SCNd64, &rate)
                 || rate < 1 || rate > UINT32_MAX) {
@@ -2686,14 +2698,15 @@  nbctl_qos_add(struct ctl_context *ctx)
             }
         } else {
             ctl_error(ctx, "%s: supported arguments are \"dscp=\", \"rate=\", "
-                      "and \"burst=\"", ctx->argv[i]);
+                      "\"burst=\" and \"mark=\"", ctx->argv[i]);
             return;
         }
     }
 
     /* Validate rate and dscp. */
-    if (-1 == dscp && !rate) {
-        ctl_error(ctx, "Either \"rate\" and/or \"dscp\" must be specified");
+    if (-1 == dscp && !rate && !mark) {
+        ctl_error(ctx, "Either \"mark\", \"rate\" and/or \"dscp\" must be "
+                       "specified");
         return;
     }
 
@@ -2702,9 +2715,11 @@  nbctl_qos_add(struct ctl_context *ctx)
     nbrec_qos_set_priority(qos, priority);
     nbrec_qos_set_direction(qos, direction);
     nbrec_qos_set_match(qos, ctx->argv[4]);
+    if (mark) {
+        nbrec_qos_update_action_setkey(qos, "mark", mark);
+    }
     if (-1 != dscp) {
-        const char *dscp_key = "dscp";
-        nbrec_qos_set_action(qos, &dscp_key, &dscp, 1);
+        nbrec_qos_update_action_setkey(qos, "dscp", dscp);
     }
     if (rate) {
         const char *bandwidth_key[2] = {"rate", "burst"};