> -Original Message-
> From: Martin Varghese
> Sent: Tuesday, 13 April, 2021 16:20
> To: Jan Scheurich
> Cc: Eelco Chaudron ; d...@openvswitch.org;
> pshe...@ovn.org; martin.vargh...@nokia.com
> Subject: Re: [PATCH v4 1/2] Encap & Decap actions for MPLS packet type.
>
> On Wed, Apr 07, 2021 at 03:49:07PM +, Jan Scheurich wrote:
> > Hi Martin,
> >
> > I guess you are aware of the original design document we wrote for generic
> encap/decap and NSH support:
> > https://protect2.fireeye.com/v1/url?k=993ba795-c6a09d8c-993be70e-8682a
> > aa22bc0-3c9b4464027ca7bf&q=1&e=f89fc25e-8dc0-45bf-bdd9-
> 2d0ca03b5686&u=
> >
> https%3A%2F%2Fdocs.google.com%2Fdocument%2Fd%2F1oWMYUH8sjZJzWa72
> o2q9kU
> > 0N6pNE-rwZcLH3-kbbDR8%2Fedit%23
> >
> > It is no longer 100% aligned with the final implementation in OVS but still
> > a
> good reference for understanding the design principles behind the
> implementation and some specifics for Ethernet and NSH encap/decap use
> cases.
> >
> > Please find some more answers/comments below.
> >
> > BR, Jan
> >
> > > -Original Message-
> > > From: Martin Varghese
> > > Sent: Wednesday, 7 April, 2021 10:43
> > > To: Jan Scheurich
> > > Cc: Eelco Chaudron ; d...@openvswitch.org;
> > > pshe...@ovn.org; martin.vargh...@nokia.com
> > > Subject: Re: [PATCH v4 1/2] Encap & Decap actions for MPLS packet type.
> > >
> > > On Tue, Apr 06, 2021 at 09:00:16AM +, Jan Scheurich wrote:
> > > > Hi,
> > > >
> > > > Thanks for the heads up. The interaction with MPLS push/pop is a
> > > > use case
> > > that was likely not tested during the NSH and generic encap/decap
> > > design. It's complex code and a long time ago. I'm willing to help,
> > > but I will need some time to go back and have a look.
> > > >
> > > > It would definitely help, if you could provide a minimal example
> > > > for
> > > reproducing the problem.
> > > >
> > >
> > > Hi Jan ,
> > >
> > > Thanks for your help.
> > >
> > > I was trying to implement ENCAP/DECAP support for MPLS.
> > >
> > > The programming of datapath flow for the below userspace rule fails
> > > as there is set(eth() action between pop_mpls and recirc ovs-ofctl
> > > -O OpenFlow13 add- flow br_mpls2
> > > "in_port=$egress_port,dl_type=0x8847
> > > actions=decap(),decap(packet_type(ns=0,type=0),goto_table:1
> > >
> > > 2021-04-05T05:46:49.192Z|00068|dpif(handler51)|WARN|system@ovs-
> > > system: failed to put[create] (Invalid argument)
> > > ufid:1dddb0ba-27fe-44ea- 9a99-5815764b4b9c
> > > recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(6),skb_mark(0/0)
> > > ,ct_state
> > > (0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=00:00:00:00:00:01/00:
> > > 00:00:00:00:00,dst=00:00:00:00:00:02/00:00:00:00:00:00),eth_type(0x8
> > > 847) ,mpls(label=2/0x0,tc=0/0,ttl=64/0x0,bos=1/1),
> > > actions:pop_eth,pop_mpls(eth_type=0x6558),set(eth()),recirc(0x45)
> > >
> >
> > Conceptually, what should happen in this scenario is that, after the second
> decap(packet_type(ns=0,type=0) action, OVS processes the unchanged inner
> packet as packet type PT_ETH, i.e. as L2 Ethernet frame. Overwriting the
> existing Ethernet header with zero values through set(eth()) is clearly
> incorrect. That is a logical error inside the ofproto-dpif-xlate module (see
> below).
> >
> > I believe the netdev userspace datapath would still have accepted the
> incorrect datapath flow. I have too little experience with the kernel
> datapath to
> explain why that rejects the datapath flow as invalid.
> >
> > Unlike in the Ethernet and NSH cases, the MPLS header does not contain any
> indication about the inner packet type. That is why the packet_type must be
> provided by the SDN controller as part of the decap() action. And the
> ofproto-
> dpif-xlate module must consider the specified inner packet type when
> continuing the translation. In the general case, a decap() action should
> trigger
> recirculation for reparsing of the inner packet, so the new packet type must
> be
> set before recirculation. (Exceptions to the general recirculation rule are
> those
> where OVS has already parsed further into the packet and ofproto can modify
> the flow on the fly: decap(Ethernet) and possibly decap(MPLS) for all but the
> last bottom of stack label).
> >
> > I have had a look at your new code for encap/decap of MPLS headers, but I
> must admit I cannot fully judge in how far re-using the existing translation
> functions for MPLS label stacks written for the legacy push/pop_mpls case
> (i.e.
> manipulating a label stack between the L2 and the L3 headers of a PT_ETH
> Packet) are possible to re-use in the new context.
> >
> > BTW: Do you support multiple MPLS label encap or decap actions with your
> patch? Have you tested that?
> >
> > I am uncertain about the handling of the ethertype of the decapsulated inner
> packet. In the design base, the ethertype that is set in the existing L2
> header of
> the packet after pop_mpls of the last label is coming from the pop_mpls
> ac
