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

Jesse Gross jesse at nicira.com
Mon Sep 30 20:16:45 UTC 2013


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.

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

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

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

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

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



More information about the dev mailing list