[ovs-dev] [netlink 05/16] flow: Separate "flow_t" from "struct odp_flow_key".

[Jesse Gross]

On Mon, Oct 11, 2010 at 1:09 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Fri, Sep 24, 2010 at 01:22:03PM -0700, Jesse Gross wrote:
>> I'm surprised to see the use of struct odp_flow_key here.  I wasn't
>> expecting to see it used outside of dpif-linux.c  I imagined that the
>> contents of datapath-protocol.h would be moving to be a Linux-specific
>> userspace/kernel interface.  The upcoming commits still use a fairly
>> monolithic structure for the flows to the kernel but you could imagine
>> it being broken apart as well.  If we keep using this structure
>> throughout userspace then there is very little benefit to decoupling
>> and it adds an extra structure for people to be aware of.  If you
>> think of the kernel datapath becoming less special and merely one of
>> many (including hardware) then this is even more true: every datapath
>> can transform the nominal flow structure into whatever format it
>> needs, everywhere else we should use flow_t.
>
> I agree that we should move toward datapath-implementation-neutral data
> structures.  But that would be a much larger change, because
> odp_flow_key is contained within a number of other data structures.  The
> interface to the dpif_*() functions would need to change to use them,
> and so would the implementations and the callers, of course.
>
> My goal in this patch was just to take one step in the right direction.

I would probably move in that direction sooner, rather than later.
You already had to rename all of the userspace callers once in your
later patch to match the versioned type for Netlink.  If we want to
have any kind of independence between kernel and userspace versions
(one of the goals of moving to Netlink), we're going to need this.
Seeing as both data structures were one before the patch, is it
actually that different from renaming some of the callers?  We just
need to introduce some conversion functions in dpif-linux.c, right?

>
>> > diff --git a/lib/flow.h b/lib/flow.h
>> > index 603c4ac..7b5f144 100644
>> > -typedef struct odp_flow_key flow_t;
>> > +typedef struct flow flow_t;
>> > +struct flow {
>> > +    uint32_t tun_id;            /* Encapsulating tunnel ID. */
>> > +    uint32_t nw_src;            /* IP source address. */
>> > +    uint32_t nw_dst;            /* IP destination address. */
>> > +    uint16_t in_port;           /* Input switch port. */
>> > +    uint16_t dl_vlan;           /* Input VLAN. */
>> > +    uint16_t dl_type;           /* Ethernet frame type. */
>> > +    uint16_t tp_src;            /* TCP/UDP source port. */
>> > +    uint16_t tp_dst;            /* TCP/UDP destination port. */
>> > +    uint8_t dl_src[6];          /* Ethernet source address. */
>> > +    uint8_t dl_dst[6];          /* Ethernet destination address. */
>> > +    uint8_t nw_proto;           /* IP protocol or low 8 bits of ARP opcode. */
>> > +    uint8_t dl_vlan_pcp;        /* Input VLAN priority. */
>> > +    uint8_t nw_tos;             /* IP ToS (DSCP field, 6 bits). */
>> > +};
>> > +
>> > +/* Assert that there are FLOW_SIG_SIZE bytes of significant data in "struct
>> > + * flow", followed by FLOW_PAD_SIZE bytes of padding. */
>> > +#define FLOW_SIG_SIZE 37
>> > +#define FLOW_PAD_SIZE 3
>> > +BUILD_ASSERT_DECL(offsetof(struct flow, nw_tos) == FLOW_SIG_SIZE - 1);
>> > +BUILD_ASSERT_DECL(sizeof(((struct flow *)0)->nw_tos) == 1);
>> > +BUILD_ASSERT_DECL(sizeof(struct flow) == FLOW_SIG_SIZE + FLOW_PAD_SIZE);
>>
>> Assuming the amount of padding seems oddly compiler and architecture
>> dependent.  Is there that much benefit to not
>> memsetting/packing/comparing fields, etc.?
>
> It's theoretically compiler and architecture dependent but in practice
> I don't know of any interesting architecture for OVS that won't pad this
> structure this way.

This is probably fine for now.