[ovs-dev] [PATCH 2/3] datapath: Mega flow implementation

[Jesse Gross]

On Fri, Jun 7, 2013 at 3:35 AM, Andy Zhou <azhou at nicira.com> wrote:
> On Thu, Jun 6, 2013 at 7:01 PM, Jesse Gross <jesse at nicira.com> wrote:
>> On Wed, Jun 5, 2013 at 10:46 PM, Andy Zhou <azhou at nicira.com> wrote:
>> > diff --git a/datapath/datapath.c b/datapath/datapath.c
>> > index 42af315..98d78a8 100644
>> > --- a/datapath/datapath.c
>> > +++ b/datapath/datapath.c
>> > @@ -1138,14 +1142,27 @@ static int ovs_flow_cmd_fill_info(struct sw_flow
>> > *flow, struct datapath *dp,
>> [...]
>> > +    nla = nla_nest_start(skb, OVS_FLOW_ATTR_MASK);
>> > +    if (!nla)
>> > +        goto nla_put_failure;
>> > +
>> > +    err = ovs_flow_to_nlattrs(&flow->key,
>> > +                              &flow->mfm->key, skb);
>> > +    if (err)
>> > +        goto error;
>> > +
>> > +    nla_nest_end(skb, nla);
>>
>> It looks to me like we don't allocate any extra space in the buffer
>> for the mask when called ovs_flow_cmd_build_info().
>
> Do you mean ovs_dp_cmd_msg_size() does not include the size of mask (and
> key) attributes?

Yes, I although I think it is ovs_flow_cmd_msg_size().

>> > @@ -1267,7 +1297,23 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff
>> > *skb, struct genl_info *info)
>> >                 goto err_unlock_ovs;
>> >
>> >         table = ovsl_dereference(dp->table);
>> > -       flow = ovs_flow_tbl_lookup(table, &key, key_len);
>> > +
>> > +       /* Deduplicate the mega flow mask. Note ovs_mutex is held. */
>> > +       existing_mfm = sw_flow_mask_find(mfm);
>> > +       if (existing_mfm) {
>> > +               flow = ovs_flow_tbl_lookup(table, &key, existing_mfm,
>> > key_len);
>> > +               if (flow) {
>> > +                       if (!ovs_flow_cmp_id(flow, &key, key_len)) {
>> > +                               error = -EINVAL;
>> > +                               goto err_unlock_ovs;
>> > +                       }
>>
>> I believe this allows for inserting the same flow with multiple masks
>> but this isn't really good because the order of the masks (and
>> therefore priority) is somewhat arbitrary. If we just do an lookup
>> similar to how a packet is processed on the exact flow then it seems a
>> little more robust and means that we can eliminate the need to have a
>> second hash table.
>
> So just look for any existing flow with key only. The mask attribute is only
> be used
> for creating a new flow?

Yes, that's what I was thinking.

>> > diff --git a/datapath/flow.h b/datapath/flow.h
>> > index dba66cf..9fb5a1c 100644
>> > --- a/datapath/flow.h
>> > +++ b/datapath/flow.h
>> > @@ -117,9 +119,13 @@ struct sw_flow_key {
>> >  struct sw_flow {
>> >         struct rcu_head rcu;
>> >         struct hlist_node hash_node[2];
>> > +       struct hlist_node hash_node_id[2];
>>
>> I don't think there is a need to have two instances of hash_node_id
>> because it is only accessed under lock. The reason for having two in
>> packet lookup is to avoid a hiccup in traffic when we move a flow from
>> one table to another. However, there are no such concurrent accesses
>> in the lookup table.
>
> It seems we are going down the path of the removing the hash_node_id
> entirely, right?

Yes, I just mentioned in case there was a reason not to go that direction.

>> > +struct sw_flow_desc {
>> > +       size_t start;
>> > +       size_t end;
>> > +};
>>
>> I found the name "desc" to not be very intuitive - is the another name
>> that is more self-explanatory?
>
> How about sw_flow_key_range? Any other suggestions?

Sure, that seems better.