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

[Martin Varghese <martinvarghesenokia@gmail.com>]

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.