On Mon, Sep 30, 2013 at 1:16 PM, Jesse Gross <je...@nicira.com> wrote:
> This on the previous version of the patch since I had already started
> writing it before you sent out the new version:
>
> On Thu, Sep 26, 2013 at 9:01 AM, Pravin B Shelar <pshe...@nicira.com> wrote:
>> ovs-flow rehash does not touch mega flow list. Following patch
>> moves it dp struct datapath. Avoid one extra indirection for
>> accessing mega-flow list head on every packet receive.
>
> Does this actually reduce the number of cache misses? When processing
> packets we don't touch the mask list without also touching the
> buckets, which still require a dereference.
>
It might reduce cache miss, depends on actual memory allocation. But
it does avoid indirection for accessing list_head.
>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>> index 13002b0..ff572ac 100644
>> --- a/datapath/datapath.c
>> +++ b/datapath/datapath.c
>> @@ -1010,10 +969,9 @@ static int ovs_flow_cmd_del(struct sk_buff *skb,
>> struct genl_info *info)
>> if (err)
>> goto unlock;
>>
>> - table = ovsl_dereference(dp->table);
>> - flow = ovs_flow_lookup_unmasked_key(table, &match);
>> - if (!flow) {
>> - err = -ENOENT;
>> + flow = ovs_flow_lookup_unmasked_key(&dp->table, &match);
>> + if (IS_ERR(flow)) {
>> + err = PTR_ERR(flow);
>
> This looks suspicious to me since I think
> ovs_flow_lookup_unmasked_key() returns either a flow or NULL.
>
right.
>> static int ovs_flow_cmd_dump(struct sk_buff *skb, struct netlink_callback
>> *cb)
>> {
>> struct ovs_header *ovs_header = genlmsg_data(nlmsg_data(cb->nlh));
>> + struct hash_table *htable;
>> struct datapath *dp;
>> - struct flow_table *table;
>>
>> rcu_read_lock();
>> dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
>> @@ -1051,15 +1009,15 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb,
>> struct netlink_callback *cb)
>> rcu_read_unlock();
>> return -ENODEV;
>> }
>> + htable = rcu_dereference(dp->table.htable);
>>
>> - table = rcu_dereference(dp->table);
>> for (;;) {
>> struct sw_flow *flow;
>> u32 bucket, obj;
>>
>> bucket = cb->args[0];
>> obj = cb->args[1];
>> - flow = ovs_flow_dump_next(table, &bucket, &obj);
>> + flow = ovs_flow_dump_next(htable, &bucket, &obj);
>
> Do we need to pull out htable here? It seems like it would be cleaner
> if ovs_flow_dump_next() was able to get it directly without needing to
> expose the internals like other places.
>
I thought abt that but this might give different result if
rehashing/expansion is going on in parallel.
so I decided to keep current behavior.
>> diff --git a/datapath/flow_table.c b/datapath/flow_table.c
>> index cfabc44..f7655fa 100644
>> --- a/datapath/flow_table.c
>> +++ b/datapath/flow_table.c
>> -static void __flow_tbl_destroy(struct flow_table *table)
>> +static void __flow_tbl_destroy(struct hash_table *table)
>
> We might want to rename these functions now that they are
> allocating/freeing hash_tables, rather than flow tables.
>
ok.
>> diff --git a/datapath/flow_table.h b/datapath/flow_table.h
>> index 619ead6..6c3355e 100644
>> --- a/datapath/flow_table.h
>> +++ b/datapath/flow_table.h
>> -struct flow_table {
>> +struct hash_table {
>
> hash_table is a pretty generic name - is there something more
> descriptive that we can use?
>
>> struct flex_array *buckets;
>> unsigned int count, n_buckets;
>> struct rcu_head rcu;
>> - struct list_head *mask_list;
>> int node_ver;
>> u32 hash_seed;
>> bool keep_flows;
>> };
>
> Are there any other fields that we can move into struct flow_table,
> such as count or keep_flows?
>
I can move count to flow_table. keep_flows is related to hash table so
I will keep it as is.
>> @@ -97,4 +90,5 @@ struct sw_flow_mask *ovs_sw_flow_mask_find(const struct
>> flow_table *,
>> void ovs_flow_key_mask(struct sw_flow_key *dst, const struct sw_flow_key
>> *src,
>> const struct sw_flow_mask *mask);
>>
>> +int ovs_flush_flows(struct flow_table *flow_table);
>
> Can we group this with some related functions - maybe the insert and
> delete ones?
yes, it is done in v3.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
