[ovs-dev] [PATCH v4 1/2] Encap & Decap actions for MPLS packet type.

Eelco Chaudron echaudro at redhat.com
Mon Jun 21 07:36:50 UTC 2021



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 <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

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 <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