Re: [ovs-dev] [PATCH v5 1/2] OF support and translation of generic encap and decap

[Jan Scheurich]

If this is just about ovs-ofctl action syntax, I guess that is a matter of 
taste. In my eyes there is a value in keeping a closer correspondence between 
the OpenFlow actions and the action syntax to avoid confusion for users, even 
if the resulting syntax looks slightly less elegant. ovs-ofctl dump-flows is 
very frequently used by controller developers testing their code against OVS.

If I understand it right, the suggested change would also imply creating 
specific abstracted encap_eth, encap_nsh, etc. actions in the OFPACTS macro in 
ofp-actions.h, with a significant amount of rework (or at least reorganization) 
of code in many places (ofp-actions.c, ofproto-dpif-xlate.c, etc.)

Seems like it's quite late to do this at this late stage. But I guess it is 
your decision.

BR, Jan

> -Original Message-
> From: Ben Pfaff [mailto:b...@ovn.org]
> Sent: Thursday, 10 August, 2017 20:17
> To: Jan Scheurich 
> Cc: Yi Yang ; d...@openvswitch.org; Zoltán Balogh 
> 
> Subject: Re: [PATCH v5 1/2] OF support and translation of generic encap and 
> decap
> 
> I'm inclined to make the action name specific to the header,
> e.g. encap_nsh, decap_nsh.  There doesn't have to be a one-to-one
> correspondence between syntax and OpenFlow encoding.
> 
> On Thu, Aug 10, 2017 at 03:25:20PM +, Jan Scheurich wrote:
> > The generic code today in function parse_ENCAP() uses the encap header 
> > string also as string for the property class. I am afraid that
> implementer of subsequent encap headers might not realize that this is a 
> temporary implementation shortcut that should have been
> generalized.
> >
> > The minimum we should add now is a comment explaining this, such as, for 
> > example:
> >
> > diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> > index bfc8a80..84522e0 100644
> > --- a/lib/ofp-actions.c
> > +++ b/lib/ofp-actions.c
> > @@ -4134,9 +4134,19 @@ parse_ed_props(const uint16_t prop_class, char 
> > **arg, int *n_props, struct ofpbu
> >  return NULL;
> >  }
> >
> > -/* The string representation of the encap action is
> > - * encap(header_type(prop=,tlv(,,),...))
> > - */
> > +/* The generic string representation of the encap action is
> > + * encap()
> > + * encap((=,(),...))
> > + *
> > + * TODO: The current implementation only supports the simple case that all
> > + * encap parameters for a given encap header type are in a single property
> > + * class, identified by the same keyword as the encap header type.
> > + * To represent different property classes allowed by the OF action, the
> > + * syntax should be generalized as follows:
> > + *
> > + * encap((=,(),...),
> > + *   (=,(),...),...)
> > +*/
> >
> >  static char * OVS_WARN_UNUSED_RESULT
> >  parse_ENCAP(char *arg,
> >
> > BR, Jan
> >
> > > -Original Message-
> > > From: Ben Pfaff [mailto:b...@ovn.org]
> > > Sent: Tuesday, 08 August, 2017 21:36
> > > To: Jan Scheurich 
> > > Cc: Yi Yang ; d...@openvswitch.org; Zoltán Balogh 
> > > 
> > > Subject: Re: [PATCH v5 1/2] OF support and translation of generic encap 
> > > and decap
> > >
> > > We can add the additional syntax at the same time we add something that
> > > has the need for it.
> > >
> > > On Tue, Aug 08, 2017 at 03:52:32PM +, Jan Scheurich wrote:
> > > > I know this comment is late as the patch has been merged already, but I 
> > > > just returned from vacation and only found today:
> > > >
> > > > I agree that the new simplified ovs-ofctl syntax for encap properties 
> > > > looks quite neat for the NSH use case at hand:
> > > >
> > > > encap(nsh(md_type=1))
> > > > encap(nsh(md_type=2,tlv(0x1000,10,0x12345678),tlv(0x2000,20,0xfedcba9876543210)))
> > > >
> > > > But unfortunately we have fallen into the trap of oversimplifying. The 
> > > > generic encap action (drafted in EXT-382) distinguishes
> between
> > > the encapsulation packet type (in this case (1,0x894f) for NSH) and the 
> > > encap property class (in this case 0x4 for NSH), which may be
> > > different for every property in an encap action.
> > > >
> > > > Using the "nsh" keyword both as shorthand for the packet type and the 
> > > > property class selector works here because the NSH encap
> action
> > > only supports NSH properties so far, and in many simple applications of 
> > > the generic encap action there may well be 1-1 relation
> between
> > > the encap packet type and the class of all supported properties. But the 
> > > ONF draft specifically decouples the packet type from property
> > > classes for a richer set of use cases.
> > > >
> > > > For example there could be a more powerful generic encap operation to 
> > > > add multiple protocol layers in one action. In this case the
> > > encap properties would likely belong to several property classes. Or an 
> > > encapsulation header re-uses a general encap property
> already
> > > 

Re: [ovs-dev] [PATCH v5 1/2] OF support and translation of generic encap and decap

[Ben Pfaff]

I'm inclined to make the action name specific to the header,
e.g. encap_nsh, decap_nsh.  There doesn't have to be a one-to-one
correspondence between syntax and OpenFlow encoding.

On Thu, Aug 10, 2017 at 03:25:20PM +, Jan Scheurich wrote:
> The generic code today in function parse_ENCAP() uses the encap header string 
> also as string for the property class. I am afraid that implementer of 
> subsequent encap headers might not realize that this is a temporary 
> implementation shortcut that should have been generalized.
> 
> The minimum we should add now is a comment explaining this, such as, for 
> example:
> 
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index bfc8a80..84522e0 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -4134,9 +4134,19 @@ parse_ed_props(const uint16_t prop_class, char **arg, 
> int *n_props, struct ofpbu
>  return NULL;
>  }
> 
> -/* The string representation of the encap action is
> - * encap(header_type(prop=,tlv(,,),...))
> - */
> +/* The generic string representation of the encap action is
> + * encap()
> + * encap((=,(),...))
> + *
> + * TODO: The current implementation only supports the simple case that all
> + * encap parameters for a given encap header type are in a single property
> + * class, identified by the same keyword as the encap header type.
> + * To represent different property classes allowed by the OF action, the
> + * syntax should be generalized as follows:
> + *
> + * encap((=,(),...),
> + *   (=,(),...),...)
> +*/
> 
>  static char * OVS_WARN_UNUSED_RESULT
>  parse_ENCAP(char *arg,
> 
> BR, Jan
> 
> > -Original Message-
> > From: Ben Pfaff [mailto:b...@ovn.org]
> > Sent: Tuesday, 08 August, 2017 21:36
> > To: Jan Scheurich 
> > Cc: Yi Yang ; d...@openvswitch.org; Zoltán Balogh 
> > 
> > Subject: Re: [PATCH v5 1/2] OF support and translation of generic encap and 
> > decap
> > 
> > We can add the additional syntax at the same time we add something that
> > has the need for it.
> > 
> > On Tue, Aug 08, 2017 at 03:52:32PM +, Jan Scheurich wrote:
> > > I know this comment is late as the patch has been merged already, but I 
> > > just returned from vacation and only found today:
> > >
> > > I agree that the new simplified ovs-ofctl syntax for encap properties 
> > > looks quite neat for the NSH use case at hand:
> > >
> > > encap(nsh(md_type=1))
> > > encap(nsh(md_type=2,tlv(0x1000,10,0x12345678),tlv(0x2000,20,0xfedcba9876543210)))
> > >
> > > But unfortunately we have fallen into the trap of oversimplifying. The 
> > > generic encap action (drafted in EXT-382) distinguishes between
> > the encapsulation packet type (in this case (1,0x894f) for NSH) and the 
> > encap property class (in this case 0x4 for NSH), which may be
> > different for every property in an encap action.
> > >
> > > Using the "nsh" keyword both as shorthand for the packet type and the 
> > > property class selector works here because the NSH encap action
> > only supports NSH properties so far, and in many simple applications of the 
> > generic encap action there may well be 1-1 relation between
> > the encap packet type and the class of all supported properties. But the 
> > ONF draft specifically decouples the packet type from property
> > classes for a richer set of use cases.
> > >
> > > For example there could be a more powerful generic encap operation to add 
> > > multiple protocol layers in one action. In this case the
> > encap properties would likely belong to several property classes. Or an 
> > encapsulation header re-uses a general encap property already
> > defined elsewhere, which should not be duplicated.
> > >
> > > In order not to restrict the generality of the ovs-ofctl syntax, I'd 
> > > suggest to separate packet type and the property class as follows:
> > >
> > > encap()
> > > encap((,(=,()),...)
> > >
> > > For NSH the syntax would look:
> > >
> > > encap(nsh,nsh(md_type=1))
> > > encap(nsh,nsh(md_type=2,tlv(0x1000,10,0x12345678),tlv(0x2000,20,0xfedcba9876543210)))
> > >
> > > To be user friendly we can also allow the above more concise syntax in 
> > > the special case of a 1-1 relation between packet type and
> > property class. In that case we could re-add the generalization of the 
> > syntax after the OVS 2.8 release without braking backward
> > compatibility.
> > >
> > > BR, Jan
> > >
> > > > -Original Message-
> > > > From: Yi Yang [mailto:yi.y.y...@intel.com]
> > > > Sent: Wednesday, 02 August, 2017 10:04
> > > > To: d...@openvswitch.org
> > > > Cc: b...@ovn.org; Jan Scheurich ; Yi Yang 
> > > > ; Zoltán Balogh
> > > > 
> > > > Subject: [PATCH v5 1/2] OF support and translation of generic encap and 
> > > > decap
> > > >
> > > > From: Jan Scheurich 
> > > >
> > > > This commit adds support for the OpenFlow actions generic 

Re: [ovs-dev] [PATCH v5 1/2] OF support and translation of generic encap and decap

[Jan Scheurich]

The generic code today in function parse_ENCAP() uses the encap header string 
also as string for the property class. I am afraid that implementer of 
subsequent encap headers might not realize that this is a temporary 
implementation shortcut that should have been generalized.

The minimum we should add now is a comment explaining this, such as, for 
example:

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index bfc8a80..84522e0 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -4134,9 +4134,19 @@ parse_ed_props(const uint16_t prop_class, char **arg, 
int *n_props, struct ofpbu
 return NULL;
 }

-/* The string representation of the encap action is
- * encap(header_type(prop=,tlv(,,),...))
- */
+/* The generic string representation of the encap action is
+ * encap()
+ * encap((=,(),...))
+ *
+ * TODO: The current implementation only supports the simple case that all
+ * encap parameters for a given encap header type are in a single property
+ * class, identified by the same keyword as the encap header type.
+ * To represent different property classes allowed by the OF action, the
+ * syntax should be generalized as follows:
+ *
+ * encap((=,(),...),
+ *   (=,(),...),...)
+*/

 static char * OVS_WARN_UNUSED_RESULT
 parse_ENCAP(char *arg,

BR, Jan

> -Original Message-
> From: Ben Pfaff [mailto:b...@ovn.org]
> Sent: Tuesday, 08 August, 2017 21:36
> To: Jan Scheurich 
> Cc: Yi Yang ; d...@openvswitch.org; Zoltán Balogh 
> 
> Subject: Re: [PATCH v5 1/2] OF support and translation of generic encap and 
> decap
> 
> We can add the additional syntax at the same time we add something that
> has the need for it.
> 
> On Tue, Aug 08, 2017 at 03:52:32PM +, Jan Scheurich wrote:
> > I know this comment is late as the patch has been merged already, but I 
> > just returned from vacation and only found today:
> >
> > I agree that the new simplified ovs-ofctl syntax for encap properties looks 
> > quite neat for the NSH use case at hand:
> >
> > encap(nsh(md_type=1))
> > encap(nsh(md_type=2,tlv(0x1000,10,0x12345678),tlv(0x2000,20,0xfedcba9876543210)))
> >
> > But unfortunately we have fallen into the trap of oversimplifying. The 
> > generic encap action (drafted in EXT-382) distinguishes between
> the encapsulation packet type (in this case (1,0x894f) for NSH) and the encap 
> property class (in this case 0x4 for NSH), which may be
> different for every property in an encap action.
> >
> > Using the "nsh" keyword both as shorthand for the packet type and the 
> > property class selector works here because the NSH encap action
> only supports NSH properties so far, and in many simple applications of the 
> generic encap action there may well be 1-1 relation between
> the encap packet type and the class of all supported properties. But the ONF 
> draft specifically decouples the packet type from property
> classes for a richer set of use cases.
> >
> > For example there could be a more powerful generic encap operation to add 
> > multiple protocol layers in one action. In this case the
> encap properties would likely belong to several property classes. Or an 
> encapsulation header re-uses a general encap property already
> defined elsewhere, which should not be duplicated.
> >
> > In order not to restrict the generality of the ovs-ofctl syntax, I'd 
> > suggest to separate packet type and the property class as follows:
> >
> > encap()
> > encap((,(=,()),...)
> >
> > For NSH the syntax would look:
> >
> > encap(nsh,nsh(md_type=1))
> > encap(nsh,nsh(md_type=2,tlv(0x1000,10,0x12345678),tlv(0x2000,20,0xfedcba9876543210)))
> >
> > To be user friendly we can also allow the above more concise syntax in the 
> > special case of a 1-1 relation between packet type and
> property class. In that case we could re-add the generalization of the syntax 
> after the OVS 2.8 release without braking backward
> compatibility.
> >
> > BR, Jan
> >
> > > -Original Message-
> > > From: Yi Yang [mailto:yi.y.y...@intel.com]
> > > Sent: Wednesday, 02 August, 2017 10:04
> > > To: d...@openvswitch.org
> > > Cc: b...@ovn.org; Jan Scheurich ; Yi Yang 
> > > ; Zoltán Balogh
> > > 
> > > Subject: [PATCH v5 1/2] OF support and translation of generic encap and 
> > > decap
> > >
> > > From: Jan Scheurich 
> > >
> > > This commit adds support for the OpenFlow actions generic encap
> > > and decap (as specified in ONF EXT-382) to the OVS control plane.
> > >
> > > CLI syntax for encap action with properties:
> > >   encap()
> > >   encap((=,(,,),...))
> > >
> > > For example:
> > >   encap(ethernet)
> > >   encap(nsh(md_type=1))
> > >   
> > > encap(nsh(md_type=2,tlv(0x1000,10,0x12345678),tlv(0x2000,20,0xfedcba9876543210)))
> > >
> > > CLI syntax for decap action:
> > >   decap()
> > >   decap(packet_type(ns=,type=))
> > >
> > > For 

Re: [ovs-dev] [PATCH v5 1/2] OF support and translation of generic encap and decap

[Ben Pfaff]

We can add the additional syntax at the same time we add something that
has the need for it.

On Tue, Aug 08, 2017 at 03:52:32PM +, Jan Scheurich wrote:
> I know this comment is late as the patch has been merged already, but I just 
> returned from vacation and only found today:
> 
> I agree that the new simplified ovs-ofctl syntax for encap properties looks 
> quite neat for the NSH use case at hand:
> 
> encap(nsh(md_type=1))
> encap(nsh(md_type=2,tlv(0x1000,10,0x12345678),tlv(0x2000,20,0xfedcba9876543210)))
> 
> But unfortunately we have fallen into the trap of oversimplifying. The 
> generic encap action (drafted in EXT-382) distinguishes between the 
> encapsulation packet type (in this case (1,0x894f) for NSH) and the encap 
> property class (in this case 0x4 for NSH), which may be different for every 
> property in an encap action.
> 
> Using the "nsh" keyword both as shorthand for the packet type and the 
> property class selector works here because the NSH encap action only supports 
> NSH properties so far, and in many simple applications of the generic encap 
> action there may well be 1-1 relation between the encap packet type and the 
> class of all supported properties. But the ONF draft specifically decouples 
> the packet type from property classes for a richer set of use cases.
> 
> For example there could be a more powerful generic encap operation to add 
> multiple protocol layers in one action. In this case the encap properties 
> would likely belong to several property classes. Or an encapsulation header 
> re-uses a general encap property already defined elsewhere, which should not 
> be duplicated.
> 
> In order not to restrict the generality of the ovs-ofctl syntax, I'd suggest 
> to separate packet type and the property class as follows:
> 
> encap()
> encap((,(=,()),...)
> 
> For NSH the syntax would look:
> 
> encap(nsh,nsh(md_type=1))
> encap(nsh,nsh(md_type=2,tlv(0x1000,10,0x12345678),tlv(0x2000,20,0xfedcba9876543210)))
> 
> To be user friendly we can also allow the above more concise syntax in the 
> special case of a 1-1 relation between packet type and property class. In 
> that case we could re-add the generalization of the syntax after the OVS 2.8 
> release without braking backward compatibility.
> 
> BR, Jan
> 
> > -Original Message-
> > From: Yi Yang [mailto:yi.y.y...@intel.com]
> > Sent: Wednesday, 02 August, 2017 10:04
> > To: d...@openvswitch.org
> > Cc: b...@ovn.org; Jan Scheurich ; Yi Yang 
> > ; Zoltán Balogh
> > 
> > Subject: [PATCH v5 1/2] OF support and translation of generic encap and 
> > decap
> > 
> > From: Jan Scheurich 
> > 
> > This commit adds support for the OpenFlow actions generic encap
> > and decap (as specified in ONF EXT-382) to the OVS control plane.
> > 
> > CLI syntax for encap action with properties:
> >   encap()
> >   encap((=,(,,),...))
> > 
> > For example:
> >   encap(ethernet)
> >   encap(nsh(md_type=1))
> >   
> > encap(nsh(md_type=2,tlv(0x1000,10,0x12345678),tlv(0x2000,20,0xfedcba9876543210)))
> > 
> > CLI syntax for decap action:
> >   decap()
> >   decap(packet_type(ns=,type=))
> > 
> > For example:
> >   decap()
> >   decap(packet_type(ns=0,type=0xfffe))
> >   decap(packet_type(ns=1,type=0x894f))
> > 
> > The first header supported for encap and decap is "ethernet" to convert
> > packets between packet_type (1,Ethertype) and (0,0).
> > 
> > This commit also implements a skeleton for the translation of generic
> > encap and decap actions in ofproto-dpif and adds support to encap and
> > decap an Ethernet header.
> > 
> > In general translation of encap commits pending actions and then rewrites
> > struct flow in accordance with the new packet type and header. In the
> > case of encap(ethernet) it suffices to change the packet type from
> > (1, Ethertype) to (0,0) and set the dl_type accordingly. A new
> > pending_encap flag in xlate ctx is set to mark that an corresponding
> > datapath encap action must be triggered at the next commit. In the
> > case of encap(ethernet) ofproto generetas a push_eth action.
> > 
> > The general case for translation of decap() is to emit a datapath action
> > to decap the current outermost header and then recirculate the packet
> > to reparse the inner headers. In the special case of an Ethernet packet,
> > decap() just changes the packet type from (0,0) to (1, dl_type) without
> > a need to recirculate. The emission of the pop_eth action for the
> > datapath is postponed to the next commit.
> > 
> > Hence encap(ethernet) and decap() on an Ethernet packet are OF octions
> > that only incur a cost in the dataplane when a modifed packet is
> > actually committed, e.g. because it is sent out. They can freely be
> > used for normalizing the packet type in the OF pipeline without
> > degrading performance.
> > 
> > Signed-off-by: Jan Scheurich 
> > Signed-off-by: 

Re: [ovs-dev] [PATCH v5 1/2] OF support and translation of generic encap and decap

[Jan Scheurich]

I know this comment is late as the patch has been merged already, but I just 
returned from vacation and only found today:

I agree that the new simplified ovs-ofctl syntax for encap properties looks 
quite neat for the NSH use case at hand:

encap(nsh(md_type=1))
encap(nsh(md_type=2,tlv(0x1000,10,0x12345678),tlv(0x2000,20,0xfedcba9876543210)))

But unfortunately we have fallen into the trap of oversimplifying. The generic 
encap action (drafted in EXT-382) distinguishes between the encapsulation 
packet type (in this case (1,0x894f) for NSH) and the encap property class (in 
this case 0x4 for NSH), which may be different for every property in an encap 
action.

Using the "nsh" keyword both as shorthand for the packet type and the property 
class selector works here because the NSH encap action only supports NSH 
properties so far, and in many simple applications of the generic encap action 
there may well be 1-1 relation between the encap packet type and the class of 
all supported properties. But the ONF draft specifically decouples the packet 
type from property classes for a richer set of use cases.

For example there could be a more powerful generic encap operation to add 
multiple protocol layers in one action. In this case the encap properties would 
likely belong to several property classes. Or an encapsulation header re-uses a 
general encap property already defined elsewhere, which should not be 
duplicated.

In order not to restrict the generality of the ovs-ofctl syntax, I'd suggest to 
separate packet type and the property class as follows:

encap()
encap((,(=,()),...)

For NSH the syntax would look:

encap(nsh,nsh(md_type=1))
encap(nsh,nsh(md_type=2,tlv(0x1000,10,0x12345678),tlv(0x2000,20,0xfedcba9876543210)))

To be user friendly we can also allow the above more concise syntax in the 
special case of a 1-1 relation between packet type and property class. In that 
case we could re-add the generalization of the syntax after the OVS 2.8 release 
without braking backward compatibility.

BR, Jan

> -Original Message-
> From: Yi Yang [mailto:yi.y.y...@intel.com]
> Sent: Wednesday, 02 August, 2017 10:04
> To: d...@openvswitch.org
> Cc: b...@ovn.org; Jan Scheurich ; Yi Yang 
> ; Zoltán Balogh
> 
> Subject: [PATCH v5 1/2] OF support and translation of generic encap and decap
> 
> From: Jan Scheurich 
> 
> This commit adds support for the OpenFlow actions generic encap
> and decap (as specified in ONF EXT-382) to the OVS control plane.
> 
> CLI syntax for encap action with properties:
>   encap()
>   encap((=,(,,),...))
> 
> For example:
>   encap(ethernet)
>   encap(nsh(md_type=1))
>   
> encap(nsh(md_type=2,tlv(0x1000,10,0x12345678),tlv(0x2000,20,0xfedcba9876543210)))
> 
> CLI syntax for decap action:
>   decap()
>   decap(packet_type(ns=,type=))
> 
> For example:
>   decap()
>   decap(packet_type(ns=0,type=0xfffe))
>   decap(packet_type(ns=1,type=0x894f))
> 
> The first header supported for encap and decap is "ethernet" to convert
> packets between packet_type (1,Ethertype) and (0,0).
> 
> This commit also implements a skeleton for the translation of generic
> encap and decap actions in ofproto-dpif and adds support to encap and
> decap an Ethernet header.
> 
> In general translation of encap commits pending actions and then rewrites
> struct flow in accordance with the new packet type and header. In the
> case of encap(ethernet) it suffices to change the packet type from
> (1, Ethertype) to (0,0) and set the dl_type accordingly. A new
> pending_encap flag in xlate ctx is set to mark that an corresponding
> datapath encap action must be triggered at the next commit. In the
> case of encap(ethernet) ofproto generetas a push_eth action.
> 
> The general case for translation of decap() is to emit a datapath action
> to decap the current outermost header and then recirculate the packet
> to reparse the inner headers. In the special case of an Ethernet packet,
> decap() just changes the packet type from (0,0) to (1, dl_type) without
> a need to recirculate. The emission of the pop_eth action for the
> datapath is postponed to the next commit.
> 
> Hence encap(ethernet) and decap() on an Ethernet packet are OF octions
> that only incur a cost in the dataplane when a modifed packet is
> actually committed, e.g. because it is sent out. They can freely be
> used for normalizing the packet type in the OF pipeline without
> degrading performance.
> 
> Signed-off-by: Jan Scheurich 
> Signed-off-by: Yi Yang 
> Signed-off-by: Zoltan Balogh 
> Co-authored-by: Zoltan Balogh 
> Signed-off-by: Ben Pfaff 
> ---
>  NEWS   |   6 +
>  include/openflow/openflow-common.h |   1 +
>  include/openvswitch/automake.mk|   1 +
>  include/openvswitch/ofp-actions.h  |