[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openvswitch-discuss
Subject:    [ovs-discuss] [ACLv2 11/19] flow: Decouple datapath flow structure from userspace flow.
From:       blp () nicira ! com (Ben Pfaff)
Date:       2009-08-25 23:10:34
Message-ID: 87my5nwkut.fsf () blp ! benpfaff ! org
[Download RAW message or body]

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).

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?

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.

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

> +    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(...)?

> @@ -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.

> 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.

> 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?


[prev in list] [next in list] [prev in thread] [next in thread]