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 <[email protected]> > > > > Sent: Wednesday, 7 April, 2021 10:43 > > > > To: Jan Scheurich <[email protected]> > > > > Cc: Eelco Chaudron <[email protected]>; [email protected]; > > > > [email protected]; [email protected] > > > > 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@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 <[email protected]> > > > > > > Sent: Tuesday, 6 April, 2021 10:55 > > > > > > To: Martin Varghese <[email protected]>; Jan Scheurich > > > > > > <[email protected]> > > > > > > Cc: [email protected]; [email protected]; > > > > > > [email protected] > > > > > > 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 > > > > > > > > > > > > > > > > > <[email protected]> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 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@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@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 > > > > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
