Re: [ovs-dev] [PATCH] nsh: rework NSH netlink keys and actions

[Jan Scheurich]

> Jan, MD2 is flexible enough, so we mustn't worry there will be one MDx, it
> is very weird to use OVS_NSH_PUSH_ATTR_CONTEXT as sub key of
> OVS_KEY_ATTR_NSH.

There is no point in introducing OVS_NSH_KEY_ATTR_MD2 as we will never transfer 
MD2 TLV context headers as explicit fields neither as for matching in 
OVS_KEY_ATTR_NSH or for setting as part of OVS_ACTION_ATTR_SET/SET_MASKED. MD2 
TLVs will be mapped to some new generic TLV match fields.

> For PUSH_NSH, the below format is clear enough, it won't make people
> confused. Yes, maybe we won't use OVS_NSH_KEY_ATTR_MD2 to transfer
> MD2 match fields, but this isn't a good reason why we want to use a worse
> name.
> 
> OVS_ACTION_ATTR_PUSH_NSH
> -- OVS_NSH_KEY_ATTR_BASE
> -- OVS_NSH_KEY_ATTR_MD1
> 
> Or
> 
> OVS_ACTION_ATTR_PUSH_NSH
> -- OVS_NSH_KEY_ATTR_BASE
> -- OVS_NSH_KEY_ATTR_MD2
> 

This would be a possible alternative solution, but it, for no gain, implies 
more complex code both on sender and receiver. It will also require explicit 
changes on the uAPI and datapath code if ever a new MD type is introduced. I 
don't think that is very far-fetched: One could for example consider MD3 to 
combine the simplicity of MD1 fixed contexts (accessible to HW SFFs) with the 
flexibility of MD2 TLVs (mainly for SFs).

If we should anyway stick to your approach, you should definitely rename 
OVS_NSH_KEY_ATTR_MD2 to OVS_NSH_PUSH_ATTR_MD2.

For comparison again: In my simple and generic approach push_nsh will always be 
represented as 
OVS_ACTION_ATTR_PUSH_NSH
-- OVS_NSH_KEY_ATTR_BASE   (mandatory)
-- OVS_NSH_PUSH_ATTR_CONTEXT   (optional)

> I have worked out a version with ttl key and aligned to the lasted NSH draft,
> I have double confirmed from its author, this will be final version, NSH
> header format won't be changed anymore except some text changes, I'll
> send out this version today to catch up with OVS guys' review, kernel
> version will be sent out later today.

That is good news. I am looking forward to seeing the changed version.

BR, Jan

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] nsh: rework NSH netlink keys and actions

[Jan Scheurich]

> > As discussed please adjust the netlink uAPI for NSH as follows:
> >
> > OVS_KEY_ATTR_NSH
> > -- OVS_NSH_KEY_ATTR_BASEmandatory
> > -- OVS_NSH_KEY_ATTR_MD1 conditional: if mdtype=MD1
> >
> > OVS_ACTION_ATTR_PUSH_NSH
> > -- OVS_NSH_KEY_ATTR_BASEmandatory
> > -- OVS_NSH_PUSH_ATTR_CONTEXTconditional: currently if MD1 or
> > MD2 with TLV encap properties
> >
> > OVS_ACTION_ATTR_POP_NSH no data
> >
> > Remove OVS_NSH_KEY_ATTR_MD2 because the MD2 context headers
> will not
> > be modelled as dedicated key fields but mapped to some generic TLV
> fields,
> > similar to but independent from the current tunnel metadata TLV
> registers.
> >
> > Using the name OVS_NSH_KEY_ATTR_MD2 for the opaque context data
> in
> > PUSH_NSH would be misleading as the variable length byte array can
> carry
> > any type of metadata format: MD1, MD2, or any future MD type. For
> that
> > reason I prefer the generic name OVS_NSH_PUSH_ATTR_CONTEXT.
> 
> Then why even have ATTR_MD1? Sounds a single attribute is enough to
> support all cases.

OVS_NSH_KEY_ATTR_MD1 represents a struct containing the four fixed length 
context headers defined in MD1. These are match fields in OVS and must be 
represented explicitly in OVS_KEY_ATTR_NSH. But their presence is conditional 
on MD type 1, that's why Jiri rightly insisted on having it split from 
OVS_NSH_KEY_ATTR_BASE.

In OVS_ACTION_ATTR_PUSH_NSH we again have the same mandatory header part 
represented by OVS_NSH_KEY_ATTR_BASE, this time followed by an optional 
variable length opaque container for up to 248 octets of context data (the 
format is of no concern for the push_nsh action). The content of the 
ATTR_CONTEXT, if any, is compiled by ofproto when the action is sent.

BR, Jan
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] nsh: rework NSH netlink keys and actions

[Eric Garver]

On Thu, Aug 17, 2017 at 12:04:25PM +, Jan Scheurich wrote:
> Hi Yi,
> 
> As discussed please adjust the netlink uAPI for NSH as follows:
> 
> OVS_KEY_ATTR_NSH
> -- OVS_NSH_KEY_ATTR_BASE  mandatory
> -- OVS_NSH_KEY_ATTR_MD1   conditional: if mdtype=MD1
> 
> OVS_ACTION_ATTR_PUSH_NSH
> -- OVS_NSH_KEY_ATTR_BASE  mandatory
> -- OVS_NSH_PUSH_ATTR_CONTEXT  conditional: currently if MD1 or 
>   MD2 with TLV encap properties
> 
> OVS_ACTION_ATTR_POP_NSH   no data
> 
> Remove OVS_NSH_KEY_ATTR_MD2 because the MD2 context headers will not 
> be modelled as dedicated key fields but mapped to some generic TLV fields, 
> similar to but independent from the current tunnel metadata TLV registers.
> 
> Using the name OVS_NSH_KEY_ATTR_MD2 for the opaque context data in 
> PUSH_NSH would be misleading as the variable length byte array can carry
> any type of metadata format: MD1, MD2, or any future MD type. For that
> reason I prefer the generic name OVS_NSH_PUSH_ATTR_CONTEXT. 

Then why even have ATTR_MD1? Sounds a single attribute is enough to
support all cases.

> 
> Note that this attribute will not be used as part of the NSH key, so you 
> might 
> consider generalizing the NSH attribute enum definition to 
> 
> enum ovs_nsh_attr {
>   OVS_NSH_KEY_ATTR_BASE,  /* struct ovs_nsh_key_base. */
>   OVS_NSH_KEY_ATTR_MD1,   /* struct ovs_nsh_key_md1. */
>   OVS_NSH_PUSH_ATTR_CONTEXT,   /* opaque context headers */
>   __OVS_NSH_ATTR_MAX
> };
> 
> BR, Jan
> 
> 
> > -Original Message-
> > From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> > boun...@openvswitch.org] On Behalf Of Yi Yang
> > Sent: Wednesday, 16 August, 2017 09:56
> > To: d...@openvswitch.org
> > Subject: [ovs-dev] [PATCH] nsh: rework NSH netlink keys and actions
> > 
> > Per kernel data path requirements, this patch changes
> > OVS_KEY_ATTR_NSH
> > to nested attribute and adds three new NSH sub attribute keys:
> > 
> > OVS_NSH_KEY_ATTR_BASE: for length-fixed NSH base header
> > OVS_NSH_KEY_ATTR_MD1:  for length-fixed MD type 1 context
> > OVS_NSH_KEY_ATTR_MD2:  for length-variable MD type 2 metadata
> > 
> > NSH match fields, set and PUSH_NSH action all use the below
> > nested attribute format:
> > 
> > OVS_KEY_ATTR_NSH begin
> > OVS_NSH_KEY_ATTR_BASE
> > OVS_NSH_KEY_ATTR_MD1
> > OVS_KEY_ATTR_NSH end
> > 
> > or
> > 
> > OVS_KEY_ATTR_NSH begin
> > OVS_NSH_KEY_ATTR_BASE
> > OVS_NSH_KEY_ATTR_MD2
> > OVS_KEY_ATTR_NSH end
> > 
> > In addition, NSH encap and decap actions are renamed as push_nsh
> > and pop_nsh to meet action naming convention.
> > 
> > Signed-off-by: Yi Yang <yi.y.y...@intel.com>
> > ---
> >  datapath/linux/compat/include/linux/openvswitch.h |  57 +-
> >  include/openvswitch/nsh.h |  32 +-
> >  include/openvswitch/packets.h |  11 +-
> >  lib/dpif-netdev.c |   4 +-
> >  lib/dpif.c|   4 +-
> >  lib/flow.c|  15 +-
> >  lib/match.c   |  12 +-
> >  lib/meta-flow.c   |  13 +-
> >  lib/nx-match.c|   4 +-
> >  lib/odp-execute.c |  76 ++-
> >  lib/odp-util.c| 713 
> > ++
> >  lib/odp-util.h|   4 +
> >  lib/packets.c |  24 +-
> >  lib/packets.h |   6 +-
> >  ofproto/ofproto-dpif-ipfix.c  |   4 +-
> >  ofproto/ofproto-dpif-sflow.c  |   4 +-
> >  ofproto/ofproto-dpif-xlate.c  |  24 +-
> >  tests/nsh.at  |  28 +-
> >  18 files changed, 773 insertions(+), 262 deletions(-)
> > 
> > diff --git a/datapath/linux/compat/include/linux/openvswitch.h
> > b/datapath/linux/compat/include/linux/openvswitch.h
> > index bc6c94b..d7f9029 100644
> > --- a/datapath/linux/compat/include/linux/openvswitch.h
> > +++ b/datapath/linux/compat/include/linux/openvswitch.h
> > @@ -369,7 +369,7 @@ enum ovs_key_attr {
> >  #ifndef __KERNEL__
> > /* Only used within userspace data path. */
> > OVS_KEY_ATTR_PACKET_TYPE,  /* be32 packet type */
> > -   OVS_KEY_ATTR_NSH,  /*

Re: [ovs-dev] [PATCH] nsh: rework NSH netlink keys and actions

[Jan Scheurich]

Hi Yi,

As discussed please adjust the netlink uAPI for NSH as follows:

OVS_KEY_ATTR_NSH
-- OVS_NSH_KEY_ATTR_BASEmandatory
-- OVS_NSH_KEY_ATTR_MD1 conditional: if mdtype=MD1

OVS_ACTION_ATTR_PUSH_NSH
-- OVS_NSH_KEY_ATTR_BASEmandatory
-- OVS_NSH_PUSH_ATTR_CONTEXTconditional: currently if MD1 or 
MD2 with TLV encap properties

OVS_ACTION_ATTR_POP_NSH no data

Remove OVS_NSH_KEY_ATTR_MD2 because the MD2 context headers will not 
be modelled as dedicated key fields but mapped to some generic TLV fields, 
similar to but independent from the current tunnel metadata TLV registers.

Using the name OVS_NSH_KEY_ATTR_MD2 for the opaque context data in 
PUSH_NSH would be misleading as the variable length byte array can carry
any type of metadata format: MD1, MD2, or any future MD type. For that
reason I prefer the generic name OVS_NSH_PUSH_ATTR_CONTEXT. 

Note that this attribute will not be used as part of the NSH key, so you might 
consider generalizing the NSH attribute enum definition to 

enum ovs_nsh_attr {
OVS_NSH_KEY_ATTR_BASE,  /* struct ovs_nsh_key_base. */
OVS_NSH_KEY_ATTR_MD1,   /* struct ovs_nsh_key_md1. */
OVS_NSH_PUSH_ATTR_CONTEXT,   /* opaque context headers */
__OVS_NSH_ATTR_MAX
};

BR, Jan


> -Original Message-
> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Yi Yang
> Sent: Wednesday, 16 August, 2017 09:56
> To: d...@openvswitch.org
> Subject: [ovs-dev] [PATCH] nsh: rework NSH netlink keys and actions
> 
> Per kernel data path requirements, this patch changes
> OVS_KEY_ATTR_NSH
> to nested attribute and adds three new NSH sub attribute keys:
> 
> OVS_NSH_KEY_ATTR_BASE: for length-fixed NSH base header
> OVS_NSH_KEY_ATTR_MD1:  for length-fixed MD type 1 context
> OVS_NSH_KEY_ATTR_MD2:  for length-variable MD type 2 metadata
> 
> NSH match fields, set and PUSH_NSH action all use the below
> nested attribute format:
> 
> OVS_KEY_ATTR_NSH begin
> OVS_NSH_KEY_ATTR_BASE
> OVS_NSH_KEY_ATTR_MD1
> OVS_KEY_ATTR_NSH end
> 
> or
> 
> OVS_KEY_ATTR_NSH begin
> OVS_NSH_KEY_ATTR_BASE
> OVS_NSH_KEY_ATTR_MD2
> OVS_KEY_ATTR_NSH end
> 
> In addition, NSH encap and decap actions are renamed as push_nsh
> and pop_nsh to meet action naming convention.
> 
> Signed-off-by: Yi Yang <yi.y.y...@intel.com>
> ---
>  datapath/linux/compat/include/linux/openvswitch.h |  57 +-
>  include/openvswitch/nsh.h |  32 +-
>  include/openvswitch/packets.h |  11 +-
>  lib/dpif-netdev.c |   4 +-
>  lib/dpif.c|   4 +-
>  lib/flow.c|  15 +-
>  lib/match.c   |  12 +-
>  lib/meta-flow.c   |  13 +-
>  lib/nx-match.c|   4 +-
>  lib/odp-execute.c |  76 ++-
>  lib/odp-util.c| 713 
> ++
>  lib/odp-util.h|   4 +
>  lib/packets.c |  24 +-
>  lib/packets.h |   6 +-
>  ofproto/ofproto-dpif-ipfix.c  |   4 +-
>  ofproto/ofproto-dpif-sflow.c  |   4 +-
>  ofproto/ofproto-dpif-xlate.c  |  24 +-
>  tests/nsh.at  |  28 +-
>  18 files changed, 773 insertions(+), 262 deletions(-)
> 
> diff --git a/datapath/linux/compat/include/linux/openvswitch.h
> b/datapath/linux/compat/include/linux/openvswitch.h
> index bc6c94b..d7f9029 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -369,7 +369,7 @@ enum ovs_key_attr {
>  #ifndef __KERNEL__
>   /* Only used within userspace data path. */
>   OVS_KEY_ATTR_PACKET_TYPE,  /* be32 packet type */
> - OVS_KEY_ATTR_NSH,  /* struct ovs_key_nsh */
> + OVS_KEY_ATTR_NSH,  /* Nested set of ovs_nsh_key_* */
>  #endif
> 
>   __OVS_KEY_ATTR_MAX
> @@ -492,13 +492,27 @@ struct ovs_key_ct_labels {
>   };
>  };
> 
> -struct ovs_key_nsh {
> -__u8 flags;
> -__u8 mdtype;
> -__u8 np;
> -__u8 pad;
> -__be32 path_hdr;
> -__be32 c[4];
> +enum ovs_nsh_key_attr {
> + OVS_NSH_KEY_ATTR_BASE,  /* struct ovs_nsh_key_base. */
> + OVS_NSH_KEY_ATTR_MD1,   /* struct ovs_nsh_key_md1. */
> + OVS_NSH_KEY_ATTR_MD2,   /* variable-length octets. */
> + __OVS_N

[ovs-dev] [PATCH] nsh: rework NSH netlink keys and actions

[Yi Yang]

Per kernel data path requirements, this patch changes OVS_KEY_ATTR_NSH
to nested attribute and adds three new NSH sub attribute keys:

OVS_NSH_KEY_ATTR_BASE: for length-fixed NSH base header
OVS_NSH_KEY_ATTR_MD1:  for length-fixed MD type 1 context
OVS_NSH_KEY_ATTR_MD2:  for length-variable MD type 2 metadata

NSH match fields, set and PUSH_NSH action all use the below
nested attribute format:

OVS_KEY_ATTR_NSH begin
OVS_NSH_KEY_ATTR_BASE
OVS_NSH_KEY_ATTR_MD1
OVS_KEY_ATTR_NSH end

or

OVS_KEY_ATTR_NSH begin
OVS_NSH_KEY_ATTR_BASE
OVS_NSH_KEY_ATTR_MD2
OVS_KEY_ATTR_NSH end

In addition, NSH encap and decap actions are renamed as push_nsh
and pop_nsh to meet action naming convention.

Signed-off-by: Yi Yang 
---
 datapath/linux/compat/include/linux/openvswitch.h |  57 +-
 include/openvswitch/nsh.h |  32 +-
 include/openvswitch/packets.h |  11 +-
 lib/dpif-netdev.c |   4 +-
 lib/dpif.c|   4 +-
 lib/flow.c|  15 +-
 lib/match.c   |  12 +-
 lib/meta-flow.c   |  13 +-
 lib/nx-match.c|   4 +-
 lib/odp-execute.c |  76 ++-
 lib/odp-util.c| 713 ++
 lib/odp-util.h|   4 +
 lib/packets.c |  24 +-
 lib/packets.h |   6 +-
 ofproto/ofproto-dpif-ipfix.c  |   4 +-
 ofproto/ofproto-dpif-sflow.c  |   4 +-
 ofproto/ofproto-dpif-xlate.c  |  24 +-
 tests/nsh.at  |  28 +-
 18 files changed, 773 insertions(+), 262 deletions(-)

diff --git a/datapath/linux/compat/include/linux/openvswitch.h 
b/datapath/linux/compat/include/linux/openvswitch.h
index bc6c94b..d7f9029 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -369,7 +369,7 @@ enum ovs_key_attr {
 #ifndef __KERNEL__
/* Only used within userspace data path. */
OVS_KEY_ATTR_PACKET_TYPE,  /* be32 packet type */
-   OVS_KEY_ATTR_NSH,  /* struct ovs_key_nsh */
+   OVS_KEY_ATTR_NSH,  /* Nested set of ovs_nsh_key_* */
 #endif
 
__OVS_KEY_ATTR_MAX
@@ -492,13 +492,27 @@ struct ovs_key_ct_labels {
};
 };
 
-struct ovs_key_nsh {
-__u8 flags;
-__u8 mdtype;
-__u8 np;
-__u8 pad;
-__be32 path_hdr;
-__be32 c[4];
+enum ovs_nsh_key_attr {
+   OVS_NSH_KEY_ATTR_BASE,  /* struct ovs_nsh_key_base. */
+   OVS_NSH_KEY_ATTR_MD1,   /* struct ovs_nsh_key_md1. */
+   OVS_NSH_KEY_ATTR_MD2,   /* variable-length octets. */
+   __OVS_NSH_KEY_ATTR_MAX
+};
+
+#define OVS_NSH_KEY_ATTR_MAX (__OVS_NSH_KEY_ATTR_MAX - 1)
+
+struct ovs_nsh_key_base {
+   __u8 flags;
+   __u8 mdtype;
+   __u8 np;
+   __u8 pad;
+   __be32 path_hdr;
+};
+
+#define NSH_MD1_CONTEXT_SIZE 4
+
+struct ovs_nsh_key_md1 {
+   __be32 context[NSH_MD1_CONTEXT_SIZE];
 };
 
 /* OVS_KEY_ATTR_CT_STATE flags */
@@ -793,24 +807,7 @@ struct ovs_action_push_eth {
struct ovs_key_ethernet addresses;
 };
 
-#define OVS_ENCAP_NSH_MAX_MD_LEN 16
-/*
- * struct ovs_action_encap_nsh - %OVS_ACTION_ATTR_ENCAP_NSH
- * @flags: NSH header flags.
- * @mdtype: NSH metadata type.
- * @mdlen: Length of NSH metadata in bytes.
- * @np: NSH next_protocol: Inner packet type.
- * @path_hdr: NSH service path id and service index.
- * @metadata: NSH metadata for MD type 1 or 2
- */
-struct ovs_action_encap_nsh {
-uint8_t flags;
-uint8_t mdtype;
-uint8_t mdlen;
-uint8_t np;
-__be32 path_hdr;
-uint8_t metadata[OVS_ENCAP_NSH_MAX_MD_LEN];
-};
+#define OVS_PUSH_NSH_MAX_MD_LEN 248
 
 /**
  * enum ovs_nat_attr - Attributes for %OVS_CT_ATTR_NAT.
@@ -887,8 +884,8 @@ enum ovs_nat_attr {
  * @OVS_ACTION_ATTR_PUSH_ETH: Push a new outermost Ethernet header onto the
  * packet.
  * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the packet.
- * @OVS_ACTION_ATTR_ENCAP_NSH: encap NSH action to push NSH header.
- * @OVS_ACTION_ATTR_DECAP_NSH: decap NSH action to remove NSH header.
+ * @OVS_ACTION_ATTR_PUSH_NSH: push NSH header to the packet.
+ * @OVS_ACTION_ATTR_POP_NSH: pop the outermost NSH header off the packet.
  *
  * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not all
  * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
@@ -930,8 +927,8 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_TUNNEL_POP,/* u32 port number. */
OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*.  */
OVS_ACTION_ATTR_METER, /* u32 meter number. */
-   OVS_ACTION_ATTR_ENCAP_NSH,