On Mon, Jun 16, 2014 at 1:18 PM, Andy Zhou <az...@nicira.com> wrote:
> Simplify flow mask cache replacement without using expensive atomic
> memory access to the mask pointers.
>
> Signed-off-by: Andy Zhou <az...@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 <pshe...@nicira.com>
> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
