[ovs-dev] [mask array v6 1/2] datapath: simplify ovs_flow_tbl_lookup_stats()

[Pravin Shelar]

On Mon, Jun 16, 2014 at 1:18 PM, Andy Zhou <azhou at nicira.com> wrote:
> Simplify flow mask cache replacement without using expensive atomic
> memory access to the mask pointers.
>
> Signed-off-by: Andy Zhou <azhou at nicira.com>
> ---
>  datapath/flow_table.c | 44 +++++++++++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/datapath/flow_table.c b/datapath/flow_table.c
> index 58a25c7..c448eec 100644
> --- a/datapath/flow_table.c
> +++ b/datapath/flow_table.c
> @@ -554,8 +554,9 @@ struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table *tbl,
>  {
>         struct mask_array *ma = rcu_dereference_ovsl(tbl->mask_array);
>         struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
> -       struct mask_cache_entry  *entries, *ce, *del;
> +       struct mask_cache_entry  *entries, *ce;
Can you also remove double space.

>         struct sw_flow *flow;
> +       struct sw_flow_mask *cache;
>         u32 hash = skb_hash;
>         int seg;
>
> @@ -566,42 +567,39 @@ struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table *tbl,
>                 return flow_lookup(tbl, ti, ma, key, n_mask_hit, &mask_index);
>         }
>
> -       del = NULL;
> +       ce = NULL;
> +       cache = NULL;
can you move sw_flow_mask *cache to if {} block inside for {} ? It
also saves setting NULL.

>         entries = this_cpu_ptr(tbl->mask_cache);
>
> +       /* Find the cache entry 'ce' to operate on. */
>         for (seg = 0; seg < MC_HASH_SEGS; seg++) {
> -               int index;
> -
> -               index = hash & (MC_HASH_ENTRIES - 1);
> -               ce = &entries[index];
> -
> -               if (ce->skb_hash == skb_hash) {
> -                       struct sw_flow_mask *mask;
> -
> -                       mask = rcu_dereference_ovsl(ma->masks[ce->mask_index]);
> -                       if (mask) {
> -                               flow = masked_flow_lookup(ti, key, mask,
> +               int index = hash & (MC_HASH_ENTRIES - 1);
> +               struct mask_cache_entry *e;
> +
> +               e = &entries[index];
> +               if (e->skb_hash == skb_hash) {
> +                       cache = rcu_dereference_ovsl(ma->masks[e->mask_index]);
> +                       if (cache) {
> +                               flow = masked_flow_lookup(ti, key, cache,
>                                                           n_mask_hit);
> -                               if (flow)  /* Found */
> +                               if (flow) /* Cache hit. */
>                                         return flow;
> -
>                         }
> -                       del = ce;
> +                       e->skb_hash = 0;
> +                       ce = e;  /* The best cache replacement candidate. */
>                         break;
>                 }
>
> -               if (!del || (del->skb_hash && !ce->skb_hash) ||
> -                   (rcu_dereference_ovsl(ma->masks[del->mask_index]) &&
> -                   !rcu_dereference_ovsl(ma->masks[ce->mask_index]))) {
> -                       del = ce;
> -               }
> +               if (!ce || e->skb_hash < ce->skb_hash)
> +                       ce = e;  /* A better replacement cache candidate. */
>
>                 hash >>= MC_HASH_SHIFT;
>         }
>
> -       flow = flow_lookup(tbl, ti, ma, key, n_mask_hit, &del->mask_index);
> +       /* Cache miss, do full lookup. */
> +       flow = flow_lookup(tbl, ti, ma, key, n_mask_hit, &ce->mask_index);
>         if (flow)
> -               del->skb_hash = skb_hash;
> +               ce->skb_hash = skb_hash;
>
>         return flow;
>  }

Looks good.

Acked-by: Pravin B Shelar <pshelar at nicira.com>


> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev