On 19 Jun 2021, at 8:06, Martin Varghese wrote:
> 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 Cool, I will continue testing/reviewing with the next revision once all depending fixes are in. >>> +++ 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? I’m suggesting explaining what those two fields mean to this specific action. What happens if you specify the packet type? Are only those packets encapsulated/decapsulated, or is it used for encapsulating? Same for name_space. >> >>>>> >>>>> 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
