[PATCH net-next 0/3] New openvswitch MPLS actions for layer 2 tunnelling

[PATCH net-next 0/3] New openvswitch MPLS actions for layer 2 tunnelling

[Martin Varghese]

From: Martin Varghese <martin.varghese@nokia.com>

The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS header
between ethernet header and the IP header. Though this behaviour is fine
for L3 VPN where an IP packet is encapsulated inside a MPLS tunnel, it
does not suffice the L2 VPN (l2 tunnelling) requirements. In L2 VPN
the MPLS header should encapsulate the ethernet packet.

The new mpls actions PTAP_PUSH_MPLS & PTAP_POP_MPLS inserts and removes
MPLS header from start of the packet respectively.

PTAP_PUSH_MPLS - Inserts MPLS header at the start of the packet.
@ethertype - Ethertype of MPLS header. 0x8847 for unicast,0x8848 for multicast.

PTAP_POP_MPLS - Removes MPLS header from the start of the packet.
@ethertype - Ethertype of next header following the popped MPLS header.
             Value 0 in ethertype indicates the tunnelled packet is
             ethernet.

OVS userspace changes -
---------------------
Encap & Decap ovs actions are extended to support MPLS packet type. The encap & decap
adds and removes MPLS header at the start of packet as depicted below.

Actions - encap(mpls(ether_type=0x8847)),encap(ethernet)

Incoming packet -> | ETH | IP | Payload |

1 Actions -  encap(mpls(ether_type=0x8847)) [Kernel action - ptap_push_mpls:0x8847]

        Outgoing packet -> | MPLS | ETH | Payload|

2 Actions - encap(ethernet) [ Kernel action - push_eth ]

        Outgoing packet -> | ETH | MPLS | ETH | Payload|

Decapsulation:

Incoming packet -> | ETH | MPLS | ETH | IP | Payload |

Actions - decap(),decap(packet_type(ns=0,type=0)

1 Actions -  decap() [Kernel action - pop_eth)

        Outgoing packet -> | MPLS | ETH | IP | Payload|

2 Actions - decap(packet_type(ns=0,type=0) [Kernel action - ptap_pop_mpls:0]

        Outgoing packet -> | ETH  | IP | Payload

Martin Varghese (3):
  net: skb_mpls_push() modified to allow MPLS header push at start of
    packet.
  net: Rephrased comments section of skb_mpls_pop()
  openvswitch: New MPLS actions for layer 2 tunnelling

 include/uapi/linux/openvswitch.h |  2 ++
 net/core/skbuff.c                |  9 ++++++---
 net/openvswitch/actions.c        | 40 ++++++++++++++++++++++++++++++++++++++++
 net/openvswitch/flow_netlink.c   | 21 +++++++++++++++++++++
 4 files changed, 69 insertions(+), 3 deletions(-)

-- 
1.8.3.1

[PATCH net-next 1/3] net: skb_mpls_push() modified to allow MPLS header push at start of packet.

[Martin Varghese]

From: Martin Varghese <martin.varghese@nokia.com>

The existing skb_mpls_push() implementation always inserts mpls header
after the mac header. L2 VPN use cases requires MPLS header to be
inserted before the ethernet header as the ethernet packet gets tunnelled
inside MPLS header in those cases.

Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
---
 net/core/skbuff.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 973a71f..7773176 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5472,12 +5472,15 @@ static void skb_mod_eth_type(struct sk_buff *skb, struct ethhdr *hdr,
 }
 
 /**
- * skb_mpls_push() - push a new MPLS header after the mac header
+ * skb_mpls_push() - push a new MPLS header after mac_len bytes from start of
+ *                   the packet
  *
  * @skb: buffer
  * @mpls_lse: MPLS label stack entry to push
  * @mpls_proto: ethertype of the new MPLS header (expects 0x8847 or 0x8848)
  * @mac_len: length of the MAC header
+ * #ethernet: flag to indicate if the resulting packet after skb_mpls_push is
+ *            ethernet
  *
  * Expects skb->data at mac header.
  *
@@ -5501,7 +5504,7 @@ int skb_mpls_push(struct sk_buff *skb, __be32 mpls_lse, __be16 mpls_proto,
 		return err;
 
 	if (!skb->inner_protocol) {
-		skb_set_inner_network_header(skb, mac_len);
+		skb_set_inner_network_header(skb, skb_network_offset(skb));
 		skb_set_inner_protocol(skb, skb->protocol);
 	}
 
-- 
1.8.3.1

[PATCH net-next 2/3] net: Rephrased comments section of skb_mpls_pop()

[Martin Varghese]

From: Martin Varghese <martin.varghese@nokia.com>

Rephrased comments section of skb_mpls_pop() to align it with
comments section of skb_mpls_push().

Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
---
 net/core/skbuff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 7773176..024679e 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5532,7 +5532,7 @@ int skb_mpls_push(struct sk_buff *skb, __be32 mpls_lse, __be16 mpls_proto,
  * @skb: buffer
  * @next_proto: ethertype of header after popped MPLS header
  * @mac_len: length of the MAC header
- * @ethernet: flag to indicate if ethernet header is present in packet
+ * @ethernet: flag to indicate if the packet is ethernet
  *
  * Expects skb->data at mac header.
  *
-- 
1.8.3.1

[PATCH net-next 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Martin Varghese]

From: Martin Varghese <martin.varghese@nokia.com>

The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS header
between ethernet header and the IP header. Though this behaviour is fine
for L3 VPN where an IP packet is encapsulated inside a MPLS tunnel, it
does not suffice the L2 VPN (l2 tunnelling) requirements. In L2 VPN
the MPLS header should encapsulate the ethernet packet.

The new mpls actions PTAP_PUSH_MPLS & PTAP_POP_MPLS inserts and removes
MPLS header from start of the packet respectively.

PTAP_PUSH_MPLS - Inserts MPLS header at the start of the packet.
@ethertype - Ethertype of MPLS header.

PTAP_POP_MPLS - Removes MPLS header from the start of the packet.
@ethertype - Ethertype of next header following the popped MPLS header.
             Value 0 in ethertype indicates the tunnelled packet is
             ethernet.

Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
---
 include/uapi/linux/openvswitch.h |  2 ++
 net/openvswitch/actions.c        | 40 ++++++++++++++++++++++++++++++++++++++++
 net/openvswitch/flow_netlink.c   | 21 +++++++++++++++++++++
 3 files changed, 63 insertions(+)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index a87b44c..af05062 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -927,6 +927,8 @@ enum ovs_action_attr {
 	OVS_ACTION_ATTR_METER,        /* u32 meter ID. */
 	OVS_ACTION_ATTR_CLONE,        /* Nested OVS_CLONE_ATTR_*.  */
 	OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
+	OVS_ACTION_ATTR_PTAP_PUSH_MPLS,    /* struct ovs_action_push_mpls. */
+	OVS_ACTION_ATTR_PTAP_POP_MPLS,     /* __be16 ethertype. */
 
 	__OVS_ACTION_ATTR_MAX,	      /* Nothing past this will be accepted
 				       * from userspace. */
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 4c83954..d43c37e 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -160,6 +160,38 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 			      struct sw_flow_key *key,
 			      const struct nlattr *attr, int len);
 
+static int push_ptap_mpls(struct sk_buff *skb, struct sw_flow_key *key,
+			  const struct ovs_action_push_mpls *mpls)
+{
+	int err;
+
+	err = skb_mpls_push(skb, mpls->mpls_lse, mpls->mpls_ethertype,
+			    0, false);
+	if (err)
+		return err;
+
+	key->mac_proto = MAC_PROTO_NONE;
+	invalidate_flow_key(key);
+	return 0;
+}
+
+static int ptap_pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
+			 const __be16 ethertype)
+{
+	int err;
+
+	err = skb_mpls_pop(skb, ethertype, skb->mac_len,
+			   ovs_key_mac_proto(key) == MAC_PROTO_ETHERNET);
+	if (err)
+		return err;
+
+	if (!ethertype)
+		key->mac_proto = MAC_PROTO_ETHERNET;
+
+	invalidate_flow_key(key);
+	return 0;
+}
+
 static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
 		     const struct ovs_action_push_mpls *mpls)
 {
@@ -1233,10 +1265,18 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
 			err = push_mpls(skb, key, nla_data(a));
 			break;
 
+		case OVS_ACTION_ATTR_PTAP_PUSH_MPLS:
+			err = push_ptap_mpls(skb, key, nla_data(a));
+			break;
+
 		case OVS_ACTION_ATTR_POP_MPLS:
 			err = pop_mpls(skb, key, nla_get_be16(a));
 			break;
 
+		case OVS_ACTION_ATTR_PTAP_POP_MPLS:
+			err = ptap_pop_mpls(skb, key, nla_get_be16(a));
+			break;
+
 		case OVS_ACTION_ATTR_PUSH_VLAN:
 			err = push_vlan(skb, key, nla_data(a));
 			break;
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 65c2e34..4a68aae 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -79,6 +79,8 @@ static bool actions_may_change_flow(const struct nlattr *actions)
 		case OVS_ACTION_ATTR_SET_MASKED:
 		case OVS_ACTION_ATTR_METER:
 		case OVS_ACTION_ATTR_CHECK_PKT_LEN:
+		case OVS_ACTION_ATTR_PTAP_PUSH_MPLS:
+		case OVS_ACTION_ATTR_PTAP_POP_MPLS:
 		default:
 			return true;
 		}
@@ -3005,6 +3007,8 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 			[OVS_ACTION_ATTR_METER] = sizeof(u32),
 			[OVS_ACTION_ATTR_CLONE] = (u32)-1,
 			[OVS_ACTION_ATTR_CHECK_PKT_LEN] = (u32)-1,
+			[OVS_ACTION_ATTR_PTAP_PUSH_MPLS] = sizeof(struct ovs_action_push_mpls),
+			[OVS_ACTION_ATTR_PTAP_POP_MPLS] = sizeof(__be16),
 		};
 		const struct ovs_action_push_vlan *vlan;
 		int type = nla_type(a);
@@ -3072,6 +3076,19 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 		case OVS_ACTION_ATTR_RECIRC:
 			break;
 
+		case OVS_ACTION_ATTR_PTAP_PUSH_MPLS: {
+			const struct ovs_action_push_mpls *mpls = nla_data(a);
+
+			eth_type = mpls->mpls_ethertype;
+			if (mac_proto != MAC_PROTO_NONE) {
+				mpls_label_count = 1;
+				mac_proto = MAC_PROTO_NONE;
+			} else {
+				mpls_label_count++;
+			}
+			break;
+		}
+
 		case OVS_ACTION_ATTR_PUSH_MPLS: {
 			const struct ovs_action_push_mpls *mpls = nla_data(a);
 
@@ -3092,6 +3109,10 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
 			break;
 		}
 
+		case OVS_ACTION_ATTR_PTAP_POP_MPLS:
+			if (mac_proto != MAC_PROTO_NONE)
+				return -EINVAL;
+
 		case OVS_ACTION_ATTR_POP_MPLS: {
 			__be16  proto;
 			if (vlan_tci & htons(VLAN_CFI_MASK) ||
-- 
1.8.3.1

Re: [PATCH net-next 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Jiri Benc]

On Tue, 10 Dec 2019 13:46:41 +0530, Martin Varghese wrote:
> +static int push_ptap_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> +static int ptap_pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,

The names are inconsistent (*_ptap_mpls vs. ptap_*_mpls). Otherwise,
this looks good to me.

 Jiri

Re: [PATCH net-next 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Pravin Shelar]

On Tue, Dec 10, 2019 at 12:17 AM Martin Varghese
<martinvarghesenokia@gmail.com> wrote:
>
> From: Martin Varghese <martin.varghese@nokia.com>
>
> The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS header
> between ethernet header and the IP header. Though this behaviour is fine
> for L3 VPN where an IP packet is encapsulated inside a MPLS tunnel, it
> does not suffice the L2 VPN (l2 tunnelling) requirements. In L2 VPN
> the MPLS header should encapsulate the ethernet packet.
>
> The new mpls actions PTAP_PUSH_MPLS & PTAP_POP_MPLS inserts and removes
> MPLS header from start of the packet respectively.
>
> PTAP_PUSH_MPLS - Inserts MPLS header at the start of the packet.
> @ethertype - Ethertype of MPLS header.
>
> PTAP_POP_MPLS - Removes MPLS header from the start of the packet.
> @ethertype - Ethertype of next header following the popped MPLS header.
>              Value 0 in ethertype indicates the tunnelled packet is
>              ethernet.
>
Did you considered using existing MPLS action to handle L2 tunneling
packet ? It can be done by adding another parameter to the MPLS
actions.

Re: [PATCH net-next 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Martin Varghese]

On Tue, Dec 10, 2019 at 04:11:22PM +0100, Jiri Benc wrote:
> On Tue, 10 Dec 2019 13:46:41 +0530, Martin Varghese wrote:
> > +static int push_ptap_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> > +static int ptap_pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> 
> The names are inconsistent (*_ptap_mpls vs. ptap_*_mpls). Otherwise,
> this looks good to me.
>

Thanks Jiri 

> 

Re: [PATCH net-next 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Martin Varghese]

On Tue, Dec 10, 2019 at 01:22:56PM -0800, Pravin Shelar wrote:
> On Tue, Dec 10, 2019 at 12:17 AM Martin Varghese
> <martinvarghesenokia@gmail.com> wrote:
> >
> > From: Martin Varghese <martin.varghese@nokia.com>
> >
> > The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS header
> > between ethernet header and the IP header. Though this behaviour is fine
> > for L3 VPN where an IP packet is encapsulated inside a MPLS tunnel, it
> > does not suffice the L2 VPN (l2 tunnelling) requirements. In L2 VPN
> > the MPLS header should encapsulate the ethernet packet.
> >
> > The new mpls actions PTAP_PUSH_MPLS & PTAP_POP_MPLS inserts and removes
> > MPLS header from start of the packet respectively.
> >
> > PTAP_PUSH_MPLS - Inserts MPLS header at the start of the packet.
> > @ethertype - Ethertype of MPLS header.
> >
> > PTAP_POP_MPLS - Removes MPLS header from the start of the packet.
> > @ethertype - Ethertype of next header following the popped MPLS header.
> >              Value 0 in ethertype indicates the tunnelled packet is
> >              ethernet.
> >
> Did you considered using existing MPLS action to handle L2 tunneling
> packet ? It can be done by adding another parameter to the MPLS
> actions.



Not really.

Are you suggesting to extend the ovs_action_push_mpls and similarly for pop

struct ovs_action_push_mpls {
        __be32 mpls_lse;
        __be16 mpls_ethertype; /* Either %ETH_P_MPLS_UC or %ETH_P_MPLS_MC */
+        bool l2_tunnel;
};

Does not that break the compatibilty with the existing userspace
OVS ?

Will not push_mpls and pop_mpls called from existing OVS fail in action_lens check
in __ovs_nla_copy_actions ?

Re: [PATCH net-next 1/3] net: skb_mpls_push() modified to allow MPLS header push at start of packet.

[David Miller]

From: Martin Varghese <martinvarghesenokia@gmail.com>
Date: Tue, 10 Dec 2019 13:45:52 +0530

> @@ -5472,12 +5472,15 @@ static void skb_mod_eth_type(struct sk_buff *skb, struct ethhdr *hdr,
>  }
>  
>  /**
> - * skb_mpls_push() - push a new MPLS header after the mac header
> + * skb_mpls_push() - push a new MPLS header after mac_len bytes from start of
> + *                   the packet
>   *
>   * @skb: buffer
>   * @mpls_lse: MPLS label stack entry to push
>   * @mpls_proto: ethertype of the new MPLS header (expects 0x8847 or 0x8848)
>   * @mac_len: length of the MAC header
> + * #ethernet: flag to indicate if the resulting packet after skb_mpls_push is
> + *            ethernet

Why "#ethernet" and not "@ethernet" to refer to this argument?

RE: [PATCH net-next 1/3] net: skb_mpls_push() modified to allow MPLS header push at start of packet.

[Varghese, Martin (Nokia - IN/Bangalore)]

Oops Typo, will fix it. thanks

-----Original Message-----
From: David Miller <davem@davemloft.net> 
Sent: Wednesday, December 11, 2019 7:14 AM
To: martinvarghesenokia@gmail.com
Cc: netdev@vger.kernel.org; pshelar@ovn.org; Drennan, Scott (Nokia - US/Mountain View) <scott.drennan@nokia.com>; jbenc@redhat.com; Varghese, Martin (Nokia - IN/Bangalore) <martin.varghese@nokia.com>
Subject: Re: [PATCH net-next 1/3] net: skb_mpls_push() modified to allow MPLS header push at start of packet.

From: Martin Varghese <martinvarghesenokia@gmail.com>
Date: Tue, 10 Dec 2019 13:45:52 +0530

> @@ -5472,12 +5472,15 @@ static void skb_mod_eth_type(struct sk_buff 
> *skb, struct ethhdr *hdr,  }
>  
>  /**
> - * skb_mpls_push() - push a new MPLS header after the mac header
> + * skb_mpls_push() - push a new MPLS header after mac_len bytes from start of
> + *                   the packet
>   *
>   * @skb: buffer
>   * @mpls_lse: MPLS label stack entry to push
>   * @mpls_proto: ethertype of the new MPLS header (expects 0x8847 or 0x8848)
>   * @mac_len: length of the MAC header
> + * #ethernet: flag to indicate if the resulting packet after skb_mpls_push is
> + *            ethernet

Why "#ethernet" and not "@ethernet" to refer to this argument?

Re: [PATCH net-next 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Pravin Shelar]

On Tue, Dec 10, 2019 at 4:02 PM Martin Varghese
<martinvarghesenokia@gmail.com> wrote:
>
> On Tue, Dec 10, 2019 at 01:22:56PM -0800, Pravin Shelar wrote:
> > On Tue, Dec 10, 2019 at 12:17 AM Martin Varghese
> > <martinvarghesenokia@gmail.com> wrote:
> > >
> > > From: Martin Varghese <martin.varghese@nokia.com>
> > >
> > > The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS header
> > > between ethernet header and the IP header. Though this behaviour is fine
> > > for L3 VPN where an IP packet is encapsulated inside a MPLS tunnel, it
> > > does not suffice the L2 VPN (l2 tunnelling) requirements. In L2 VPN
> > > the MPLS header should encapsulate the ethernet packet.
> > >
> > > The new mpls actions PTAP_PUSH_MPLS & PTAP_POP_MPLS inserts and removes
> > > MPLS header from start of the packet respectively.
> > >
> > > PTAP_PUSH_MPLS - Inserts MPLS header at the start of the packet.
> > > @ethertype - Ethertype of MPLS header.
> > >
> > > PTAP_POP_MPLS - Removes MPLS header from the start of the packet.
> > > @ethertype - Ethertype of next header following the popped MPLS header.
> > >              Value 0 in ethertype indicates the tunnelled packet is
> > >              ethernet.
> > >
> > Did you considered using existing MPLS action to handle L2 tunneling
> > packet ? It can be done by adding another parameter to the MPLS
> > actions.
>

>
>
> Not really.
>
> Are you suggesting to extend the ovs_action_push_mpls and similarly for pop
>
> struct ovs_action_push_mpls {
>         __be32 mpls_lse;
>         __be16 mpls_ethertype; /* Either %ETH_P_MPLS_UC or %ETH_P_MPLS_MC */
> +        bool l2_tunnel;
> };
>
> Does not that break the compatibilty with the existing userspace
> OVS ?
>
Right, extending this would not look good. I am fine with new action.
But we can design this new action as superset of existing and PTAP
functionality, This way in future we can deprecate existing MPLS
action in favor of new action.
I think if you add mac_len parameter for the action it would take case
of both cases.

Re: [PATCH net-next 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Pravin Shelar]

On Tue, Dec 10, 2019 at 12:17 AM Martin Varghese
<martinvarghesenokia@gmail.com> wrote:
>
> From: Martin Varghese <martin.varghese@nokia.com>
>
> The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS header
> between ethernet header and the IP header. Though this behaviour is fine
> for L3 VPN where an IP packet is encapsulated inside a MPLS tunnel, it
> does not suffice the L2 VPN (l2 tunnelling) requirements. In L2 VPN
> the MPLS header should encapsulate the ethernet packet.
>
> The new mpls actions PTAP_PUSH_MPLS & PTAP_POP_MPLS inserts and removes
> MPLS header from start of the packet respectively.
>
> PTAP_PUSH_MPLS - Inserts MPLS header at the start of the packet.
> @ethertype - Ethertype of MPLS header.
>
> PTAP_POP_MPLS - Removes MPLS header from the start of the packet.
> @ethertype - Ethertype of next header following the popped MPLS header.
>              Value 0 in ethertype indicates the tunnelled packet is
>              ethernet.
>
> Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
> ---
>  include/uapi/linux/openvswitch.h |  2 ++
>  net/openvswitch/actions.c        | 40 ++++++++++++++++++++++++++++++++++++++++
>  net/openvswitch/flow_netlink.c   | 21 +++++++++++++++++++++
>  3 files changed, 63 insertions(+)
>
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index a87b44c..af05062 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -927,6 +927,8 @@ enum ovs_action_attr {
>         OVS_ACTION_ATTR_METER,        /* u32 meter ID. */
>         OVS_ACTION_ATTR_CLONE,        /* Nested OVS_CLONE_ATTR_*.  */
>         OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
> +       OVS_ACTION_ATTR_PTAP_PUSH_MPLS,    /* struct ovs_action_push_mpls. */
> +       OVS_ACTION_ATTR_PTAP_POP_MPLS,     /* __be16 ethertype. */
>
>         __OVS_ACTION_ATTR_MAX,        /* Nothing past this will be accepted
>                                        * from userspace. */
What about MPLS set action? does existing action works with PTAP MPLS?


> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 4c83954..d43c37e 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -160,6 +160,38 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>                               struct sw_flow_key *key,
>                               const struct nlattr *attr, int len);
>
> +static int push_ptap_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> +                         const struct ovs_action_push_mpls *mpls)
> +{
> +       int err;
> +
> +       err = skb_mpls_push(skb, mpls->mpls_lse, mpls->mpls_ethertype,
> +                           0, false);
> +       if (err)
> +               return err;
> +
> +       key->mac_proto = MAC_PROTO_NONE;
> +       invalidate_flow_key(key);
> +       return 0;
> +}
> +
Can you factor out code from existing MPLS action to avoid code duplication.

> +static int ptap_pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> +                        const __be16 ethertype)
> +{
> +       int err;
> +
> +       err = skb_mpls_pop(skb, ethertype, skb->mac_len,
> +                          ovs_key_mac_proto(key) == MAC_PROTO_ETHERNET);
> +       if (err)
> +               return err;
> +
Why is mac_len passed here? given MPLS is topmost header I do not see
any need to move headers during pop operation.

Re: [PATCH net-next 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Martin Varghese]

On Tue, Dec 10, 2019 at 10:15:19PM -0800, Pravin Shelar wrote:
> On Tue, Dec 10, 2019 at 4:02 PM Martin Varghese
> <martinvarghesenokia@gmail.com> wrote:
> >
> > On Tue, Dec 10, 2019 at 01:22:56PM -0800, Pravin Shelar wrote:
> > > On Tue, Dec 10, 2019 at 12:17 AM Martin Varghese
> > > <martinvarghesenokia@gmail.com> wrote:
> > > >
> > > > From: Martin Varghese <martin.varghese@nokia.com>
> > > >
> > > > The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS header
> > > > between ethernet header and the IP header. Though this behaviour is fine
> > > > for L3 VPN where an IP packet is encapsulated inside a MPLS tunnel, it
> > > > does not suffice the L2 VPN (l2 tunnelling) requirements. In L2 VPN
> > > > the MPLS header should encapsulate the ethernet packet.
> > > >
> > > > The new mpls actions PTAP_PUSH_MPLS & PTAP_POP_MPLS inserts and removes
> > > > MPLS header from start of the packet respectively.
> > > >
> > > > PTAP_PUSH_MPLS - Inserts MPLS header at the start of the packet.
> > > > @ethertype - Ethertype of MPLS header.
> > > >
> > > > PTAP_POP_MPLS - Removes MPLS header from the start of the packet.
> > > > @ethertype - Ethertype of next header following the popped MPLS header.
> > > >              Value 0 in ethertype indicates the tunnelled packet is
> > > >              ethernet.
> > > >
> > > Did you considered using existing MPLS action to handle L2 tunneling
> > > packet ? It can be done by adding another parameter to the MPLS
> > > actions.
> >
> 
> >
> >
> > Not really.
> >
> > Are you suggesting to extend the ovs_action_push_mpls and similarly for pop
> >
> > struct ovs_action_push_mpls {
> >         __be32 mpls_lse;
> >         __be16 mpls_ethertype; /* Either %ETH_P_MPLS_UC or %ETH_P_MPLS_MC */
> > +        bool l2_tunnel;
> > };
> >
> > Does not that break the compatibilty with the existing userspace
> > OVS ?
> >
> Right, extending this would not look good. I am fine with new action.
> But we can design this new action as superset of existing and PTAP
> functionality, This way in future we can deprecate existing MPLS
> action in favor of new action.
> I think if you add mac_len parameter for the action it would take case
> of both cases.
Yes i guess so.
On the similar lines i guess we dont need a new PTAP_POP action as the existing
pop action pops mpls header from the start of the packet if the skb->mac_len=0
We just neeed a add a special handling for ethertype 0 is the existing pop 
implementation

Re: [PATCH net-next 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Martin Varghese]

On Tue, Dec 10, 2019 at 10:15:57PM -0800, Pravin Shelar wrote:
> On Tue, Dec 10, 2019 at 12:17 AM Martin Varghese
> <martinvarghesenokia@gmail.com> wrote:
> >
> > From: Martin Varghese <martin.varghese@nokia.com>
> >
> > The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS header
> > between ethernet header and the IP header. Though this behaviour is fine
> > for L3 VPN where an IP packet is encapsulated inside a MPLS tunnel, it
> > does not suffice the L2 VPN (l2 tunnelling) requirements. In L2 VPN
> > the MPLS header should encapsulate the ethernet packet.
> >
> > The new mpls actions PTAP_PUSH_MPLS & PTAP_POP_MPLS inserts and removes
> > MPLS header from start of the packet respectively.
> >
> > PTAP_PUSH_MPLS - Inserts MPLS header at the start of the packet.
> > @ethertype - Ethertype of MPLS header.
> >
> > PTAP_POP_MPLS - Removes MPLS header from the start of the packet.
> > @ethertype - Ethertype of next header following the popped MPLS header.
> >              Value 0 in ethertype indicates the tunnelled packet is
> >              ethernet.
> >
> > Signed-off-by: Martin Varghese <martin.varghese@nokia.com>
> > ---
> >  include/uapi/linux/openvswitch.h |  2 ++
> >  net/openvswitch/actions.c        | 40 ++++++++++++++++++++++++++++++++++++++++
> >  net/openvswitch/flow_netlink.c   | 21 +++++++++++++++++++++
> >  3 files changed, 63 insertions(+)
> >
> > diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> > index a87b44c..af05062 100644
> > --- a/include/uapi/linux/openvswitch.h
> > +++ b/include/uapi/linux/openvswitch.h
> > @@ -927,6 +927,8 @@ enum ovs_action_attr {
> >         OVS_ACTION_ATTR_METER,        /* u32 meter ID. */
> >         OVS_ACTION_ATTR_CLONE,        /* Nested OVS_CLONE_ATTR_*.  */
> >         OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */
> > +       OVS_ACTION_ATTR_PTAP_PUSH_MPLS,    /* struct ovs_action_push_mpls. */
> > +       OVS_ACTION_ATTR_PTAP_POP_MPLS,     /* __be16 ethertype. */
> >
> >         __OVS_ACTION_ATTR_MAX,        /* Nothing past this will be accepted
> >                                        * from userspace. */
> What about MPLS set action? does existing action works with PTAP MPLS?
>
It should work as skb->network header is used to locate MPLS header
but skb_mpls_push is not calling skb_reset_mac_len to set mac len to 0
We need to add that. 
> 
> > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> > index 4c83954..d43c37e 100644
> > --- a/net/openvswitch/actions.c
> > +++ b/net/openvswitch/actions.c
> > @@ -160,6 +160,38 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
> >                               struct sw_flow_key *key,
> >                               const struct nlattr *attr, int len);
> >
> > +static int push_ptap_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> > +                         const struct ovs_action_push_mpls *mpls)
> > +{
> > +       int err;
> > +
> > +       err = skb_mpls_push(skb, mpls->mpls_lse, mpls->mpls_ethertype,
> > +                           0, false);
> > +       if (err)
> > +               return err;
> > +
> > +       key->mac_proto = MAC_PROTO_NONE;
> > +       invalidate_flow_key(key);
> > +       return 0;
> > +}
> > +
> Can you factor out code from existing MPLS action to avoid code duplication.
> 
yes

> > +static int ptap_pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> > +                        const __be16 ethertype)
> > +{
> > +       int err;
> > +
> > +       err = skb_mpls_pop(skb, ethertype, skb->mac_len,
> > +                          ovs_key_mac_proto(key) == MAC_PROTO_ETHERNET);
> > +       if (err)
> > +               return err;
> > +
> Why is mac_len passed here? given MPLS is topmost header I do not see
> any need to move headers during pop operation.
skb->mac_len will be 0 here.

Re: [PATCH net-next 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Pravin Shelar]

On Wed, Dec 11, 2019 at 7:39 AM Martin Varghese
<martinvarghesenokia@gmail.com> wrote:
>
> On Tue, Dec 10, 2019 at 10:15:19PM -0800, Pravin Shelar wrote:
> > On Tue, Dec 10, 2019 at 4:02 PM Martin Varghese
> > <martinvarghesenokia@gmail.com> wrote:
> > >
> > > On Tue, Dec 10, 2019 at 01:22:56PM -0800, Pravin Shelar wrote:
> > > > On Tue, Dec 10, 2019 at 12:17 AM Martin Varghese
> > > > <martinvarghesenokia@gmail.com> wrote:
> > > > >
> > > > > From: Martin Varghese <martin.varghese@nokia.com>
> > > > >
> > > > > The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS header
> > > > > between ethernet header and the IP header. Though this behaviour is fine
> > > > > for L3 VPN where an IP packet is encapsulated inside a MPLS tunnel, it
> > > > > does not suffice the L2 VPN (l2 tunnelling) requirements. In L2 VPN
> > > > > the MPLS header should encapsulate the ethernet packet.
> > > > >
> > > > > The new mpls actions PTAP_PUSH_MPLS & PTAP_POP_MPLS inserts and removes
> > > > > MPLS header from start of the packet respectively.
> > > > >
> > > > > PTAP_PUSH_MPLS - Inserts MPLS header at the start of the packet.
> > > > > @ethertype - Ethertype of MPLS header.
> > > > >
> > > > > PTAP_POP_MPLS - Removes MPLS header from the start of the packet.
> > > > > @ethertype - Ethertype of next header following the popped MPLS header.
> > > > >              Value 0 in ethertype indicates the tunnelled packet is
> > > > >              ethernet.
> > > > >
> > > > Did you considered using existing MPLS action to handle L2 tunneling
> > > > packet ? It can be done by adding another parameter to the MPLS
> > > > actions.
> > >
> >
> > >
> > >
> > > Not really.
> > >
> > > Are you suggesting to extend the ovs_action_push_mpls and similarly for pop
> > >
> > > struct ovs_action_push_mpls {
> > >         __be32 mpls_lse;
> > >         __be16 mpls_ethertype; /* Either %ETH_P_MPLS_UC or %ETH_P_MPLS_MC */
> > > +        bool l2_tunnel;
> > > };
> > >
> > > Does not that break the compatibilty with the existing userspace
> > > OVS ?
> > >
> > Right, extending this would not look good. I am fine with new action.
> > But we can design this new action as superset of existing and PTAP
> > functionality, This way in future we can deprecate existing MPLS
> > action in favor of new action.
> > I think if you add mac_len parameter for the action it would take case
> > of both cases.
> Yes i guess so.
> On the similar lines i guess we dont need a new PTAP_POP action as the existing
> pop action pops mpls header from the start of the packet if the skb->mac_len=0
> We just neeed a add a special handling for ethertype 0 is the existing pop
> implementation

Passing next_proto as zero to skb_mpls_pop() would set skb->protocol
to zero. That does not look good. Lets pass mac_len and next_proto for
both Push and Pop action. I am also fine using using boolean to
distinguish between L2 and L3 case. In that case we are dependent on
skb->mac_len.

Re: [PATCH net-next 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Martin Varghese]

On Wed, Dec 11, 2019 at 08:19:08PM -0800, Pravin Shelar wrote:
> On Wed, Dec 11, 2019 at 7:39 AM Martin Varghese
> <martinvarghesenokia@gmail.com> wrote:
> >
> > On Tue, Dec 10, 2019 at 10:15:19PM -0800, Pravin Shelar wrote:
> > > On Tue, Dec 10, 2019 at 4:02 PM Martin Varghese
> > > <martinvarghesenokia@gmail.com> wrote:
> > > >
> > > > On Tue, Dec 10, 2019 at 01:22:56PM -0800, Pravin Shelar wrote:
> > > > > On Tue, Dec 10, 2019 at 12:17 AM Martin Varghese
> > > > > <martinvarghesenokia@gmail.com> wrote:
> > > > > >
> > > > > > From: Martin Varghese <martin.varghese@nokia.com>
> > > > > >
> > > > > > The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS header
> > > > > > between ethernet header and the IP header. Though this behaviour is fine
> > > > > > for L3 VPN where an IP packet is encapsulated inside a MPLS tunnel, it
> > > > > > does not suffice the L2 VPN (l2 tunnelling) requirements. In L2 VPN
> > > > > > the MPLS header should encapsulate the ethernet packet.
> > > > > >
> > > > > > The new mpls actions PTAP_PUSH_MPLS & PTAP_POP_MPLS inserts and removes
> > > > > > MPLS header from start of the packet respectively.
> > > > > >
> > > > > > PTAP_PUSH_MPLS - Inserts MPLS header at the start of the packet.
> > > > > > @ethertype - Ethertype of MPLS header.
> > > > > >
> > > > > > PTAP_POP_MPLS - Removes MPLS header from the start of the packet.
> > > > > > @ethertype - Ethertype of next header following the popped MPLS header.
> > > > > >              Value 0 in ethertype indicates the tunnelled packet is
> > > > > >              ethernet.
> > > > > >
> > > > > Did you considered using existing MPLS action to handle L2 tunneling
> > > > > packet ? It can be done by adding another parameter to the MPLS
> > > > > actions.
> > > >
> > >
> > > >
> > > >
> > > > Not really.
> > > >
> > > > Are you suggesting to extend the ovs_action_push_mpls and similarly for pop
> > > >
> > > > struct ovs_action_push_mpls {
> > > >         __be32 mpls_lse;
> > > >         __be16 mpls_ethertype; /* Either %ETH_P_MPLS_UC or %ETH_P_MPLS_MC */
> > > > +        bool l2_tunnel;
> > > > };
> > > >
> > > > Does not that break the compatibilty with the existing userspace
> > > > OVS ?
> > > >
> > > Right, extending this would not look good. I am fine with new action.
> > > But we can design this new action as superset of existing and PTAP
> > > functionality, This way in future we can deprecate existing MPLS
> > > action in favor of new action.
> > > I think if you add mac_len parameter for the action it would take case
> > > of both cases.
> > Yes i guess so.
> > On the similar lines i guess we dont need a new PTAP_POP action as the existing
> > pop action pops mpls header from the start of the packet if the skb->mac_len=0
> > We just neeed a add a special handling for ethertype 0 is the existing pop
> > implementation
> 
> Passing next_proto as zero to skb_mpls_pop() would set skb->protocol
> to zero. That does not look good. Lets pass mac_len and next_proto for
> both Push and Pop action. I am also fine using using boolean to
> distinguish between L2 and L3 case. In that case we are dependent on
> skb->mac_len.

But setting to zero may be appropriate ? (a kind of reset of the protocol)
Normally skb->protocol holds the ethertype, but in this case we have a ethernet
header after the MPLS header and we need to read that ethernet header to 
find the ethertype.
Also if we decide the caller has to pass the ethertype as it is in normal pop
along with a l2 flag, which ethertype the skb_mpls_pop caller will pass.

Or should the caller pass the trans ether bridging ethertype 0x6558.In that 
case we may not need a flag, but i am not sure if using 0x6558 is correct here.

Re: [PATCH net-next 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Pravin Shelar]

On Thu, Dec 12, 2019 at 8:02 AM Martin Varghese
<martinvarghesenokia@gmail.com> wrote:
>
> On Wed, Dec 11, 2019 at 08:19:08PM -0800, Pravin Shelar wrote:
> > On Wed, Dec 11, 2019 at 7:39 AM Martin Varghese
> > <martinvarghesenokia@gmail.com> wrote:
> > >
> > > On Tue, Dec 10, 2019 at 10:15:19PM -0800, Pravin Shelar wrote:
> > > > On Tue, Dec 10, 2019 at 4:02 PM Martin Varghese
> > > > <martinvarghesenokia@gmail.com> wrote:
> > > > >
> > > > > On Tue, Dec 10, 2019 at 01:22:56PM -0800, Pravin Shelar wrote:
> > > > > > On Tue, Dec 10, 2019 at 12:17 AM Martin Varghese
> > > > > > <martinvarghesenokia@gmail.com> wrote:
> > > > > > >
> > > > > > > From: Martin Varghese <martin.varghese@nokia.com>
> > > > > > >
> > > > > > > The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS header
> > > > > > > between ethernet header and the IP header. Though this behaviour is fine
> > > > > > > for L3 VPN where an IP packet is encapsulated inside a MPLS tunnel, it
> > > > > > > does not suffice the L2 VPN (l2 tunnelling) requirements. In L2 VPN
> > > > > > > the MPLS header should encapsulate the ethernet packet.
> > > > > > >
> > > > > > > The new mpls actions PTAP_PUSH_MPLS & PTAP_POP_MPLS inserts and removes
> > > > > > > MPLS header from start of the packet respectively.
> > > > > > >
> > > > > > > PTAP_PUSH_MPLS - Inserts MPLS header at the start of the packet.
> > > > > > > @ethertype - Ethertype of MPLS header.
> > > > > > >
> > > > > > > PTAP_POP_MPLS - Removes MPLS header from the start of the packet.
> > > > > > > @ethertype - Ethertype of next header following the popped MPLS header.
> > > > > > >              Value 0 in ethertype indicates the tunnelled packet is
> > > > > > >              ethernet.
> > > > > > >
> > > > > > Did you considered using existing MPLS action to handle L2 tunneling
> > > > > > packet ? It can be done by adding another parameter to the MPLS
> > > > > > actions.
> > > > >
> > > >
> > > > >
> > > > >
> > > > > Not really.
> > > > >
> > > > > Are you suggesting to extend the ovs_action_push_mpls and similarly for pop
> > > > >
> > > > > struct ovs_action_push_mpls {
> > > > >         __be32 mpls_lse;
> > > > >         __be16 mpls_ethertype; /* Either %ETH_P_MPLS_UC or %ETH_P_MPLS_MC */
> > > > > +        bool l2_tunnel;
> > > > > };
> > > > >
> > > > > Does not that break the compatibilty with the existing userspace
> > > > > OVS ?
> > > > >
> > > > Right, extending this would not look good. I am fine with new action.
> > > > But we can design this new action as superset of existing and PTAP
> > > > functionality, This way in future we can deprecate existing MPLS
> > > > action in favor of new action.
> > > > I think if you add mac_len parameter for the action it would take case
> > > > of both cases.
> > > Yes i guess so.
> > > On the similar lines i guess we dont need a new PTAP_POP action as the existing
> > > pop action pops mpls header from the start of the packet if the skb->mac_len=0
> > > We just neeed a add a special handling for ethertype 0 is the existing pop
> > > implementation
> >
> > Passing next_proto as zero to skb_mpls_pop() would set skb->protocol
> > to zero. That does not look good. Lets pass mac_len and next_proto for
> > both Push and Pop action. I am also fine using using boolean to
> > distinguish between L2 and L3 case. In that case we are dependent on
> > skb->mac_len.
>
> But setting to zero may be appropriate ? (a kind of reset of the protocol)
> Normally skb->protocol holds the ethertype, but in this case we have a ethernet
> header after the MPLS header and we need to read that ethernet header to
> find the ethertype.
> Also if we decide the caller has to pass the ethertype as it is in normal pop
> along with a l2 flag, which ethertype the skb_mpls_pop caller will pass.
>
> Or should the caller pass the trans ether bridging ethertype 0x6558.In that
> case we may not need a flag, but i am not sure if using 0x6558 is correct here.
>
The inner packet type needs to be part of MPLS pop action. We can not
assume it would be ethernet packet. Otherwise OVS will not be able to
support multiple tagged packets for L2 MPLS tunneling.

Re: [PATCH net-next 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Martin Varghese]

On Thu, Dec 12, 2019 at 07:15:47PM -0800, Pravin Shelar wrote:
> On Thu, Dec 12, 2019 at 8:02 AM Martin Varghese
> <martinvarghesenokia@gmail.com> wrote:
> >
> > On Wed, Dec 11, 2019 at 08:19:08PM -0800, Pravin Shelar wrote:
> > > On Wed, Dec 11, 2019 at 7:39 AM Martin Varghese
> > > <martinvarghesenokia@gmail.com> wrote:
> > > >
> > > > On Tue, Dec 10, 2019 at 10:15:19PM -0800, Pravin Shelar wrote:
> > > > > On Tue, Dec 10, 2019 at 4:02 PM Martin Varghese
> > > > > <martinvarghesenokia@gmail.com> wrote:
> > > > > >
> > > > > > On Tue, Dec 10, 2019 at 01:22:56PM -0800, Pravin Shelar wrote:
> > > > > > > On Tue, Dec 10, 2019 at 12:17 AM Martin Varghese
> > > > > > > <martinvarghesenokia@gmail.com> wrote:
> > > > > > > >
> > > > > > > > From: Martin Varghese <martin.varghese@nokia.com>
> > > > > > > >
> > > > > > > > The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS header
> > > > > > > > between ethernet header and the IP header. Though this behaviour is fine
> > > > > > > > for L3 VPN where an IP packet is encapsulated inside a MPLS tunnel, it
> > > > > > > > does not suffice the L2 VPN (l2 tunnelling) requirements. In L2 VPN
> > > > > > > > the MPLS header should encapsulate the ethernet packet.
> > > > > > > >
> > > > > > > > The new mpls actions PTAP_PUSH_MPLS & PTAP_POP_MPLS inserts and removes
> > > > > > > > MPLS header from start of the packet respectively.
> > > > > > > >
> > > > > > > > PTAP_PUSH_MPLS - Inserts MPLS header at the start of the packet.
> > > > > > > > @ethertype - Ethertype of MPLS header.
> > > > > > > >
> > > > > > > > PTAP_POP_MPLS - Removes MPLS header from the start of the packet.
> > > > > > > > @ethertype - Ethertype of next header following the popped MPLS header.
> > > > > > > >              Value 0 in ethertype indicates the tunnelled packet is
> > > > > > > >              ethernet.
> > > > > > > >
> > > > > > > Did you considered using existing MPLS action to handle L2 tunneling
> > > > > > > packet ? It can be done by adding another parameter to the MPLS
> > > > > > > actions.
> > > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > Not really.
> > > > > >
> > > > > > Are you suggesting to extend the ovs_action_push_mpls and similarly for pop
> > > > > >
> > > > > > struct ovs_action_push_mpls {
> > > > > >         __be32 mpls_lse;
> > > > > >         __be16 mpls_ethertype; /* Either %ETH_P_MPLS_UC or %ETH_P_MPLS_MC */
> > > > > > +        bool l2_tunnel;
> > > > > > };
> > > > > >
> > > > > > Does not that break the compatibilty with the existing userspace
> > > > > > OVS ?
> > > > > >
> > > > > Right, extending this would not look good. I am fine with new action.
> > > > > But we can design this new action as superset of existing and PTAP
> > > > > functionality, This way in future we can deprecate existing MPLS
> > > > > action in favor of new action.
> > > > > I think if you add mac_len parameter for the action it would take case
> > > > > of both cases.
> > > > Yes i guess so.
> > > > On the similar lines i guess we dont need a new PTAP_POP action as the existing
> > > > pop action pops mpls header from the start of the packet if the skb->mac_len=0
> > > > We just neeed a add a special handling for ethertype 0 is the existing pop
> > > > implementation
> > >
> > > Passing next_proto as zero to skb_mpls_pop() would set skb->protocol
> > > to zero. That does not look good. Lets pass mac_len and next_proto for
> > > both Push and Pop action. I am also fine using using boolean to
> > > distinguish between L2 and L3 case. In that case we are dependent on
> > > skb->mac_len.
> >
> > But setting to zero may be appropriate ? (a kind of reset of the protocol)
> > Normally skb->protocol holds the ethertype, but in this case we have a ethernet
> > header after the MPLS header and we need to read that ethernet header to
> > find the ethertype.
> > Also if we decide the caller has to pass the ethertype as it is in normal pop
> > along with a l2 flag, which ethertype the skb_mpls_pop caller will pass.
> >
> > Or should the caller pass the trans ether bridging ethertype 0x6558.In that
> > case we may not need a flag, but i am not sure if using 0x6558 is correct here.
> >
> The inner packet type needs to be part of MPLS pop action. We can not
> assume it would be ethernet packet. Otherwise OVS will not be able to
> support multiple tagged packets for L2 MPLS tunneling.

I am not sure if i got you comment correctly ?

We are not assuming the inner packet is ethernet always.We are just retriggring the
parsing with ethernet header if ethertype is 0

By multiple tagged packets you mean below

MPLS 1|MPLS 2| ethernet--|

But in this case ethertype passed to pop action will be 0x8847 or 0x8848 and not 0.


My other doubt is If we use a flag to say it is a l2 tunnel.
what the caller  will pass in ethertype.and subsequently what we will set in openvswitch.

Re: [PATCH net-next 3/3] openvswitch: New MPLS actions for layer 2 tunnelling

[Pravin Shelar]

On Thu, Dec 12, 2019 at 7:53 PM Martin Varghese
<martinvarghesenokia@gmail.com> wrote:
>
> On Thu, Dec 12, 2019 at 07:15:47PM -0800, Pravin Shelar wrote:
> > On Thu, Dec 12, 2019 at 8:02 AM Martin Varghese
> > <martinvarghesenokia@gmail.com> wrote:
> > >
> > > On Wed, Dec 11, 2019 at 08:19:08PM -0800, Pravin Shelar wrote:
> > > > On Wed, Dec 11, 2019 at 7:39 AM Martin Varghese
> > > > <martinvarghesenokia@gmail.com> wrote:
> > > > >
> > > > > On Tue, Dec 10, 2019 at 10:15:19PM -0800, Pravin Shelar wrote:
> > > > > > On Tue, Dec 10, 2019 at 4:02 PM Martin Varghese
> > > > > > <martinvarghesenokia@gmail.com> wrote:
> > > > > > >
> > > > > > > On Tue, Dec 10, 2019 at 01:22:56PM -0800, Pravin Shelar wrote:
> > > > > > > > On Tue, Dec 10, 2019 at 12:17 AM Martin Varghese
> > > > > > > > <martinvarghesenokia@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > From: Martin Varghese <martin.varghese@nokia.com>
> > > > > > > > >
> > > > > > > > > The existing PUSH MPLS & POP MPLS actions inserts & removes MPLS header
> > > > > > > > > between ethernet header and the IP header. Though this behaviour is fine
> > > > > > > > > for L3 VPN where an IP packet is encapsulated inside a MPLS tunnel, it
> > > > > > > > > does not suffice the L2 VPN (l2 tunnelling) requirements. In L2 VPN
> > > > > > > > > the MPLS header should encapsulate the ethernet packet.
> > > > > > > > >
> > > > > > > > > The new mpls actions PTAP_PUSH_MPLS & PTAP_POP_MPLS inserts and removes
> > > > > > > > > MPLS header from start of the packet respectively.
> > > > > > > > >
> > > > > > > > > PTAP_PUSH_MPLS - Inserts MPLS header at the start of the packet.
> > > > > > > > > @ethertype - Ethertype of MPLS header.
> > > > > > > > >
> > > > > > > > > PTAP_POP_MPLS - Removes MPLS header from the start of the packet.
> > > > > > > > > @ethertype - Ethertype of next header following the popped MPLS header.
> > > > > > > > >              Value 0 in ethertype indicates the tunnelled packet is
> > > > > > > > >              ethernet.
> > > > > > > > >
> > > > > > > > Did you considered using existing MPLS action to handle L2 tunneling
> > > > > > > > packet ? It can be done by adding another parameter to the MPLS
> > > > > > > > actions.
> > > > > > >
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > Not really.
> > > > > > >
> > > > > > > Are you suggesting to extend the ovs_action_push_mpls and similarly for pop
> > > > > > >
> > > > > > > struct ovs_action_push_mpls {
> > > > > > >         __be32 mpls_lse;
> > > > > > >         __be16 mpls_ethertype; /* Either %ETH_P_MPLS_UC or %ETH_P_MPLS_MC */
> > > > > > > +        bool l2_tunnel;
> > > > > > > };
> > > > > > >
> > > > > > > Does not that break the compatibilty with the existing userspace
> > > > > > > OVS ?
> > > > > > >
> > > > > > Right, extending this would not look good. I am fine with new action.
> > > > > > But we can design this new action as superset of existing and PTAP
> > > > > > functionality, This way in future we can deprecate existing MPLS
> > > > > > action in favor of new action.
> > > > > > I think if you add mac_len parameter for the action it would take case
> > > > > > of both cases.
> > > > > Yes i guess so.
> > > > > On the similar lines i guess we dont need a new PTAP_POP action as the existing
> > > > > pop action pops mpls header from the start of the packet if the skb->mac_len=0
> > > > > We just neeed a add a special handling for ethertype 0 is the existing pop
> > > > > implementation
> > > >
> > > > Passing next_proto as zero to skb_mpls_pop() would set skb->protocol
> > > > to zero. That does not look good. Lets pass mac_len and next_proto for
> > > > both Push and Pop action. I am also fine using using boolean to
> > > > distinguish between L2 and L3 case. In that case we are dependent on
> > > > skb->mac_len.
> > >
> > > But setting to zero may be appropriate ? (a kind of reset of the protocol)
> > > Normally skb->protocol holds the ethertype, but in this case we have a ethernet
> > > header after the MPLS header and we need to read that ethernet header to
> > > find the ethertype.
> > > Also if we decide the caller has to pass the ethertype as it is in normal pop
> > > along with a l2 flag, which ethertype the skb_mpls_pop caller will pass.
> > >
> > > Or should the caller pass the trans ether bridging ethertype 0x6558.In that
> > > case we may not need a flag, but i am not sure if using 0x6558 is correct here.
> > >
> > The inner packet type needs to be part of MPLS pop action. We can not
> > assume it would be ethernet packet. Otherwise OVS will not be able to
> > support multiple tagged packets for L2 MPLS tunneling.
>
> I am not sure if i got you comment correctly ?
>
> We are not assuming the inner packet is ethernet always.We are just retriggring the
> parsing with ethernet header if ethertype is 0
>
> By multiple tagged packets you mean below
>
> MPLS 1|MPLS 2| ethernet--|
>
> But in this case ethertype passed to pop action will be 0x8847 or 0x8848 and not 0.
>
>
ok.

> My other doubt is If we use a flag to say it is a l2 tunnel.
> what the caller  will pass in ethertype.and subsequently what we will set in openvswitch.

I do not like creating special case for ethernet packet. OVS can pass
ethertype in the POP action parameter. That way skb-protocol would
have correct value when OVS pop MPLS header from L2 tunnel packet.

Second issue is inconsistency between  PUSH  and POP action. This
patch is using mac_len from action parameter in Push but for POP
action it is using skb->mac_len.