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

[Martin Varghese <martinvarghesenokia@gmail.com>]

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.