[ovs-dev] [PATCH v2 2/4] datapath: Move mega-flow list out of rehashing struct.

Pravin Shelar pshelar at nicira.com
Mon Sep 30 20:46:09 UTC 2013


On Mon, Sep 30, 2013 at 1:16 PM, Jesse Gross <jesse at 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 <pshelar at 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.



More information about the dev mailing list