Re: [ovs-dev] [PATCH v5 1/2] OF support and translation of generic encap and decap
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 > > > defined elsewhere, which should not be duplicated. > > > > > > > > In order not to restrict the generality of the ovs-ofctl syntax, I'd > > > > suggest to sep
Re: [ovs-dev] [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 > > 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((=,(,,),..
Re: [ovs-dev] [PATCH v5 1/2] OF support and translation of generic encap and decap
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 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" t
Re: [ovs-dev] [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 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
Re: [ovs-dev] [PATCH v5 1/2] OF support and translation of generic encap and decap
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 | 32 > include/openvswitch/ofp-ed-props.h | 77 > include/openvswitch/ofp-errors.h | 9 + > lib/automake.mk| 1 + > lib/odp-util.c | 84 ++--- > lib/odp-util.h