[ovs-dev] [PATCH v4 1/2] Encap & Decap actions for MPLS packet type.
Martin Varghese
martinvarghesenokia at gmail.com
Sat Jun 19 06:06:23 UTC 2021
On Thu, Apr 08, 2021 at 03:31:24PM +0200, Eelco Chaudron wrote:
>
>
> On 8 Apr 2021, at 14:05, Martin Varghese wrote:
>
> > On Wed, Apr 07, 2021 at 03:49:07PM +0000, 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://docs.google.com/document/d/1oWMYUH8sjZJzWa72o2q9kU0N6pNE-rwZcLH3-kbbDR8/edit#
> > >
> > > 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 <martinvarghesenokia at gmail.com>
> > > > Sent: Wednesday, 7 April, 2021 10:43
> > > > To: Jan Scheurich <jan.scheurich at ericsson.com>
> > > > Cc: Eelco Chaudron <echaudro at redhat.com>; dev at openvswitch.org;
> > > > pshelar at ovn.org; martin.varghese at nokia.com
> > > > Subject: Re: [PATCH v4 1/2] Encap & Decap actions for MPLS
> > > > packet type.
> > > >
> > > > On Tue, Apr 06, 2021 at 09:00:16AM +0000, 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 at 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(0x8847)
> > > > ,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?
> > >
> > Yes, I will add those tests too.
>
> Maybe you could add some tests the same way NSH does, by sending a packet
> and verifying the modified content. The current test does encap than decap,
> so if both go wrong, or are skipped we are not actually testing anything.
>
> > > 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 action, while in the
> > > decap(packet_type(0,0)) case the entire inner packet should be
> > > recirculated as is with packet_type PT_ETH.
> > >
> > > case PT_MPLS: {
> > > int n;
> > > ovs_be16 ethertype;
> > >
> > > flow->packet_type = decap->new_pkt_type;
> > > ethertype = pt_ns_type_be(flow->packet_type);
> > >
> > > n = flow_count_mpls_labels(flow, ctx->wc);
> > > flow_pop_mpls(flow, n, ethertype, ctx->wc);
> > > if (!ctx->xbridge->support.add_mpls) {
> > > ctx->xout->slow |= SLOW_ACTION;
> > > }
> > > ctx->pending_decap = true;
> > > return true;
> > >
> > > In the example scenario the new_pkt_type is PT_ETH, so the ethertype
> > > and hence flow->dl_type will be also be set to zero. Is that
> > > intentional? For target packet types of form
> > > PACKET_TYPE(OFPHTN_ETHERTYPE, <ethertype>) you would set the correct
> > > flow->dl_type but does this matter just before recirculation?
> > >
> > Yes, that was intentional.
> > But i am thinking now to store dl_type as 0x6558 when the new
> > packet_type is PT_ETH. In that way it is consistent with the datapath dl
> > type. I could also avoid the special handling done now for decap MPLS
> > in commit_mpls_action.
> > Setting flow->dl_type matters as this value is used as the argument for
> > datapath pop_mpls action
> > > I don't see any logic to check if the popped label was the BoS
> > > label. Only when popping the BoS label, we should recirculate the
> > > packet for parsing the inner packet.
> > >
> > I missed that. I will fix it
> > > Conversely, in the encap(mpls) action, the new PT should be PT_MPLS
> > > and the all other flow layers (L2, L3, L4) should be cleared. I
> > > think the reused flow_push_mpls() function doesn't take care of
> > > clearing the L2 flow fields.
> > >
> > I will add that outside the flow_push_mpls
> > > Have you tested with other packet_types for inner packets (e.g.
> > > PT_IP or PT_ARP). If this is supposed to work, it would be nice to
> > > test the combination of
> > >
> > > decap(), encap(mpls), set_mpls_label, encap(eth), set_field:dl_dst
> > >
> > > on one side against the equivalent legacy action pop_mpls on the
> > > other end and vice versa.
> > >
> > Noted
> > >
> > > > As I pointed out in my previous mail, in
> > > > commit_set_ether_action Function
> > > > “flow->packet_type != htonl(PT_ETH)” is used to check if the
> > > > packet is ethernet
> > > > instead of base_flow->packet_type.
> > > >
> > > > The mac address in flow structure is not cleared in PT_ETH
> > > > handling of
> > > > xlate_generic_decap_action but it is cleared in base_flow in
> > > > the decap
> > > > handling of PT_ETH in commit_encap_decap_action function Due to this
> > > > difference the set(eth(dst) action will be programmed to the
> > > > datapath.
> > > >
> > > > I tried to fix this issue in 2 different ways.
> > > > 1 I have cleared the mac address in flow structure in PT_ETH
> > > > handling of
> > > > xlate_generic_decap action.
> > > > 2 In the commit_set_ether action I changed the check from
> > > > “flow-
> > > > > packet_type != htonl(PT_ETH)” to “base_flow->packet_type !=
> > > > htonl(PT_ETH))”.
> > >
> > > In the light of the discussion, I think you should implement both
> > > fixes as follows:
> > >
> > > 1. Clear the mac addresses in struct flow in the PT_ETH clause of
> > > xlate_generic_decap_action().
> > >
> > > This is correct as the flow struct is modified on the fly here and
> > > should accurately reflect the current packet headers.
> > >
> > > 2. Change the guard in commit_set_ether_action() to
> > >
> > > if (flow->packet_type != htonl(PT_ETH) || base_flow->packet_type
> > > != htonl(PT_ETH)) {
> > > return;
> > > }
> > >
> > > In either case setting the MAC header based on differences between
> > > base_flow and flow doesn't make sense.
> > >
> > > >
> > > > Though both of them solves this problem, couple of NSH
> > > > regression tests are
> > > > failing
> > > >
> > > > 2291: nsh - md1 encap over a veth link FAILED
> > > > (nsh.at:85)
> > > > 58022292: nsh - md2 encap over a veth link FAILED
> > > > (nsh.at:213)
> > > >
> > > > I see that they are failing as they are expecting a set(eth(dst)
> > > > between the the
> > > > pop_nsh and the recirc.
> > > > Set(eth) action is because of the reasons explained above –
> > > > Datapath actions:
> > > > push_nsh(flags=0,ttl=63,mdtype=1,np=3,spi=0x1234,si=255,c1=0x11223344,c
> > > > 2=0x0,c3=0x0,c4=0x0),push_eth(src=00:00:00:00:00:00,dst=11:22:33:44:55:6
> > > > 6),pop_eth,pop_nsh(),set(eth(dst=11:22:33:44:55:66)),recirc(0x1)
> > > >
> > > > In my understanding set(eth) here is wrong as there is no set
> > > > ethernet action
> > > > in the userspace rule
> > > > table=0,in_port=4,dl_type=0x894f,nsh_mdtype=1,nsh_spi=0x1234,nsh_c1=0x
> > > > 11223344,actions=decap(),decap(),2.
> > >
> > > I think you are right here. The extra set(eth(dst)) action is a bad
> > > translation artefact triggered by the above bugs.
> > >
> > > The failing basic NSH test cases carry Ethernet packets inside the
> > > NSH tunnel encap, so the decap() action should just recirculate the
> > > unchanged packets with PT_ETH.
> > >
> > > These test cases should be corrected as part of the bug-fix commit
> > > for the set(eth()) issue.
> > >
> > I will fix it.
> >
> > Thanks for your time.
>
> Copy me on the new version and I continue my review.
> Here are some small nits you might be able to fix in your next revision:
>
With a rebase to the latest. The ARP test case you have tried should work.jfyi
> > +++ b/lib/odp-util.c
> > @@ -142,6 +142,8 @@ odp_action_len(uint16_t type)
> > case OVS_ACTION_ATTR_PUSH_NSH: return ATTR_LEN_VARIABLE;
> > case OVS_ACTION_ATTR_POP_NSH: return 0;
> > case OVS_ACTION_ATTR_CHECK_PKT_LEN: return ATTR_LEN_VARIABLE;
> > + case OVS_ACTION_ATTR_ADD_MPLS:
> > + return sizeof(struct ovs_action_add_mpls);
>
> Guess this can be on one line like all the other case statements as it fits
> the 80 character limit.
>
noted
> > diff --git a/lib/ovs-actions.xml b/lib/ovs-actions.xml
> > index a2778de4b..e97f818d9 100644
> > --- a/lib/ovs-actions.xml
> > +++ b/lib/ovs-actions.xml
> > @@ -265,13 +265,13 @@
> ...
> > @@ -1144,6 +1152,9 @@ for <var>i</var> in [1,<var>n_members</var>]:
> > <action name="DECAP">
> > <h2>The <code>decap</code> action</h2>
> > <syntax><code>decap</code></syntax>
> > + <syntax><code>decap(packet_type(ns=<var>name_space</var>,
> > + type=<var>ethertype</var>))</code></syntax> for decapsulating
> > MPLS
> > + packets.
>
> Can you add an explanation for name_space and type? As it’s not clear to me
> how these values are used.
packet_type is already documented in ovs-fields man page. Do we need to
enhance that?
>
> > > >
> > > > To reproduce the mpls problem you could use the same test what I
> > > > have added
> > > > in the patch. But you have to modify the test script like below
> > > > - table=0,priority=100,dl_type=0x0800
> > > > actions=encap(mpls(ether_type=0x8847)),set_mpls_label:2,encap(ethernet),o
> > > > utput:100
> > > > + table=0,priority=100,dl_type=0x0800
> > > > actions=encap(mpls(ether_type=0x8847)),set_mpls_label:2,
> > > > encap(ethernet),set_field:00:00:00:00:00:02-
> > > > > dl_dst,set_field:00:00:00:00:00:01->dl_src,output:100"
> > > >
> > > >
> > > > > BR, Jan
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Eelco Chaudron <echaudro at redhat.com>
> > > > > > Sent: Tuesday, 6 April, 2021 10:55
> > > > > > To: Martin Varghese <martinvarghesenokia at gmail.com>; Jan Scheurich
> > > > > > <jan.scheurich at ericsson.com>
> > > > > > Cc: dev at openvswitch.org; pshelar at ovn.org;
> > > > > > martin.varghese at nokia.com
> > > > > > Subject: Re: [PATCH v4 1/2] Encap & Decap actions for
> > > > > > MPLS packet type.
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 6 Apr 2021, at 10:27, Martin Varghese wrote:
> > > > > >
> > > > > > > On Thu, Apr 01, 2021 at 11:32:06AM +0200, Eelco Chaudron wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > On 1 Apr 2021, at 11:28, Martin Varghese wrote:
> > > > > > > >
> > > > > > > > > On Thu, Apr 01, 2021 at 11:17:14AM +0200, Eelco Chaudron wrote:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On 1 Apr 2021, at 11:09, Martin Varghese wrote:
> > > > > > > > > >
> > > > > > > > > > > On Thu, Apr 01, 2021 at 10:54:42AM
> > > > > > > > > > > +0200, Eelco Chaudron wrote:
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > On 1 Apr 2021, at 10:35, Martin Varghese wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > > On Thu, Apr 01, 2021 at 08:59:27AM +0200, Eelco Chaudron
> > > > wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On 1 Apr 2021, at 6:10, Martin Varghese wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > On Wed, Mar 31, 2021 at 03:59:40PM +0200, Eelco Chaudron
> > > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > On 26 Mar 2021, at 7:21, Martin Varghese wrote:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > From: Martin Varghese <martin.varghese at nokia.com>
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > The encap & decap actions are extended to support MPLS
> > > > > > > > > > > > > > > > > packet type.
> > > > > > > > > > > > > > > > > Encap & decap actions adds and removes MPLS header at
> > > > > > > > > > > > > > > > > start of the packet.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Hi Martin,
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I’m trying to do
> > > > > > > > > > > > > > > > some real-life
> > > > > > > > > > > > > > > > testing, and I’m
> > > > > > > > > > > > > > > > running
> > > > > > > > > > > > > > > > into issues. This might be me setting it up wrongly but
> > > > > > > > > > > > > > > > just wanting to confirm…
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I’m sending an MPLS packet that contains an ARP packet
> > > > > > > > > > > > > > > > into a physical port.
> > > > > > > > > > > > > > > > This is the packet:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Frame 4: 64 bytes on wire (512 bits), 64 bytes captured
> > > > > > > > > > > > > > > > (512
> > > > > > > > > > > > > > > > bits)
> > > > > > > > > > > > > > > > Encapsulation type: Ethernet (1)
> > > > > > > > > > > > > > > > [Protocols in frame: eth:ethertype:mpls:data]
> > > > > > > > > > > > > > > > Ethernet II,
> > > > > > > > > > > > > > > > Src:
> > > > > > > > > > > > > > > > 00:00:00_00:00:01
> > > > > > > > > > > > > > > > (00:00:00:00:00:01),
> > > > > > > > > > > > > > > > Dst:
> > > > > > > > > > > > > > > > 00:00:00_00:00:02 (00:00:00:00:00:02)
> > > > > > > > > > > > > > > > Destination: 00:00:00_00:00:02 (00:00:00:00:00:02)
> > > > > > > > > > > > > > > > Address: 00:00:00_00:00:02 (00:00:00:00:00:02)
> > > > > > > > > > > > > > > > .... ..0. .... .... .... .... = LG bit: Globally
> > > > > > > > > > > > > > > > unique address (factory default)
> > > > > > > > > > > > > > > > .... ...0 .... .... .... .... = IG bit:
> > > > > > > > > > > > > > > > Individual address
> > > > > > > > > > > > > > > > (unicast)
> > > > > > > > > > > > > > > > Source: 00:00:00_00:00:01 (00:00:00:00:00:01)
> > > > > > > > > > > > > > > > Address: 00:00:00_00:00:01 (00:00:00:00:00:01)
> > > > > > > > > > > > > > > > .... ..0. .... .... .... .... = LG bit: Globally
> > > > > > > > > > > > > > > > unique address (factory default)
> > > > > > > > > > > > > > > > .... ...0 .... .... .... .... = IG bit:
> > > > > > > > > > > > > > > > Individual address
> > > > > > > > > > > > > > > > (unicast)
> > > > > > > > > > > > > > > > Type: MPLS label switched packet (0x8847)
> > > > > > > > > > > > > > > > MultiProtocol
> > > > > > > > > > > > > > > > Label Switching
> > > > > > > > > > > > > > > > Header, Label:
> > > > > > > > > > > > > > > > 100, Exp: 0, S:
> > > > > > > > > > > > > > > > 1, TTL:
> > > > > > > > > > > > > > > > 64
> > > > > > > > > > > > > > > > 0000 0000
> > > > > > > > > > > > > > > > 0000 0110 0100
> > > > > > > > > > > > > > > > .... .... .... =
> > > > > > > > > > > > > > > > MPLS Label: 100
> > > > > > > > > > > > > > > > .... .... .... .... .... 000. .... .... = MPLS
> > > > > > > > > > > > > > > > Experimental
> > > > > > > > > > > > > > > > Bits: 0
> > > > > > > > > > > > > > > > .... ....
> > > > > > > > > > > > > > > > .... .... ....
> > > > > > > > > > > > > > > > ...1 .... .... =
> > > > > > > > > > > > > > > > MPLS Bottom
> > > > > > > > > > > > > > > > Of Label
> > > > > > > > > > > > > > > > Stack: 1
> > > > > > > > > > > > > > > > .... .... .... .... .... .... 0100 0000 = MPLS TTL:
> > > > > > > > > > > > > > > > 64 Data (46 bytes)
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > 0000 ff ff ff ff ff ff 52 54 00 88 51 38 08 06 00 01
> > > > > > > > > > > > > > > > ......RT..Q8....
> > > > > > > > > > > > > > > > 0010 08 00 06 04 00 01 52 54 00 88 51 38 01 01 01 65
> > > > > > > > > > > > > > > > ......RT..Q8...e
> > > > > > > > > > > > > > > > 0020 00 00 00 00 00 00 01 01 01 64 27 98 a0 47
> > > > > > > > > > > > > > > > .........d'..G
> > > > > > > > > > > > > > > > Data:
> > > > > > > > > > > > > > > >
> > > > > >
> > > > ffffffffffff52540088513808060001080006040001525400885138010101650
> > > > 000
> > > > > > 0
> > > > > > 000?
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I’m trying to use the following rules:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > ovs-ofctl del-flows ovs_pvp_br0
> > > > > > > > > > > > > > > > ovs-ofctl add-flow -O OpenFlow13 ovs_pvp_br0
> > > > > > > > > > > > > > > > "priority=100,dl_type=0x8847,mpls_label=100
> > > > > > > > > > > > > > > >
> > > > > > actions=decap(),decap(packet_type(ns=0,type=0x806)),resubmit(,3)"
> > > > > > > > > > > > > > > > ovs-ofctl add-flow -O OpenFlow13 ovs_pvp_br0
> > > > > > > > > > > > > > > > "table=3,priority=10
> > > > > > > > > > > > > > > > actions=normal"
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > With these, I expect the packet to be sent to vnet0, but
> > > > > > > > > > > > > > > > it’s not.
> > > > > > > > > > > > > > > > Actually,
> > > > > > > > > > > > > > > > the datapath rule looks odd, while the userspace rules
> > > > > > > > > > > > > > > > seem to match:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > $ ovs-dpctl dump-flows
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > recirc_id(0),in_port(1),eth(),eth_type(0x8847),mpls(label
> > > > > > > > > > > > > > > > =100 /0xfffff,tc=0/0,ttl=0/0x0,bos=1/1),
> > > > > > > > > > > > > > > > packets:13, bytes:1118, used:0.322s,
> > > > > > > > > > > > > > > > actions:pop_eth,pop_mpls(eth_type=0x806),recirc(0x19a)
> > > > > > > > > > > > > > > > recirc_id(0x19a),in_port(1),eth_type(0x0806),
> > > > > > > > > > > > > > > > packets:13, bytes:884, used:0.322s, actions:drop
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > $ ovs-ofctl dump-flows ovs_pvp_br0 -O OpenFlow13
> > > > > > > > > > > > > > > > cookie=0x0, duration=85.007s, table=0, n_packets=51,
> > > > > > > > > > > > > > > > n_bytes=4386,
> > > > > > > > > > > > > > > > priority=100,mpls,mpls_label=100
> > > > > > > > > > > > > > > >
> > > > > > actions=decap(),decap(packet_type(ns=0,type=0x806)),resubmit(,3)
> > > > > > > > > > > > > > > > cookie=0x0, duration=84.990s, table=3, n_packets=51,
> > > > > > > > > > > > > > > > n_bytes=3468,
> > > > > > > > > > > > > > > > priority=10 actions=NORMAL
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > The inner packet is
> > > > > > > > > > > > > > > ethernet. So the
> > > > > > > > > > > > > > > packet type should
> > > > > > > > > > > > > > > be
> > > > > > > > > > > > > > > (ns=0,type=0)
> > > > > > > > > > > > > > > ?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Forgot to add that I already tried that to start with,
> > > > > > > > > > > > > > based on the example,
> > > > > > > > > > > > > > but as that did not work
> > > > > > > > > > > > > > I tried 0x806.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > PS: I have this as a remark in my review notes, i.e., to
> > > > > > > > > > > > > > explain the ns and type usage here.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > This resulted in packets being counted at the open flow
> > > > > > > > > > > > > > level, but it results in
> > > > > > > > > > > > > > NO data path rules. Do
> > > > > > > > > > > > > > get an error though:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > 2021-04-
> > > > > > 01T06:53:36.056Z|00141|dpif(handler37)|WARN|system at ovs-system:
> > > > > > > > > > > > > > failed to put[create] (Invalid argument)
> > > > > > > > > > > > > > ufid:3d2d6f6d-5a66-4ace-8b09-7cdcfa5efc8e
> > > > > > > > > > > > > > recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(1),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(0x8847),mpls(label=100/0xfffff,
> > > > > > > > > > > > > > tc=0
> > > > > > > > > > > > > > /0,ttl=64/0x0,bos=1/1),
> > > > > > > > > > > > > > actions:pop_eth,pop_mpls(eth_type=0x6558),set(eth()),recirc
> > > > > > > > > > > > > > (0x4
> > > > > > > > > > > > > > c)
> > > > > > > > > > > > >
> > > > > > > > > > > > > This set(eth) before the recirc is the problem i guesss. I
> > > > > > > > > > > > > need to check
> > > > > > >
> > > > > > > I could reproduce the problem. It has nothing to do
> > > > > > > with ARP or IP.
> > > > > > > Unlike my test scripts, in your test you are setting the mac
> > > > > > > address after the encap action
> > > > > > >
> > > > > > > Ovs-vswitchd is programming a set(eth(dst) action between the
> > > > > > > pop_mpls and recirc as it sees a difference in mac address in
> > > > > > > flow structure and base_flow structure.
> > > > > > >
> > > > > > > The mac address in flow structure is not cleared in PT_ETH
> > > > > > > handling of xlate_generic_decap_action but it is cleared in
> > > > > > > base_flow in the decap handling of PT_ETH in
> > > > > > > commit_encap_decap_action function
> > > > > > >
> > > > > > > Due to this difference the set(eth(dst) action will be programmed
> > > > > > > to the datapath.
> > > > > > >
> > > > > > > Also, I see that in commit_set_ether_action Function
> > > > > > > “flow->packet_type != htonl(PT_ETH)” is used to check if the
> > > > > > > packet is ethernet instead of base_flow->packet_type.
> > > > > > >
> > > > > > > I assume check on base_flow->packet_type make more sense here ?
> > > > > > >
> > > > > > > I tried to fix this issue in 2 different ways.
> > > > > > >
> > > > > > > 1 I have cleared the mac address in flow structure in PT_ETH
> > > > > > > handling of xlate_generic_decap action.
> > > > > > >
> > > > > > > 2 In the commit_set_ether action I changed the check from
> > > > > > > “flow->packet_type != htonl(PT_ETH)” to
> > > > > > > “base_flow->packet_type
> > > > > > > != htonl(PT_ETH))”.
> > > > > > >
> > > > > > > Though both of them solves this problem, couple of NSH regression
> > > > > > > tests are failing
> > > > > > >
> > > > > > > 2291: nsh - md1 encap over a veth link FAILED
> > > > > > > (nsh.at:85)
> > > > > > >
> > > > > > > 58022292: nsh - md2 encap over a veth link FAILED
> > > > > > > (nsh.at:213)
> > > > > > >
> > > > > > > I see that they are failing as they are expecting a set(eth(dst)
> > > > > > > between the the pop_nsh and the recirc.
> > > > > > >
> > > > > > > Set(eth) action is because of the reasons explained above –
> > > > > > >
> > > > > > > Datapath actions:
> > > > > > > push_nsh(flags=0,ttl=63,mdtype=1,np=3,spi=0x1234,si=255,c1=0x11223
> > > > > > > 344,
> > > > > > > c2=0x0,c3=0x0,c4=0x0),push_eth(src=00:00:00:00:00:00,dst=11:22:33:
> > > > > > > 44:5
> > > > > > > 5:66),pop_eth,pop_nsh(),set(eth(dst=11:22:33:44:55:66)),recirc(0x1
> > > > > > > )
> > > > > > >
> > > > > > > In my understanding set(eth) here is wrong as there is no set
> > > > > > > ethernet action in the userspace rule
> > > > > > > - Hide quoted text -
> > > > > > >
> > > > > > > table=0,in_port=4,dl_type=0x894f,nsh_mdtype=1,nsh_spi=0x1234,nsh_c
> > > > > > > 1=0x
> > > > > > > 11223344,actions=decap(),decap(),2
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Could someone comment ?
> > > > > >
> > > > > > Maybe Jan can answer as he did the NSH implementation,
> > > > > > however, what
> > > > > > would be of interest if you can give me an example of how the
> > > > > > encap()
> > > > > > decap() for this would be used in real life so I’m sure
> > > > > > I’m testing it
> > > > correctly?
> > > > > >
> > > > > > What I did so far was to encapsulate all traffic going
> > > > > > from a VM to
> > > > > > the physical port in MPLS using the flows like:
> > > > > >
> > > > > > ovs-ofctl add-flow -O OpenFlow13 ovs_pvp_br0
> > > > > > "priority=100,in_port=vnet0,actions=encap(mpls(ether_type=0x8847)),s
> > > > > > et_mpls
> > > > > > _label:100,encap(ethernet),,set_field:00:00:00:00:00:02->dl_ds
> > > > > > t,set_field:00:00:00:00:00:01->dl_src,output:enp5s0f0"
> > > > > >
> > > > > > Then I would capture this traffic and sent it back over the same
> > > > > > port, hoping it would come out as plane traffic with the
> > > > > > following rule:
> > > > > >
> > > > > > ovs-ofctl add-flow -O OpenFlow15 ovs_pvp_br0
> > > > > > "priority=100,dl_type=0x8847,mpls_label=100
> > > > > > actions=decap(),decap(packet_type(ns=0,type=0)),resubmit(,3)"
> > > > > > ovs-ofctl add-flow -O OpenFlow15 ovs_pvp_br0 "table=3,priority=10
> > > > > > actions=normal"
> > > > > >
> > > > > > If this is correct, let me know, and if Jan does not
> > > > > > reply, I’ll try
> > > > > > to understand the code in this area and see if I can find out some
> > > > > > details…
> > > > > >
> > > > > > //Eelco
> > > > > >
> > > > > > > > > > > > > > 2021-04-
> > > > > > 01T06:53:36.056Z|00142|dpif(handler37)|WARN|system at ovs-system:
> > > > > > > > > > > > > > execute
> > > > > > > > > > > > > > pop_eth,pop_mpls(eth_type=0x6558),set(eth()),recirc(0x4c)
> > > > > > > > > > > > > > failed
> > > > > > > > > > > > > > (Invalid argument) on packet
> > > > > > > > > > > > > > mpls,vlan_tci=0x0000,dl_src=00:00:00:00:00:01,dl_dst=00:00:
> > > > > > > > > > > > > > 00:0
> > > > > > > > > > > > > > 0:00:02,mpls_label=100,mpls_tc=0,mpls_ttl=64,mpls_bos=1
> > > > > > > > > > > > > > with metadata
> > > > > > > > > > > > > > skb_priority(0),skb_mark(0),in_port(1)
> > > > > > > > > > > > > > mtu 0
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Are there missing parts in my kernel that do not get
> > > > > > > > > > > > > > properly detected by the feature detection?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > $ ovs-appctl dpif/show-dp-features ovs_pvp_br0 Masked set
> > > > > > > > > > > > > > action: Yes Tunnel push pop: No
> > > > > > > > > > > > > > Ufid: Yes
> > > > > > > > > > > > > > Truncate action: Yes
> > > > > > > > > > > > > > Clone action: Yes
> > > > > > > > > > > > > > Sample nesting: 10
> > > > > > > > > > > > > > Conntrack eventmask: Yes
> > > > > > > > > > > > > > Conntrack clear: Yes
> > > > > > > > > > > > > > Max dp_hash algorithm: 0
> > > > > > > > > > > > > > Check pkt length action: Yes Conntrack timeout policy: Yes
> > > > > > > > > > > > > > Explicit Drop action: No Optimized Balance TCP mode: No
> > > > > > > > > > > > > > l2 MPLS tunnelling: Yes
> > > > > > > > > > > > > > Max VLAN headers: 2
> > > > > > > > > > > > > > Max MPLS depth: 3
> > > > > > > > > > > > > > Recirc: Yes
> > > > > > > > > > > > > > CT state: Yes
> > > > > > > > > > > > > > CT zone: Yes
> > > > > > > > > > > > > > CT mark: Yes
> > > > > > > > > > > > > > CT label: Yes
> > > > > > > > > > > > > > CT state NAT: Yes
> > > > > > > > > > > > > > CT orig tuple: Yes
> > > > > > > > > > > > > > CT orig tuple for IPv6: Yes
> > > > > > > > > > > > > > IPv6 ND Extension: No
> > > > > > > > > > > > > >
> > > > > > > > > > > > > You are good
> > > > > > > > > > > > >
> > > > > > > > > > > > > I am not sure what is going
> > > > > > > > > > > > > wrong. Your test case looks
> > > > > > > > > > > > > same as
> > > > > > > > > > > > > the unit test i added.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I tried myself again and this is i get
> > > > > > > > > > > > >
> > > > > > > > > > > > > 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"
> > > > > > > > > > > > > ovs-ofctl -O OpenFlow13 add-flow br_mpls2
> > > > > > > > > > > > > +"table=1,in_port=$egress_port,dl_type=0x0800,nw_dst=1.1.1.2
> > > > > > > > > > > > > +actions=set_field:00:00:00:00:00:02->dl_dst,set_field:00:00:00:
> > > > > > > > > > > > > +00:00:01->dl_sr
> > > > > > > > > > > > > +c output:$ingress_port"
> > > > > > > > > > > > >
> > > > > > > > > > > > > recirc_id(0x3),in_port(6),eth(src=36:b1:ee:7c:01:03,dst=36:b1:ee
> > > > > > > > > > > > > :7c:01:02),eth_
> > > > > > > > > > > > > +type(0x0800),ipv4(dst=1.1.1.2,frag=no),
> > > > > > > > > > > > > packets:3, bytes:294,
> > > > > > > > > > > > > used:0.837s,
> > > > > > > > > > > > >
> > > > +actions:set(eth(src=00:00:00:00:00:01,dst=00:00:00:00:00:02)),4
> > > > > > > > > > > > > recirc_id(0),in_port(6),eth(),eth_type(0x8847),mpls(label=0/0x0,
> > > > > > > > > > > > > tc=0/0,ttl=0/0x
> > > > > > > > > > > > > +0,bos=1/1), packets:3, bytes:348, used:0.837s,
> > > > > > > > > > > > > +actions:pop_eth,pop_mpls(eth_type=0x6558),recirc(0x3)
> > > > > > > > > > > > >
> > > > > > > > > > > > > The packet to the ovs is
> > > > > > > > > > > > > ETH|MPLS|ETH|IP ?
> > > > > > > > > > > > > How it is differnt from you test case?
> > > > > > > > > > > >
> > > > > > > > > > > > Mine is ETH|MPLS|ETH|ARP, which works fine with pop_mpls
> > > > > > > > > > > >
> > > > > > > > > > > I am wondering how old mpls pop
> > > > > > > > > > > action works could you please put
> > > > > > > > > > > down the userspace and datapath rules when you used pop_mpls.
> > > > > > > > > > >
> > > > > > > > > > > In my understanding it can never
> > > > > > > > > > > work as what you have after MPLS
> > > > > > > > > > > is ethernet and not ARP.
> > > > > > > > > >
> > > > > > > > > > It’s ethernet + ARP, but here are my rules:
> > > > > > > > >
> > > > > > > > > To clarify
> > > > > > > > >
> > > > > > > > > The test vector for Decap Test case
> > > > > > > > > ETH|MPLS|ETH|ARP
> > > > > > > > > The test vector for pop mpls test case
> > > > > > > > > ETH|MPLS|ARP|
> > > > > > > > >
> > > > > > > > > The above understanding correct?
> > > > > > > >
> > > > > > > > Guess our emails crossed ;) I was sending in
> > > > > > > > the same packet for
> > > > > > > > both the test cases, so
> > > > > > > >
> > > > > > > > ETH|MPLS|ETH|ARP
> > > > > > > >
> > > > > > > > Which with decap() is resulting in the rules not
> > > > > > > > being programmed
> > > > > > > >
> > > > > > > > With popmpls I saw the packets in being
> > > > > > > > received, but did not notice
> > > > > > > > the incorrect use of popmpls so my packet after
> > > > > > > > popmpls looks like
> > > > > > > > ETH|ETH|ARP.
> > > > > > > >
> > > > > > > > So I guess all that remains is why the data path rules is not
> > > > > > > > accepted for ARP with the decap.
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > dpctl:
> > > > > > > > > >
> > > > > > > > > > recirc_id(0),in_port(2),eth(),eth_type(0x8847),mpls(label=100/0xfff
> > > > > > > > > > ff,tc=0/0,ttl=0/0x0,bos=1/1),
> > > > > > > > > > packets:64, bytes:5504, used:0.444s,
> > > > > > > > > > actions:pop_mpls(eth_type=0x806),recirc(0x80d)
> > > > > > > > > > recirc_id(0x80d),in_port(2),eth(src=00:00:00:00:00:01,dst=00:00:00:
> > > > > > > > > > 00:00:02),eth_type(0x0806), packets:64,
> > > > > > > > > > bytes:5248, used:0.444s,
> > > > > > > > > > actions:3,1
> > > > > > > > > >
> > > > > > > > > > ofctl:
> > > > > > > > > >
> > > > > > > > > > OFPST_FLOW reply (OF1.5) (xid=0x2):
> > > > > > > > > > cookie=0x0, duration=178.890s, table=0, n_packets=127,
> > > > > > > > > > n_bytes=10922, idle_age=0, priority=100,mpls,mpls_label=100
> > > > > > > > > > actions=pop_mpls:0x0806,resubmit(,3)
> > > > > > > > > > cookie=0x0, duration=178.873s, table=3, n_packets=127,
> > > > > > > > > > n_bytes=10414, idle_age=0, priority=10 actions=NORMAL
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > > > Thanks for your time.
> > > > > > > > > > > >
> > > > > > > > > > > > Your welcome
> > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > If I use the old way, doing pop_mpls, it works fine:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > ovs-ofctl del-flows ovs_pvp_br0 ovs-ofctl add-flow -O
> > > > > > > > > > > > > > > > OpenFlow13 ovs_pvp_br0
> > > > > > > > > > > > > > > > "priority=100,dl_type=0x8847,mpls_label=100
> > > > > > > > > > > > > > > > actions=pop_mpls:0x0806,resubmit(,3)"
> > > > > > > > > > > > > > > > ovs-ofctl add-flow -O OpenFlow13 ovs_pvp_br0
> > > > > > > > > > > > > > > > "table=3,priority=10
> > > > > > > > > > > > > > > > actions=normal"
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I also noticed
> > > > > > > > > > > > > > > > (despite the
> > > > > > > > > > > > > > > > test example) to
> > > > > > > > > > > > > > > > make encap work,
> > > > > > > > > > > > > > > > I had to set the
> > > > > > > > > > > > > > > > ethernet MAC
> > > > > > > > > > > > > > > > addresses, or
> > > > > > > > > > > > > > > > else the packets
> > > > > > > > > > > > > > > > were not getting out.
> > > > > > > > > > > > > > > > So something like:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > ovs-ofctl add-flow -O OpenFlow13
> > > > > > > > > > > > > > > > ovs_pvp_br0
> > > > > > > > > > > > > > > >
> > > > "priority=100,in_port=vnet0,actions=encap(mpls(ether_type=0x8
> > > > > > > > > > > > > > > > 847)),set_mpls_label:100,encap(ethernet),,set_field:00:00:00:
> > > > > > > > > > > > > > > > 00:00:02->dl_dst,set_field:00:00:00:00:00:01-
> > > > > dl_src,output:e
> > > > > > > > > > > > > > > > np5s0f0
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > The packets are not going out because you are sending the
> > > > > > > > > > > > > > > packet on a real nic
> > > > > > > > > > > > > > > and not on a virtual
> > > > > > > > > > > > > > > inerface (veth pair)
> > > > > > > > > > > > > > > ?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > So for a real NIC we
> > > > > > > > > > > > > > need to set the MAC
> > > > > > > > > > > > > > addresses, maybe some
> > > > > > > > > > > > > > where in the documentation we should add an example on how
> > > > to
> > > > > > > > > > > > > > use this feature?
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Maybe the test case can be made more realistic? Once I
> > > > > > > > > > > > > > > > understand the failure, I can continue with the review.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Cheers,
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Eelco
> > > > >
>
More information about the dev
mailing list