[ovs-dev] [ovs-discuss] [ACLv2 11/19] flow: Decouple datapath flow structure from userspace flow.

[Jesse Gross]

Ben Pfaff wrote:
> Jesse Gross <jesse at nicira.com> writes:
>
>   
>> Components of certain flows can only be matched in userspace.
>> Previously the datapath and userspace shared the same structure
>> for describing flows.  This seperates that structure to allow for
>> additional components to be added to the userspace version.
>>     
>
> "separates" has two "a"s (sorry, pet peeve).
>   

Spelling has never been my forte...

> First, thank you for separating the kernel and userspace flow
> structures.  I knew that this would need to be done eventually.
> That's the main reason that I invented flow_t, in fact: because I
> knew that the userspace structure should be different from the
> kernel structure, and yet I didn't want to create a new "struct"
> and have to deal with copying back and forth yet.
>
> Let's look at the actual flow structure first
>
>   
>> -typedef struct odp_flow_key flow_t;
>> +struct userspace_flow {
>> +    union {
>> +        struct {
>> +            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[ETH_ALEN];  /* Ethernet source address. */
>> +            uint8_t  dl_dst[ETH_ALEN];  /* Ethernet destination address. */
>> +            uint8_t  nw_proto;          /* IP protocol. */
>> +            uint8_t  reserved;          /* Pad to 64 bits. */
>> +        };
>> +        struct odp_flow_key odp;
>> +    };
>> +};
>> +
>> +typedef struct userspace_flow flow_t;
>>     
>
> Is there a reason for putting an anonymous union inside the
> struct?  I think that you could drop the outer struct entirely
> and just call this "union userspace_flow".  --Actually, never
> mind, I see that you add members to the outer struct in a later
> commit.  That makes sense.
>
> But: what's the motivation for the union?  I don't quite follow.
> What does a flow_t represent?  If I have a flow_t, then what do I
> really have?
>   

The motivation was to reduce copying overhead.  By making the kernel 
data structure a subset of the userspace structure, I could eliminate an 
extra copy in one direction.  However, we're going to need to completely 
decouple the two at some point (and I actually ended up making bigger 
changes already) so I just took out the union and did the copying.

> How attached are you to the name "userspace_flow"?  I'd prefer
> "struct ovs_flow" or just "struct flow" myself.  They are just as
> descriptive, I think, and much shorter.
>
>   

I never really liked the name anyways.  It's now "struct flow".

> Over time I'd prefer to migrate away from the flow_t typedef to
> the actual struct name.  No rush though.
>
>   

Agreed, though I left it for the future.

>> +    flow_from_odp(key, &userspace_flow);
>>      HMAP_FOR_EACH_WITH_HASH (flow, struct dp_netdev_flow, node,
>> -                             flow_hash(key, 0), &dp->flow_table) {
>> -        if (flow_equal(&flow->key, key)) {
>> +                             flow_hash(&userspace_flow, 0), &dp->flow_table) {
>> +        if (memcmp(&flow->key, key, sizeof *key)) {
>>     
>
> Surely that should be !memcmp(...)?
>
>   

Yes, thank you.

>> @@ -791,6 +793,7 @@ add_flow(struct dpif *dpif, struct odp_flow *odp_flow)
>>      struct dp_netdev *dp = get_dp_netdev(dpif);
>>      struct dp_netdev_flow *flow;
>>      int error;
>> +    flow_t userspace_flow;
>>  
>>      flow = xcalloc(1, sizeof *flow);
>>      flow->key = odp_flow->key;
>> @@ -802,7 +805,8 @@ add_flow(struct dpif *dpif, struct odp_flow *odp_flow)
>>          return error;
>>      }
>>  
>> -    hmap_insert(&dp->flow_table, &flow->node, flow_hash(&flow->key, 0));
>> +    flow_from_odp(&flow->key, &userspace_flow);
>> +    hmap_insert(&dp->flow_table, &flow->node, flow_hash(&userspace_flow, 0));
>>      return 0;
>>  }
>>  
>>     
>
> Why is dpif_netdev using flow_t at all?  Logically it fits into
> the switch the same as the kernel datapath module, so shouldn't
> it just use struct odp_flow_key?  I think that there was a logic
> error here originally using flow_t instead of struct
> odp_flow_key that we should fix.
>
>   

This is more just laziness on my part.  There were some helper functions 
for flow_t that it was using, so I was just converting it to a flow_t.  
I made odp_flow_key versions to make this more logically correct.

>> diff --git a/lib/flow.c b/lib/flow.c
>> index eaf7036..6952392 100644
>> --- a/lib/flow.c
>> +++ b/lib/flow.c
>> @@ -289,6 +289,13 @@ flow_from_match(flow_t *flow, uint32_t *wildcards,
>>      flow->reserved = 0;
>>  }
>>  
>> +void
>> +flow_from_odp(const struct odp_flow_key *odp, flow_t *flow)
>> +{
>> +    memset(flow, 0, sizeof *flow);
>> +    flow->odp = *odp;
>>     
>
> Did you check whether GCC optimizes this decently?  (It's pretty
> bad at optimizing memset() and memcpy().)  If not, most of 'flow'
> will end up being initialized twice.
>
> I don't know whether this is a frequent enough operation to
> matter.
>
>   

I'm now copying it member-by-member since the kernel version is no 
longer a direct subset of the userspace version.  However, I doubt this 
affects performance all that much.  There's basically one copy of a 40 
byte structure per flow setup.

The one place where it might affect performance is in the userspace 
datapath.  It introduces an extra copy for every packet, simply because 
it is using the userspace version of extract_flow and it copies it to a 
kernel flow structure.  I'm not sure this really matters though, given 
all of the overhead anyways in the userspace datapath.

>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index 672bfe5..578b892 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -1562,10 +1562,16 @@ static struct rule *
>>  rule_create_subrule(struct ofproto *ofproto, struct rule *rule,
>>                      const flow_t *flow)
>>  {
>> -    struct rule *subrule = rule_create(rule, NULL, 0,
>> -                                       rule->idle_timeout, rule->hard_timeout);
>> +    flow_t kernel_flow;
>> +    struct rule *subrule;
>> +
>> +    /* Reduce the flow to only those components that the kernel can match. */
>> +    flow_from_odp(&flow->odp, &kernel_flow);
>>     
>
> That's confusing: we have a struct userspace_flow (flow_t) named
> kernel_flow.  How odd, is everything factored out correctly?
>   

This is correct, just a bad choice of names.  It's now called 
"installable_flow" - still not a great name but maybe a little better.  
It's basically a userspace flow structure that contains only those 
elements that the kernel can handle.