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

2021-06-21 Thread Eelco Chaudron


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 +, 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 
> Sent: Wednesday, 7 April, 2021 10:43
> To: Jan Scheurich 
> Cc: Eelco Chaudron ; d...@openvswitch.org;
> pshe...@ovn.org; martin.vargh...@nokia.com
> Subject: Re: [PATCH v4 1/2] Encap & Decap actions for MPLS
> packet type.
>
> On Tue, Apr 06, 2021 at 09:00:16AM +, Jan Scheurich wrote:
>> Hi,
>>
>> Thanks for the heads up. The interaction with MPLS push/pop
>> is a use case
> that was likely not tested during the NSH and generic
> encap/decap design. It's
> complex code and a long time ago. I'm willing to help, but I
> will need some
> time to go back and have a look.
>>
>> It would definitely help, if you could provide a minimal
>> example for
> reproducing the problem.
>>
>
> Hi Jan ,
>
> Thanks for your help.
>
> I was trying to implement ENCAP/DECAP support for MPLS.
>
> The programming of datapath flow for the below  userspace rule
> fails as there
> is set(eth() action between pop_mpls and recirc ovs-ofctl -O
> OpenFlow13 add-
> flow br_mpls2 "in_port=$egress_port,dl_type=0x8847
> actions=decap(),decap(packet_type(ns=0,type=0),goto_table:1
>
> 2021-04-05T05:46:49.192Z|00068|dpif(handler51)|WARN|system@ovs-
> system: failed to put[create] (Invalid argument)
> ufid:1dddb0ba-27fe-44ea-
> 9a99-5815764b4b9c
> recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(6),skb_mark(0/0),ct_state
> (0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=00:00:00:00:00:01/00:
> 00:00:00:00:00,dst=00:00:00:00:00:02/00:00:00:00:00:00),eth_type(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 

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

2021-06-19 Thread Martin Varghese
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 +, 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 
> > > > Sent: Wednesday, 7 April, 2021 10:43
> > > > To: Jan Scheurich 
> > > > Cc: Eelco Chaudron ; d...@openvswitch.org;
> > > > pshe...@ovn.org; martin.vargh...@nokia.com
> > > > Subject: Re: [PATCH v4 1/2] Encap & Decap actions for MPLS
> > > > packet type.
> > > > 
> > > > On Tue, Apr 06, 2021 at 09:00:16AM +, Jan Scheurich wrote:
> > > > > Hi,
> > > > > 
> > > > > Thanks for the heads up. The interaction with MPLS push/pop
> > > > > is a use case
> > > > that was likely not tested during the NSH and generic
> > > > encap/decap design. It's
> > > > complex code and a long time ago. I'm willing to help, but I
> > > > will need some
> > > > time to go back and have a look.
> > > > > 
> > > > > It would definitely help, if you could provide a minimal
> > > > > example for
> > > > reproducing the problem.
> > > > > 
> > > > 
> > > > Hi Jan ,
> > > > 
> > > > Thanks for your help.
> > > > 
> > > > I was trying to implement ENCAP/DECAP support for MPLS.
> > > > 
> > > > The programming of datapath flow for the below  userspace rule
> > > > fails as there
> > > > is set(eth() action between pop_mpls and recirc ovs-ofctl -O
> > > > OpenFlow13 add-
> > > > flow br_mpls2 "in_port=$egress_port,dl_type=0x8847
> > > > actions=decap(),decap(packet_type(ns=0,type=0),goto_table:1
> > > > 
> > > > 2021-04-05T05:46:49.192Z|00068|dpif(handler51)|WARN|system@ovs-
> > > > system: failed to put[create] (Invalid argument)
> > > > ufid:1dddb0ba-27fe-44ea-
> > > > 9a99-5815764b4b9c
> > > > recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(6),skb_mark(0/0),ct_state
> > > > (0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=00:00:00:00:00:01/00:
> > > > 00:00:00:00:00,dst=00:00:00:00:00:02/00:00:00:00:00:00),eth_type(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 

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

2021-04-17 Thread Jan Scheurich via dev


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

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

2021-04-13 Thread Martin Varghese
On Wed, Apr 07, 2021 at 03:49:07PM +, Jan Scheurich wrote:
> Hi Martin,
> 
> I guess you are aware of the original design document we wrote for generic 
> encap/decap and NSH support:
> https://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 
> > Sent: Wednesday, 7 April, 2021 10:43
> > To: Jan Scheurich 
> > Cc: Eelco Chaudron ; d...@openvswitch.org;
> > pshe...@ovn.org; martin.vargh...@nokia.com
> > Subject: Re: [PATCH v4 1/2] Encap & Decap actions for MPLS packet type.
> > 
> > On Tue, Apr 06, 2021 at 09:00:16AM +, Jan Scheurich wrote:
> > > Hi,
> > >
> > > Thanks for the heads up. The interaction with MPLS push/pop is a use case
> > that was likely not tested during the NSH and generic encap/decap design. 
> > It's
> > complex code and a long time ago. I'm willing to help, but I will need some
> > time to go back and have a look.
> > >
> > > It would definitely help, if you could provide a minimal example for
> > reproducing the problem.
> > >
> > 
> > Hi Jan ,
> > 
> > Thanks for your help.
> > 
> > I was trying to implement ENCAP/DECAP support for MPLS.
> > 
> > The programming of datapath flow for the below  userspace rule fails as 
> > there
> > is set(eth() action between pop_mpls and recirc ovs-ofctl -O OpenFlow13 add-
> > flow br_mpls2 "in_port=$egress_port,dl_type=0x8847
> > actions=decap(),decap(packet_type(ns=0,type=0),goto_table:1
> > 
> > 2021-04-05T05:46:49.192Z|00068|dpif(handler51)|WARN|system@ovs-
> > system: failed to put[create] (Invalid argument) ufid:1dddb0ba-27fe-44ea-
> > 9a99-5815764b4b9c
> > recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(6),skb_mark(0/0),ct_state
> > (0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=00:00:00:00:00:01/00:
> > 00:00:00:00:00,dst=00:00:00:00:00:02/00:00:00:00:00:00),eth_type(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? 
> 
> 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;
>  }
> 

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

2021-04-08 Thread Eelco Chaudron



On 8 Apr 2021, at 14:05, Martin Varghese wrote:


On Wed, Apr 07, 2021 at 03:49:07PM +, Jan Scheurich wrote:

Hi Martin,

I guess you are aware of the original design document we wrote for 
generic encap/decap and NSH support:

https://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 
Sent: Wednesday, 7 April, 2021 10:43
To: Jan Scheurich 
Cc: Eelco Chaudron ; d...@openvswitch.org;
pshe...@ovn.org; martin.vargh...@nokia.com
Subject: Re: [PATCH v4 1/2] Encap & Decap actions for MPLS packet 
type.


On Tue, Apr 06, 2021 at 09:00:16AM +, Jan Scheurich wrote:

Hi,

Thanks for the heads up. The interaction with MPLS push/pop is a 
use case
that was likely not tested during the NSH and generic encap/decap 
design. It's
complex code and a long time ago. I'm willing to help, but I will 
need some

time to go back and have a look.


It would definitely help, if you could provide a minimal example 
for

reproducing the problem.




Hi Jan ,

Thanks for your help.

I was trying to implement ENCAP/DECAP support for MPLS.

The programming of datapath flow for the below  userspace rule fails 
as there
is set(eth() action between pop_mpls and recirc ovs-ofctl -O 
OpenFlow13 add-

flow br_mpls2 "in_port=$egress_port,dl_type=0x8847
actions=decap(),decap(packet_type(ns=0,type=0),goto_table:1

2021-04-05T05:46:49.192Z|00068|dpif(handler51)|WARN|system@ovs-
system: failed to put[create] (Invalid argument) 
ufid:1dddb0ba-27fe-44ea-

9a99-5815764b4b9c
recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(6),skb_mark(0/0),ct_state
(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=00:00:00:00:00:01/00:
00:00:00:00:00,dst=00:00:00:00:00:02/00:00:00:00:00:00),eth_type(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 |= 

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

2021-04-08 Thread Martin Varghese
On Wed, Apr 07, 2021 at 03:49:07PM +, Jan Scheurich wrote:
> Hi Martin,
> 
> I guess you are aware of the original design document we wrote for generic 
> encap/decap and NSH support:
> https://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 
> > Sent: Wednesday, 7 April, 2021 10:43
> > To: Jan Scheurich 
> > Cc: Eelco Chaudron ; d...@openvswitch.org;
> > pshe...@ovn.org; martin.vargh...@nokia.com
> > Subject: Re: [PATCH v4 1/2] Encap & Decap actions for MPLS packet type.
> > 
> > On Tue, Apr 06, 2021 at 09:00:16AM +, Jan Scheurich wrote:
> > > Hi,
> > >
> > > Thanks for the heads up. The interaction with MPLS push/pop is a use case
> > that was likely not tested during the NSH and generic encap/decap design. 
> > It's
> > complex code and a long time ago. I'm willing to help, but I will need some
> > time to go back and have a look.
> > >
> > > It would definitely help, if you could provide a minimal example for
> > reproducing the problem.
> > >
> > 
> > Hi Jan ,
> > 
> > Thanks for your help.
> > 
> > I was trying to implement ENCAP/DECAP support for MPLS.
> > 
> > The programming of datapath flow for the below  userspace rule fails as 
> > there
> > is set(eth() action between pop_mpls and recirc ovs-ofctl -O OpenFlow13 add-
> > flow br_mpls2 "in_port=$egress_port,dl_type=0x8847
> > actions=decap(),decap(packet_type(ns=0,type=0),goto_table:1
> > 
> > 2021-04-05T05:46:49.192Z|00068|dpif(handler51)|WARN|system@ovs-
> > system: failed to put[create] (Invalid argument) ufid:1dddb0ba-27fe-44ea-
> > 9a99-5815764b4b9c
> > recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(6),skb_mark(0/0),ct_state
> > (0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=00:00:00:00:00:01/00:
> > 00:00:00:00:00,dst=00:00:00:00:00:02/00:00:00:00:00:00),eth_type(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.
> 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 |= 

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

2021-04-07 Thread Jan Scheurich via dev
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 
> Sent: Wednesday, 7 April, 2021 10:43
> To: Jan Scheurich 
> Cc: Eelco Chaudron ; d...@openvswitch.org;
> pshe...@ovn.org; martin.vargh...@nokia.com
> Subject: Re: [PATCH v4 1/2] Encap & Decap actions for MPLS packet type.
> 
> On Tue, Apr 06, 2021 at 09:00:16AM +, Jan Scheurich wrote:
> > Hi,
> >
> > Thanks for the heads up. The interaction with MPLS push/pop is a use case
> that was likely not tested during the NSH and generic encap/decap design. It's
> complex code and a long time ago. I'm willing to help, but I will need some
> time to go back and have a look.
> >
> > It would definitely help, if you could provide a minimal example for
> reproducing the problem.
> >
> 
> Hi Jan ,
> 
> Thanks for your help.
> 
> I was trying to implement ENCAP/DECAP support for MPLS.
> 
> The programming of datapath flow for the below  userspace rule fails as there
> is set(eth() action between pop_mpls and recirc ovs-ofctl -O OpenFlow13 add-
> flow br_mpls2 "in_port=$egress_port,dl_type=0x8847
> actions=decap(),decap(packet_type(ns=0,type=0),goto_table:1
> 
> 2021-04-05T05:46:49.192Z|00068|dpif(handler51)|WARN|system@ovs-
> system: failed to put[create] (Invalid argument) ufid:1dddb0ba-27fe-44ea-
> 9a99-5815764b4b9c
> recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(6),skb_mark(0/0),ct_state
> (0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=00:00:00:00:00:01/00:
> 00:00:00:00:00,dst=00:00:00:00:00:02/00:00:00:00:00:00),eth_type(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? 

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, ) you would set 
the 

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

2021-04-07 Thread Martin Varghese
On Tue, Apr 06, 2021 at 09:00:16AM +, Jan Scheurich wrote:
> Hi,
> 
> Thanks for the heads up. The interaction with MPLS push/pop is a use case 
> that was likely not tested during the NSH and generic encap/decap design. 
> It's complex code and a long time ago. I'm willing to help, but I will need 
> some time to go back and have a look.
> 
> It would definitely help, if you could provide a minimal example for 
> reproducing the problem.
> 

Hi Jan ,

Thanks for your help.

I was trying to implement ENCAP/DECAP support for MPLS.

The programming of datapath flow for the below  userspace rule fails as there 
is set(eth() action between pop_mpls and recirc
ovs-ofctl -O OpenFlow13 add-flow br_mpls2 "in_port=$egress_port,dl_type=0x8847 
actions=decap(),decap(packet_type(ns=0,type=0),goto_table:1

2021-04-05T05:46:49.192Z|00068|dpif(handler51)|WARN|system@ovs-system: failed 
to put[create] (Invalid argument) ufid:1dddb0ba-27fe-44ea-9a99-5815764b4b9c 
recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(6),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=00:00:00:00:00:01/00:00:00:00:00:00,dst=00:00:00:00:00:02/00:00:00:00:00:00),eth_type(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)

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))”.

Though both of them solves this problem, couple of NSH regression tests are 
failing

2291: nsh - md1 encap over a veth linkFAILED (nsh.at:85)
58022292: nsh - md2 encap over a veth linkFAILED (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,c2=0x0,c3=0x0,c4=0x0),push_eth(src=00:00:00:00:00:00,dst=11:22:33:44:55: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
table=0,in_port=4,dl_type=0x894f,nsh_mdtype=1,nsh_spi=0x1234,nsh_c1=0x11223344,actions=decap(),decap(),2.

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),output: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 
> > Sent: Tuesday, 6 April, 2021 10:55
> > To: Martin Varghese ; Jan Scheurich
> > 
> > Cc: d...@openvswitch.org; pshe...@ovn.org; martin.vargh...@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 
> > >>>
> > >>> 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 

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

2021-04-06 Thread Martin Varghese
On Tue, Apr 06, 2021 at 10:54:32AM +0200, Eelco Chaudron wrote:
> 
> 
> 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 
> > > > > > > > > > > > 
> > > > > > > > > > > > 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
> > > > > > > > > > >    0110 0100    = MPLS Label: 
> > > > > > > > > > > 100
> > > > > > > > > > >      000.
> > > > > > > > > > >   = MPLS Experimental
> > > > > > > > > > > Bits: 0
> > > > > > > > > > >      ...1   = MPLS
> > > > > > > > > > > Bottom Of Label
> > > > > > > > > > > Stack: 1
> > > > > > > > > > >       0100  = MPLS TTL: 64
> > > > > > > > > > > Data (46 bytes)
> > > > > > > > > > > 
> > > > > > > > > > >   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:
> > > > > > > > > > > 5254008851380806000108000604000152540088513801010165?
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 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 

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

2021-04-06 Thread Jan Scheurich via dev
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.

BR, Jan

> -Original Message-
> From: Eelco Chaudron 
> Sent: Tuesday, 6 April, 2021 10:55
> To: Martin Varghese ; Jan Scheurich
> 
> Cc: d...@openvswitch.org; pshe...@ovn.org; martin.vargh...@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 
> >>>
> >>> 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
> >>    0110 0100    = MPLS Label: 100
> >>      000.   = MPLS
> >> Experimental
> >> Bits: 0
> >>      ...1   = MPLS Bottom Of
> >> Label
> >> Stack: 1
> >>       0100  = MPLS TTL: 64
> >> Data (46 bytes)
> >>
> >>   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:
> >>
> 52540088513808060001080006040001525400885138010101650
> 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
> >>
> >> 

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

2021-04-06 Thread Eelco Chaudron



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 

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
   0110 0100    = MPLS Label: 100
     000.   = MPLS 
Experimental

Bits: 0
     ...1   = MPLS
Bottom Of Label
Stack: 1
      0100  = MPLS TTL: 64
Data (46 bytes)

  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:
5254008851380806000108000604000152540088513801010165?


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/0xf,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/0xf,tc=0/0,ttl=64/0x0,bos=1/1),

actions:pop_eth,pop_mpls(eth_type=0x6558),set(eth()),recirc(0x4c)


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 

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

2021-04-06 Thread Martin Varghese
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 
> > > > > > > > > > 
> > > > > > > > > > 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
> > > > > > > > >    0110 0100    = MPLS Label: 100
> > > > > > > > >      000.   = MPLS 
> > > > > > > > > Experimental
> > > > > > > > > Bits: 0
> > > > > > > > >      ...1   = MPLS
> > > > > > > > > Bottom Of Label
> > > > > > > > > Stack: 1
> > > > > > > > >       0100  = MPLS TTL: 64
> > > > > > > > > Data (46 bytes)
> > > > > > > > > 
> > > > > > > > >   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:
> > > > > > > > > 5254008851380806000108000604000152540088513801010165?
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 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/0xf,tc=0/0,ttl=0/0x0,bos=1/1),
> > > > > > > > > packets:13, bytes:1118, used:0.322s,
> > > > > > > > > 

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

2021-04-06 Thread Eelco Chaudron



On 1 Apr 2021, at 11:32, 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 

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
   0110 0100    = MPLS Label: 100
     000.   = MPLS 
Experimental

Bits: 0
     ...1   = MPLS
Bottom Of Label
Stack: 1
      0100  = MPLS TTL: 64
Data (46 bytes)

  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:
5254008851380806000108000604000152540088513801010165?


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/0xf,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/0xf,tc=0/0,ttl=64/0x0,bos=1/1),

actions:pop_eth,pop_mpls(eth_type=0x6558),set(eth()),recirc(0x4c)


This set(eth) before the recirc is the problem i guesss. I need
to check

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=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00: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 

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

2021-04-01 Thread Eelco Chaudron



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 

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
   0110 0100    = MPLS Label: 100
     000.   = MPLS Experimental
Bits: 0
     ...1   = MPLS
Bottom Of Label
Stack: 1
      0100  = MPLS TTL: 64
Data (46 bytes)

  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:
5254008851380806000108000604000152540088513801010165?


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/0xf,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/0xf,tc=0/0,ttl=64/0x0,bos=1/1),

actions:pop_eth,pop_mpls(eth_type=0x6558),set(eth()),recirc(0x4c)


This set(eth) before the recirc is the problem i guesss. I need
to check

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=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00: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 

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

2021-04-01 Thread Martin Varghese
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 
> > > > > > > > 
> > > > > > > > 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
> > > > > > >    0110 0100    = MPLS Label: 100
> > > > > > >      000.   = MPLS Experimental
> > > > > > > Bits: 0
> > > > > > >      ...1   = MPLS
> > > > > > > Bottom Of Label
> > > > > > > Stack: 1
> > > > > > >       0100  = MPLS TTL: 64
> > > > > > > Data (46 bytes)
> > > > > > > 
> > > > > > >   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:
> > > > > > > 5254008851380806000108000604000152540088513801010165?
> > > > > > > 
> > > > > > > 
> > > > > > > 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/0xf,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
> > > > > > > 
> > > 

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

2021-04-01 Thread Eelco Chaudron



On 1 Apr 2021, at 11:17, 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 

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
   0110 0100    = MPLS Label: 100
     000.   = MPLS Experimental
Bits: 0
     ...1   = MPLS Bottom Of 
Label

Stack: 1
      0100  = MPLS TTL: 64
Data (46 bytes)

  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:
5254008851380806000108000604000152540088513801010165?


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/0xf,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/0xf,tc=0/0,ttl=64/0x0,bos=1/1),

actions:pop_eth,pop_mpls(eth_type=0x6558),set(eth()),recirc(0x4c)


This set(eth) before the recirc is the problem i guesss. I need to 
check

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=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00: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

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

2021-04-01 Thread Eelco Chaudron



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 

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
   0110 0100    = MPLS Label: 100
     000.   = MPLS Experimental
Bits: 0
     ...1   = MPLS Bottom Of 
Label

Stack: 1
      0100  = MPLS TTL: 64
Data (46 bytes)

  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:
5254008851380806000108000604000152540088513801010165?


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/0xf,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/0xf,tc=0/0,ttl=64/0x0,bos=1/1),

actions:pop_eth,pop_mpls(eth_type=0x6558),set(eth()),recirc(0x4c)


This set(eth) before the recirc is the problem i guesss. I need to 
check

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=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00: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

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

2021-04-01 Thread Martin Varghese
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 
> > > > > > 
> > > > > > 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
> > > > >    0110 0100    = MPLS Label: 100
> > > > >      000.   = MPLS Experimental
> > > > > Bits: 0
> > > > >      ...1   = MPLS Bottom Of Label
> > > > > Stack: 1
> > > > >       0100  = MPLS TTL: 64
> > > > > Data (46 bytes)
> > > > > 
> > > > >   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:
> > > > > 5254008851380806000108000604000152540088513801010165?
> > > > > 
> > > > > 
> > > > > 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/0xf,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 

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

2021-04-01 Thread Eelco Chaudron



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 

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
   0110 0100    = MPLS Label: 100
     000.   = MPLS Experimental
Bits: 0
     ...1   = MPLS Bottom Of Label
Stack: 1
      0100  = MPLS TTL: 64
Data (46 bytes)

  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:
5254008851380806000108000604000152540088513801010165?


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/0xf,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/0xf,tc=0/0,ttl=64/0x0,bos=1/1),

actions:pop_eth,pop_mpls(eth_type=0x6558),set(eth()),recirc(0x4c)


This set(eth) before the recirc is the problem i guesss. I need to 
check

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=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00: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 

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

2021-04-01 Thread Martin Varghese
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 
> > > > 
> > > > 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
> > >    0110 0100    = MPLS Label: 100
> > >      000.   = MPLS Experimental
> > > Bits: 0
> > >      ...1   = MPLS Bottom Of Label
> > > Stack: 1
> > >       0100  = MPLS TTL: 64
> > > Data (46 bytes)
> > > 
> > >   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:
> > > 5254008851380806000108000604000152540088513801010165?
> > > 
> > > 
> > > 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/0xf,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/0xf,tc=0/0,ttl=64/0x0,bos=1/1),
> actions:pop_eth,pop_mpls(eth_type=0x6558),set(eth()),recirc(0x4c)

This set(eth) before the recirc is the problem i guesss. I need to check
> 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 

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

2021-04-01 Thread Eelco Chaudron



On 1 Apr 2021, at 8:59, 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 

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

   0110 0100    = MPLS Label: 100
     000.   = MPLS Experimental 
Bits: 0
     ...1   = MPLS Bottom Of Label 
Stack: 1

      0100  = MPLS TTL: 64
Data (46 bytes)

  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:
5254008851380806000108000604000152540088513801010165?


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/0xf,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/0xf,tc=0/0,ttl=64/0x0,bos=1/1), 
actions:pop_eth,pop_mpls(eth_type=0x6558),set(eth()),recirc(0x4c)
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=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00: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?


Just to be sure I build the “lastest” net kernel, e43accba9b07, 
5.12.0-rc2+, and I see the same problem.



$ 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 

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

2021-04-01 Thread Eelco Chaudron



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 

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

   0110 0100    = MPLS Label: 100
     000.   = MPLS Experimental Bits: 
0
     ...1   = MPLS Bottom Of Label 
Stack: 1

      0100  = MPLS TTL: 64
Data (46 bytes)

  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:
5254008851380806000108000604000152540088513801010165?


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/0xf,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/0xf,tc=0/0,ttl=64/0x0,bos=1/1), 
actions:pop_eth,pop_mpls(eth_type=0x6558),set(eth()),recirc(0x4c)
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=0x,dl_src=00:00:00:00:00:01,dl_dst=00:00:00:00: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 

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

2021-03-31 Thread Martin Varghese
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 
> > 
> > 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
>    0110 0100    = MPLS Label: 100
>      000.   = MPLS Experimental Bits: 0
>      ...1   = MPLS Bottom Of Label Stack: 1
>       0100  = MPLS TTL: 64
> Data (46 bytes)
> 
>   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:
> 5254008851380806000108000604000152540088513801010165?
> 
> 
> 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/0xf,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)
?

> 
> 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=0x8847)),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:enp5s0f0
> 

The packets are not going out because you are sending the packet on a
real nic and not on a virtual inerface (veth pair) ?

> Maybe the test case can be made more realistic? Once I understand the
> failure, I can continue with the review.
> 
> 
> Cheers,
> 
> Eelco
> 
> 
> 
> > Signed-off-by: Martin Varghese 
> > ---
> >  NEWS  |  2 +-
> >  .../linux/compat/include/linux/openvswitch.h  | 35 ++-
> >  include/openvswitch/ofp-ed-props.h| 18 
> >  lib/dpif-netdev.c |  1 +
> >  lib/dpif.c|  1 +
> >  lib/odp-execute.c | 12 +++
> >  lib/odp-util.c| 58 +---
> >  lib/ofp-actions.c |  5 +
> >  lib/ofp-ed-props.c| 91 +++
> 

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

2021-03-31 Thread Eelco Chaudron



On 26 Mar 2021, at 7:21, Martin Varghese wrote:


From: Martin Varghese 

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
   0110 0100    = MPLS Label: 100
     000.   = MPLS Experimental Bits: 0
     ...1   = MPLS Bottom Of Label 
Stack: 1

      0100  = MPLS TTL: 64
Data (46 bytes)

  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: 
5254008851380806000108000604000152540088513801010165?



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/0xf,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



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=0x8847)),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:enp5s0f0


Maybe the test case can be made more realistic? Once I understand the 
failure, I can continue with the review.



Cheers,

Eelco




Signed-off-by: Martin Varghese 
---
 NEWS  |  2 +-
 .../linux/compat/include/linux/openvswitch.h  | 35 ++-
 include/openvswitch/ofp-ed-props.h| 18 
 lib/dpif-netdev.c |  1 +
 lib/dpif.c|  1 +
 lib/odp-execute.c | 12 +++
 lib/odp-util.c| 58 +---
 lib/ofp-actions.c |  5 +
 lib/ofp-ed-props.c| 91 
+++

 lib/ovs-actions.xml   | 31 +--
 lib/packets.c | 36 
 lib/packets.h |  2 +
 ofproto/ofproto-dpif-ipfix.c  |  1 +
 ofproto/ofproto-dpif-sflow.c  |  1 +
 ofproto/ofproto-dpif-xlate.c  | 60 
 ofproto/ofproto-dpif.c| 39 
 ofproto/ofproto-dpif.h|  5 +-
 

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

2021-03-26 Thread Martin Varghese
From: Martin Varghese 

The encap & decap actions are extended to support MPLS packet type.
Encap & decap actions adds and removes MPLS header at start of the
packet.

Signed-off-by: Martin Varghese 
---
 NEWS  |  2 +-
 .../linux/compat/include/linux/openvswitch.h  | 35 ++-
 include/openvswitch/ofp-ed-props.h| 18 
 lib/dpif-netdev.c |  1 +
 lib/dpif.c|  1 +
 lib/odp-execute.c | 12 +++
 lib/odp-util.c| 58 +---
 lib/ofp-actions.c |  5 +
 lib/ofp-ed-props.c| 91 +++
 lib/ovs-actions.xml   | 31 +--
 lib/packets.c | 36 
 lib/packets.h |  2 +
 ofproto/ofproto-dpif-ipfix.c  |  1 +
 ofproto/ofproto-dpif-sflow.c  |  1 +
 ofproto/ofproto-dpif-xlate.c  | 60 
 ofproto/ofproto-dpif.c| 39 
 ofproto/ofproto-dpif.h|  5 +-
 tests/system-traffic.at   | 36 
 18 files changed, 410 insertions(+), 24 deletions(-)

diff --git a/NEWS b/NEWS
index 95cf922aa..4bf4e9e7b 100644
--- a/NEWS
+++ b/NEWS
@@ -120,7 +120,7 @@ v2.14.0 - 17 Aug 2020
- GTP-U Tunnel Protocol
  * Add two new fields: tun_gtpu_flags, tun_gtpu_msgtype.
  * Only support for userspace datapath.
-
+   - Encap & Decap action support for MPLS packet type.
 
 v2.13.0 - 14 Feb 2020
 -
diff --git a/datapath/linux/compat/include/linux/openvswitch.h 
b/datapath/linux/compat/include/linux/openvswitch.h
index 875de2025..8feea7dd4 100644
--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -810,8 +810,32 @@ struct ovs_action_push_tnl {
 };
 #endif
 
-/**
- * enum ovs_ct_attr - Attributes for %OVS_ACTION_ATTR_CT action.
+/* struct ovs_action_add_mpls - %OVS_ACTION_ATTR_ADD_MPLS action
+ * argument.
+ * @mpls_lse: MPLS label stack entry to push.
+ * @mpls_ethertype: Ethertype to set in the encapsulating ethernet frame.
+ * @tun_flags: MPLS tunnel attributes.
+ *
+ * The only values @mpls_ethertype should ever be given are %ETH_P_MPLS_UC and
+ * %ETH_P_MPLS_MC, indicating MPLS unicast or multicast. Other are rejected.
+ */
+struct ovs_action_add_mpls {
+   __be32 mpls_lse;
+   __be16 mpls_ethertype; /* Either %ETH_P_MPLS_UC or %ETH_P_MPLS_MC */
+   __u16 tun_flags;
+};
+
+#define OVS_MPLS_L3_TUNNEL_FLAG_MASK  (1 << 0) /* Flag to specify the place of
+   * insertion of MPLS header.
+   * When false, the MPLS header
+   * will be inserted at the start
+   * of the packet.
+   * When true, the MPLS header
+   * will be inserted at the start
+   * of the l3 header.
+   */
+
+/* enum ovs_ct_attr - Attributes for %OVS_ACTION_ATTR_CT action.
  * @OVS_CT_ATTR_COMMIT: If present, commits the connection to the conntrack
  * table. This allows future packets for the same connection to be identified
  * as 'established' or 'related'. The flow key for the current packet will
@@ -1001,7 +1025,11 @@ struct check_pkt_len_arg {
  * @OVS_ACTION_ATTR_CHECK_PKT_LEN: Check the packet length and execute a set
  * of actions if greater than the specified packet length, else execute
  * another set of actions.
- * @OVS_ACTION_ATTR_DROP: Explicit drop action.
+ * @OVS_ACTION_ATTR_ADD_MPLS: Push a new MPLS label stack entry at the
+ * start of the packet or at the start of the l3 header depending on the value
+ * of l3 tunnel flag in the tun_flags field of OVS_ACTION_ATTR_ADD_MPLS
+ * argument.
+  * @OVS_ACTION_ATTR_DROP: Explicit drop action.
  */
 
 enum ovs_action_attr {
@@ -1030,6 +1058,7 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_METER,/* u32 meter number. */
OVS_ACTION_ATTR_CLONE,/* Nested OVS_CLONE_ATTR_*.  */
OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
+   OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */
 
 #ifndef __KERNEL__
OVS_ACTION_ATTR_TUNNEL_PUSH,   /* struct ovs_action_push_tnl*/
diff --git a/include/openvswitch/ofp-ed-props.h 
b/include/openvswitch/ofp-ed-props.h
index 306c6fe73..c85f3c283 100644
--- a/include/openvswitch/ofp-ed-props.h
+++ b/include/openvswitch/ofp-ed-props.h
@@ -46,6 +46,11 @@ enum ofp_ed_nsh_prop_type {
 OFPPPT_PROP_NSH_TLV = 2, /* property TLV in NSH */
 };
 
+enum ofp_ed_mpls_prop_type {
+OFPPPT_PROP_MPLS_NONE = 0,