Re: [PATCH net-next] Change in Openvswitch to support MPLS label depth of 3 in ingress direction

[Pravin Shelar <pshelar@ovn.org>]

On Tue, Oct 15, 2019 at 8:05 PM Martin Varghese
<martinvarghesenokia@gmail.com> wrote:
>
> On Tue, Oct 15, 2019 at 12:03:35AM -0700, Pravin Shelar wrote:
> > On Mon, Oct 14, 2019 at 9:06 AM Martin Varghese
> > <martinvarghesenokia@gmail.com> wrote:
> > >
> > > On Sat, Oct 12, 2019 at 01:08:26PM -0700, Pravin Shelar wrote:
> > > > On Wed, Oct 9, 2019 at 9:31 PM Martin Varghese
> > > > <martinvarghesenokia@gmail.com> wrote:
> > > > >
> > > > > On Wed, Oct 09, 2019 at 08:29:51AM -0700, Pravin Shelar wrote:
> > > > > > On Mon, Oct 7, 2019 at 9:41 PM Martin Varghese
> > > > > > <martinvarghesenokia@gmail.com> wrote:
> > > > > > >
> > > > > > > From: Martin Varghese <martin.varghese@nokia.com>
> > > > > > >
> > > > > > > The openvswitch was supporting a MPLS label depth of 1 in the ingress
> > > > > > > direction though the userspace OVS supports a max depth of 3 labels.
> > > > > > > This change enables openvswitch module to support a max depth of
> > > > > > > 3 labels in the ingress.
> > > > > > >
> > > > > > > Signed-off-by: Martin Varghese <martinvarghesenokia@gmail.com>
> > > > > > > ---
> > > > > > >  net/openvswitch/actions.c      | 10 +++++++-
> > > > > > >  net/openvswitch/flow.c         | 20 ++++++++++-----
> > > > > > >  net/openvswitch/flow.h         |  9 ++++---
> > > > > > >  net/openvswitch/flow_netlink.c | 55 +++++++++++++++++++++++++++++++++---------
> > > > > > >  4 files changed, 72 insertions(+), 22 deletions(-)
> > > > > > >
> > > > > > > diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> > > > > > > index 3572e11..eb5bed5 100644
> > > > > > > --- a/net/openvswitch/actions.c
> > > > > > > +++ b/net/openvswitch/actions.c
> > > > > > > @@ -178,10 +178,14 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> > > > > > >  {
> > > > > > >         int err;
> > > > > > >
> > > > > > > +       if (!key->mpls.num_labels_mask)
> > > > > > > +               return -EINVAL;
> > > > > > > +
> > > > > > >         err = skb_mpls_pop(skb, ethertype);
> > > > > > >         if (err)
> > > > > > >                 return err;
> > > > > > >
> > > > > > > +       key->mpls.num_labels_mask >>= 1;
> > > > > > >         invalidate_flow_key(key);
> > > > > > Since this key is immediately invalidated, what is point of updating
> > > > > > the label count?
> > > > > >
> > > > >
> > > > > The num_labels_mask is being checked in the pop_mpls action to see if anymore
> > > > > label present to pop.
> > > > >
> > > > > if (!key->mpls.num_labels_mask)
> > > > >         return -EINVAL:
> > > > >
> > > > > > >         return 0;
> > > > > > >  }
> > > > > > What about checks in OVS_ACTION_ATTR_PUSH_MPLS?
> > > > > >
> > > > > The change does not have any impact to the PUSH_MPLS actions.
> > > > > It should work as before.
> > > > >
> > > > Since the MPLS label count is checked in POP, the PUSH action needs to
> > > > update the count.
> > > >
> > > Ok, that would be to support a rule like this
> > > eth_type(0x0800),actions: push_mpls(label=7,tc=0,ttl=64,bos=1,eth_type=0x8847), pop_mpls(eth_type=0x800)
> > >
> > > Though this rule has no effect we can support it for the completeness.
> > > It would entail a code like this
> > >
> > >  if (eth_p_mpls(proto))
> > >         key->mpls.num_labels_mask = (key->mpls.num_labels_mask << 1 &
> > >                                      GENMASK(MPLS_LABEL_DEPTH -1, 0))| 0x1;  else
> > >         key->mpls.num_labels_mask = (key->mpls.num_labels_mask & 0x0)|0x01;
> > Yes, but as mentioned below, it can be moved to flow install time.
> >
> > > > > > > @@ -192,6 +196,7 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *flow_key,
> > > > > > >         struct mpls_shim_hdr *stack;
> > > > > > >         __be32 lse;
> > > > > > >         int err;
> > > > > > > +       u32 i = 0;
> > > > > > >
> > > > > > >         stack = mpls_hdr(skb);
> > > > > > >         lse = OVS_MASKED(stack->label_stack_entry, *mpls_lse, *mask);
> > > > > > > @@ -199,7 +204,10 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *flow_key,
> > > > > > >         if (err)
> > > > > > >                 return err;
> > > > > > >
> > > > > > > -       flow_key->mpls.top_lse = lse;
> > > > > > > +       for (i = MPLS_LABEL_DEPTH - 1; i > 0; i--)
> > > > > > > +               flow_key->mpls.lse[i] = flow_key->mpls.lse[i - 1];
> > > > > > > +
> > > > > > > +       flow_key->mpls.lse[i] = *mpls_lse;
> > > > > > This is changing semantic of mpls-set action. It is looking like
> > > > > > mpls-push. Lets keep the MPLS set that sets one or more MPLS lebels.
> > > > > >
> > > > > > >         return 0;
> > > > > > >  }
> > > > >
> > > > > Not sure if I got your comment correct.
> > > > > Just as before, the new code updates the top most label in the flow_key.
> > > > I am referring to the for loop which shifts labels to make room new
> > > > label been set. I think the set action should over-write labels
> > > > specified in set action.
> > > >
> > > Correct. Then this would suffice " flow_key->mpls.lse[0] = lse"
> > >
> > Yes, with this change this action need to support one more labels in set action.
> >
> shouldn't we be consistent with the existing behaviour in current kernel module, Dpif-netdev & ovs userspace, where the set_mpls action  sets the top most MPLS label params ?
ok, I am fine with setting topmost label.