[ovs-dev] [patch v11 2/2] DSCP marking on packets egressing VIF interface
Mickey Spiegel
mickeys.dev at gmail.com
Fri Sep 2 05:47:25 UTC 2016
On Wed, Aug 31, 2016 at 12:11 AM, <bschanmu at redhat.com> wrote:
> ovn-northd sets 'ip.dscp' to the DSCP value
>
If we were to go with DSCP based on port as the initial functionality, your
changes look good. A couple of nits below, and the first patch (which I
have not looked at) needs a rebase after the removal of incremental
processing.
However, I think we should be more ambitious and support arbitrary match
criteria. I am always worried about the migration impact when moving from
one way of specifying a feature in NB schema (in this case, port options
"qos_dscp") to something completely different (see below) later on.
During the OVN meeting this morning, there was a preference for creating
separate tables for each feature such as ACLs, QoS marking, SFC insertion,
rather than overloading ACLs. It was noted that tables are fairly cheap,
and each one can be customized to the purpose.
So I am proposing my earlier suggestion for ovn/ovn-nb.ovsschema once again:
@@ -26,6 +26,11 @@
"refType": "strong"},
"min": 0,
"max": "unlimited"}},
+ "qos_rules": {"type": {"key": {"type": "uuid",
+ "refTable": "QoS",
+ "refType": "strong"},
+ "min": 0,
+ "max": "unlimited"}},
"load_balancer": {"type": {"key": {"type": "uuid",
"refTable":
"Load_Balancer",
"refType": "strong"},
@@ -118,6 +123,23 @@
"type": {"key": "string", "value": "string",
"min": 0, "max": "unlimited"}}},
"isRoot": false},
+ "QoS": {
+ "columns": {
+ "priority": {"type": {"key": {"type": "integer",
+ "minInteger": 0,
+ "maxInteger": 32767}}},
+ "direction": {"type": {"key": {"type": "string",
+ "enum": ["set", ["from-lport",
"to-lport"]]}}},
+ "match": {"type": "string"},
+ "action": {"type": {"key": {"type": "string",
+ "enum": ["set", ["dscp"]]},
+ "value": {"type": "integer",
+ "minInteger": 0,
+ "maxInteger": 63}}},
+ "external_ids": {
+ "type": {"key": "string", "value": "string",
+ "min": 0, "max": "unlimited"}}},
+ "isRoot": false},
"Logical_Router": {
"columns": {
"name": {"type": "string"},
Any opinions from others whether we should stick with the patch as is or
implement arbitrary match criteria?
I don't think arbitrary match criteria requires a lot of code, though it
would benefit from new nbctl commands, based on the existing acl commands.
I am willing to help out if you wish. Note that the ovn/ovn-nb.ovsschema
proposal above is dependent on an IDL fix to overcome a build error. I have
not submitted that fix yet but I will raise it very soon.
> Signed-off-by: Babu Shanmugam <bschanmu at redhat.com>
> ---
> ovn/lib/logical-fields.c | 2 +-
> ovn/northd/ovn-northd.8.xml | 30 +++++++++++++++----
> ovn/northd/ovn-northd.c | 69 ++++++++++++++++++++++++++----
> ------------
> ovn/ovn-nb.xml | 6 ++++
> ovn/ovn-sb.xml | 5 ++++
> tests/ovn.at | 73 ++++++++++++++++++++++++++++++
> +++++++++++++++
> 6 files changed, 152 insertions(+), 33 deletions(-)
>
> diff --git a/ovn/lib/logical-fields.c b/ovn/lib/logical-fields.c
> index 6dbb4ae..068c000 100644
> --- a/ovn/lib/logical-fields.c
> +++ b/ovn/lib/logical-fields.c
> @@ -134,7 +134,7 @@ ovn_init_symtab(struct shash *symtab)
> expr_symtab_add_predicate(symtab, "ip6", "eth.type == 0x86dd");
> expr_symtab_add_predicate(symtab, "ip", "ip4 || ip6");
> expr_symtab_add_field(symtab, "ip.proto", MFF_IP_PROTO, "ip", true);
> - expr_symtab_add_field(symtab, "ip.dscp", MFF_IP_DSCP, "ip", false);
> + expr_symtab_add_field(symtab, "ip.dscp", MFF_IP_DSCP_SHIFTED, "ip",
> false);
> expr_symtab_add_field(symtab, "ip.ecn", MFF_IP_ECN, "ip", false);
> expr_symtab_add_field(symtab, "ip.ttl", MFF_IP_TTL, "ip", false);
>
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index 3448370..f142871 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> @@ -362,7 +362,25 @@
> </li>
> </ul>
>
> - <h3>Ingress Table 7: LB</h3>
> + <h3>Ingress Table 7: Ingress Port DSCP</h3>
> +
> + <p>
> + Ingress table 7 contains these logical flows:
> + </p>
> +
> + <ul>
> + <li>
> + For every port with a DSCP setting, one priority-100 flow that
> matches
> + the <code>inport</code> on the corresponding switch and sets DSCP.
> + </li>
> +
> + <li>
> + One priority-0 fallback flow that matches all packets and
> advances to
> + the next table.
> + </li>
> + </ul>
> +
> + <h3>Ingress Table 8: LB</h3>
>
> <p>
> It contains a priority-0 flow that simply moves traffic to the next
> @@ -375,7 +393,7 @@
> connection.)
> </p>
>
> - <h3>Ingress Table 8: Stateful</h3>
> + <h3>Ingress Table 9: Stateful</h3>
>
> <ul>
> <li>
> @@ -412,7 +430,7 @@
> </li>
> </ul>
>
> - <h3>Ingress Table 9: ARP/ND responder</h3>
> + <h3>Ingress Table 10: ARP/ND responder</h3>
>
> <p>
> This table implements ARP/ND responder for known IPs. It contains
> these
> @@ -484,7 +502,7 @@ nd_na {
> </li>
> </ul>
>
> - <h3>Ingress Table 10: DHCP option processing</h3>
> + <h3>Ingress Table 11: DHCP option processing</h3>
>
> <p>
> This table adds the DHCPv4 options to a DHCPv4 packet from the
> @@ -544,7 +562,7 @@ next;
> </li>
> </ul>
>
> - <h3>Ingress Table 11: DHCP responses</h3>
> + <h3>Ingress Table 12: DHCP responses</h3>
>
> <p>
> This table implements DHCP responder for the DHCP replies generated
> by
> @@ -626,7 +644,7 @@ output;
> </li>
> </ul>
>
> - <h3>Ingress Table 12: Destination Lookup</h3>
> + <h3>Ingress Table 13 Destination Lookup</h3>
^^^:^^^
> <p>
> This table implements switching behavior. It contains these logical
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index d7d61bf..f0b1bb7 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -93,21 +93,22 @@ enum ovn_datapath_type {
> * form the stage's full name, e.g. S_SWITCH_IN_PORT_SEC_L2,
> * S_ROUTER_OUT_DELIVERY. */
> enum ovn_stage {
> -#define PIPELINE_STAGES \
> - /* Logical switch ingress stages. */ \
> - PIPELINE_STAGE(SWITCH, IN, PORT_SEC_L2, 0, "ls_in_port_sec_l2")
> \
> - PIPELINE_STAGE(SWITCH, IN, PORT_SEC_IP, 1, "ls_in_port_sec_ip")
> \
> - PIPELINE_STAGE(SWITCH, IN, PORT_SEC_ND, 2, "ls_in_port_sec_nd")
> \
> - PIPELINE_STAGE(SWITCH, IN, PRE_ACL, 3, "ls_in_pre_acl") \
> - PIPELINE_STAGE(SWITCH, IN, PRE_LB, 4, "ls_in_pre_lb")
> \
> - PIPELINE_STAGE(SWITCH, IN, PRE_STATEFUL, 5, "ls_in_pre_stateful")
> \
> - PIPELINE_STAGE(SWITCH, IN, ACL, 6, "ls_in_acl") \
> - PIPELINE_STAGE(SWITCH, IN, LB, 7, "ls_in_lb") \
> - PIPELINE_STAGE(SWITCH, IN, STATEFUL, 8, "ls_in_stateful") \
> - PIPELINE_STAGE(SWITCH, IN, ARP_ND_RSP, 9, "ls_in_arp_rsp") \
> - PIPELINE_STAGE(SWITCH, IN, DHCP_OPTIONS, 10, "ls_in_dhcp_options")
> \
> - PIPELINE_STAGE(SWITCH, IN, DHCP_RESPONSE, 11,
> "ls_in_dhcp_response") \
> - PIPELINE_STAGE(SWITCH, IN, L2_LKUP, 12, "ls_in_l2_lkup")
> \
> +#define PIPELINE_STAGES
> \
> + /* Logical switch ingress stages. */
> \
> + PIPELINE_STAGE(SWITCH, IN, PORT_SEC_L2, 0, "ls_in_port_sec_l2")
> \
> + PIPELINE_STAGE(SWITCH, IN, PORT_SEC_IP, 1, "ls_in_port_sec_ip")
> \
> + PIPELINE_STAGE(SWITCH, IN, PORT_SEC_ND, 2, "ls_in_port_sec_nd")
> \
> + PIPELINE_STAGE(SWITCH, IN, PRE_ACL, 3, "ls_in_pre_acl")
> \
> + PIPELINE_STAGE(SWITCH, IN, PRE_LB, 4, "ls_in_pre_lb")
> \
> + PIPELINE_STAGE(SWITCH, IN, PRE_STATEFUL, 5, "ls_in_pre_stateful")
> \
> + PIPELINE_STAGE(SWITCH, IN, ACL, 6, "ls_in_acl")
> \
> + PIPELINE_STAGE(SWITCH, IN, PORT_DSCP, 7, "ls_in_port_dscp")
> \
>
I believe we should eventually do more than just port-based DSCP.
In addition, if we were to ever implement 802.1p CoS marking, wouldn't
we want to do that in the same pipeline stage?
I suggest the pipeline stage name be either "QOS" or "QOS_MARKING".
> + PIPELINE_STAGE(SWITCH, IN, LB, 8, "ls_in_lb")
> \
> + PIPELINE_STAGE(SWITCH, IN, STATEFUL, 9, "ls_in_stateful")
> \
> + PIPELINE_STAGE(SWITCH, IN, ARP_ND_RSP, 10, "ls_in_arp_rsp")
> \
> + PIPELINE_STAGE(SWITCH, IN, DHCP_OPTIONS, 11, "ls_in_dhcp_options")
> \
> + PIPELINE_STAGE(SWITCH, IN, DHCP_RESPONSE, 12, "ls_in_dhcp_response")
> \
> + PIPELINE_STAGE(SWITCH, IN, L2_LKUP, 13, "ls_in_l2_lkup")
> \
>
Mickey
More information about the dev
mailing list