[ovs-dev] Tunnel matching and remote_ip=flow (was Re: [ovs-discuss] vxlan remote_ip flow help)

[Ben Pfaff]

On Sun, Feb 02, 2014 at 10:45:29PM -0800, Jesse Gross wrote:
> On Fri, Jan 31, 2014 at 4:59 PM, Ben Pfaff <blp at nicira.com> wrote:
> > On Fri, Jan 31, 2014 at 10:38:20AM -0800, Jesse Gross wrote:
> >> On Thu, Jan 30, 2014 at 1:31 PM, Thomas Morin <thomas.morin at orange.com> wrote:
> >> > Hi,
> >> >
> >> > I ran into the same issue as the one described below (post on ovs-discuss a
> >> > few months back).
> >> >
> >> > The goal is to use a vport tunnel configured to use a flow-based remote IP,
> >> > e.g:
> >> >     ovs-vscl add-port br-vnet tun-port -- set Interface tun-port type=gre
> >> > options:remote_ip=flow
> >> >
> >> > This works fine on the sending OVS, but on the receiving OVS the same vport
> >> > won't receive any traffic (packets received trigger a "receive tunnel port
> >> > not found" warning, and are not further processed).
> >> >
> >> > The original poster had found a workaround relying on also using
> >> > options:key=flow, but as far as I understand, this work around should not be
> >> > necessary: it just seems that the tunnel matching code is missing a match.
> >> >
> >> > --- a/ofproto/tunnel.c
> >> > +++ b/ofproto/tunnel.c
> >> > @@ -427,8 +427,9 @@ tnl_find(const struct flow *flow) OVS_REQ_RDLOCK(rwlock)
> >> >          { false, false, IP_SRC_ANY },  /* remote_ip, in_key. */
> >> >          { true,  false, IP_SRC_CFG },  /* remote_ip, local_ip. */
> >> >          { true,  false, IP_SRC_ANY },  /* remote_ip. */
> >> > -        { true,  true,  IP_SRC_ANY },  /* Flow-based remote. */
> >> > +        { true,  true,  IP_SRC_ANY },  /* Flow-based remote and flow-based
> >> > key. */
> >> >          { true,  true,  IP_SRC_FLOW }, /* Flow-based everything. */
> >> > +        { false, true,  IP_SRC_ANY },  /* Flow-based remote */
> >> >      };
> >> >
> >> >      const struct tnl_match_pattern *p;
> >> >
> >> > With this patch, a tunnel configured with just remote_ip=flow will work fine
> >> > on the receiving end.
> >> > One thing that I'm not sure is at which position this pattern should appear
> >> > in the list.
> >> > The patch also changes the comment for true/true/IP_SRC_ANY.
> >>
> >> This looks fine to me although I think that it the new entry belongs
> >> first in the list of flow-based types. Can you submit a formal patch
> >> with this change?
> >
> > By my count there are 12 possible entries in the list of
> > possibilities.  I don't know if there's any point in adding them one
> > by one.  I guess we might as well add all of them at one
> > systematically.
> >
> > How about this, then?  It's not correct--I haven't debugged it--but it
> > gives the flavor of the idea.
> 
> I think it is a reasonable thing to do although I think it might
> change the priority order that we have now - should we switch the two
> outer loops when we do the lookup?

I'm fairly sure that the order here is correct because this program:

    #include <stdio.h>

    int
    main(void)
    {
        int in_key_flow, ip_dst_flow, ip_src;
        for (in_key_flow = 0; in_key_flow < 2; in_key_flow++) {
            for (ip_dst_flow = 0; ip_dst_flow < 2; ip_dst_flow++) {
                for (ip_src = 0; ip_src < 3; ip_src++) {
                    printf("%d %d %d\n", in_key_flow, ip_dst_flow, ip_src);
                }
            }
        }
        return 0;
    }

prints an order that matches the patterns[] array (with lines
deleted):

    0 0 0
    0 0 1
    1 0 0
    1 0 1
    1 1 1
    1 1 2

I'll look this over a bit and properly submit it.