On Fri, Jun 7, 2013 at 3:35 AM, Andy Zhou <az...@nicira.com> wrote:
> On Thu, Jun 6, 2013 at 7:01 PM, Jesse Gross <je...@nicira.com> wrote:
>> On Wed, Jun 5, 2013 at 10:46 PM, Andy Zhou <az...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
