[ovs-dev] [RFC PATCH 1/5] flow: Extend struct flow to contain tunnel outer header.

[Jesse Gross]

On Fri, Sep 28, 2012 at 12:54 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Fri, Sep 28, 2012 at 11:12:21AM -0700, Jesse Gross wrote:
>> Now that the kernel is supplying information about the outer IP
>> header for tunneled packets userspace needs to be able to track it
>> as part of the flow.  For the time being this is only used internally
>> by OVS and not exposed outwards to OpenFlow.  As a result, this
>> threads the information throughout userspace but simply stores the
>> existing tun_id in it.
>>
>> Signed-off-by: Jesse Gross <jesse at nicira.com>
>
> Is this gracefully extensible if later we need to support IPv6 tunnel
> endpoints?

We'll need to add extra space for the longer addresses and update the
tunnel code that cares, of course, but I don't think there is any
other real work.

> It took me a while to figure out the purpose of the 'tnl' parameter to
> flow_extract().  I think it would have been more obvious if the
> parameter were marked 'const'.

OK.

> If you can be satisfied with 16 bits of flags (only 3 flags exist so
> far) then struct flow_tnl can be reduced by 4 bytes, including 2 bytes
> of padding.

On 64-bit machines that would still result in the compiler adding 4
bytes of padding since it's no long 8-byte aligned.  However, this
patch already makes sure that we memset the whole struct so we can
take the benefit on 32-bit machines.

> Should struct flow_metadata get a struct flow_tnl member?  I guess it
> depends on whether we plan to expose anything other than tunnel ID via
> OpenFlow.

There are no current plans for any of this to be visible to OpenFlow.

> format_tunnel_flags() is kind of oddly written.  It doesn't need to be
> a loop if you dropped the "else"s and put "if (flags)" before the
> final "else" clause.

Hmm, yes, I'm not sure why I wrote it that way.

> The flow_format() output might be easier to read if it omitted all
> tunnel information if there isn't any, instead of writing "tunnel(0)".

It's fine to get rid of it.

> match_format() doesn't print any of the new fields.  I assume that's
> intentional?

They're not currently part of the match.

> In process_packet_in(),
>     memset(&tnl, 0, sizeof tnl);
>     tnl.tun_id = pi.fmd.tun_id;
>
>     flow_extract(&pkt, 0, &tnl, pi.fmd.in_port, &flow);
> might more simply be written as:
>     flow_extract(&pkt, 0, NULL, pi.fmd.in_port, &flow);
>     flow.tunnel.tun_id = pi.fmd.tun_id;
> and I do see it written that way in other places.
> Similarly in ofproto_unixctl_trace().

Yes, I guess that's cleaner.