[ovs-dev] [PATCH] datapath: Remove flow refcount functionality.

[Pravin Shelar]

On Mon, Nov 5, 2012 at 3:50 PM, Jesse Gross <jesse at nicira.com> wrote:

> Header caching previously required the ability to maintain the lifetime
> of flows across RCU boundaries.  However, now that header caching is
> gone we can simplfy the code and make it match the upstream version.
>
> Signed-off-by: Jesse Gross <jesse at nicira.com>
> ---
>  datapath/datapath.c |    6 +++---
>  datapath/flow.c     |   38 +++++++++++---------------------------
>  datapath/flow.h     |    7 +------
>  3 files changed, 15 insertions(+), 36 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 9c253e1..976a5f2 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -818,13 +818,13 @@ static int ovs_packet_cmd_execute(struct sk_buff
> *skb, struct genl_info *info)
>         local_bh_enable();
>         rcu_read_unlock();
>
> -       ovs_flow_put(flow);
> +       ovs_flow_free(flow);
>         return err;
>
>  err_unlock:
>         rcu_read_unlock();
>  err_flow_put:
> -       ovs_flow_put(flow);
> +       ovs_flow_free(flow);
>

In upstream OVS label is err_flow_free.



>  err_kfree_skb:
>         kfree_skb(packet);
>  err:
> @@ -1139,7 +1139,7 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff
> *skb, struct genl_info *info)
>         return 0;
>
>  error_free_flow:
> -       ovs_flow_put(flow);
> +       ovs_flow_free(flow);
>  error:
>         return error;
>  }
> diff --git a/datapath/flow.c b/datapath/flow.c
> index 44e71e6..de42ace 100644
> --- a/datapath/flow.c
> +++ b/datapath/flow.c
> @@ -226,9 +226,7 @@ struct sw_flow *ovs_flow_alloc(void)
>                 return ERR_PTR(-ENOMEM);
>
>         spin_lock_init(&flow->lock);
> -       atomic_set(&flow->refcnt, 1);
>         flow->sf_acts = NULL;
> -       flow->dead = false;
>
>         return flow;
>  }
> @@ -290,12 +288,6 @@ struct flow_table *ovs_flow_tbl_alloc(int new_size)
>         return table;
>  }
>
> -static void flow_free(struct sw_flow *flow)
> -{
> -       flow->dead = true;
> -       ovs_flow_put(flow);
> -}
> -
>  void ovs_flow_tbl_destroy(struct flow_table *table)
>  {
>         int i;
> @@ -314,7 +306,7 @@ void ovs_flow_tbl_destroy(struct flow_table *table)
>
>                 hlist_for_each_entry_safe(flow, node, n, head,
> hash_node[ver]) {
>                         hlist_del_rcu(&flow->hash_node[ver]);
> -                       flow_free(flow);
> +                       ovs_flow_free(flow);
>                 }
>         }
>
> @@ -418,13 +410,21 @@ struct flow_table *ovs_flow_tbl_expand(struct
> flow_table *table)
>         return __flow_tbl_rehash(table, table->n_buckets * 2);
>  }
>
> +void ovs_flow_free(struct sw_flow *flow)
> +{
> +       if (unlikely(!flow))
> +               return;
> +
> +       kfree((struct sf_flow_acts __force *)flow->sf_acts);
> +       kmem_cache_free(flow_cache, flow);
> +}
> +
>  /* RCU callback used by ovs_flow_deferred_free. */
>  static void rcu_free_flow_callback(struct rcu_head *rcu)
>  {
>         struct sw_flow *flow = container_of(rcu, struct sw_flow, rcu);
>
> -       flow->dead = true;
> -       ovs_flow_put(flow);
> +       ovs_flow_free(flow);
>  }
>
>  /* Schedules 'flow' to be freed after the next RCU grace period.
> @@ -434,22 +434,6 @@ void ovs_flow_deferred_free(struct sw_flow *flow)
>         call_rcu(&flow->rcu, rcu_free_flow_callback);
>  }
>
> -void ovs_flow_hold(struct sw_flow *flow)
> -{
> -       atomic_inc(&flow->refcnt);
> -}
> -
> -void ovs_flow_put(struct sw_flow *flow)
> -{
> -       if (unlikely(!flow))
> -               return;
> -
> -       if (atomic_dec_and_test(&flow->refcnt)) {
> -               kfree((struct sf_flow_acts __force *)flow->sf_acts);
> -               kmem_cache_free(flow_cache, flow);
> -       }
> -}
> -
>  /* RCU callback used by ovs_flow_deferred_free_acts. */
>  static void rcu_free_acts_callback(struct rcu_head *rcu)
>  {
> diff --git a/datapath/flow.h b/datapath/flow.h
> index f4ef285..8bc6db6 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
> @@ -104,9 +104,6 @@ struct sw_flow {
>         struct sw_flow_key key;
>         struct sw_flow_actions __rcu *sf_acts;
>
> -       atomic_t refcnt;
> -       bool dead;
> -
>         spinlock_t lock;        /* Lock for values below. */
>         unsigned long used;     /* Last used time (in jiffies). */
>         u64 packet_count;       /* Number of packets matched. */
> @@ -133,13 +130,11 @@ void ovs_flow_exit(void);
>
>  struct sw_flow *ovs_flow_alloc(void);
>  void ovs_flow_deferred_free(struct sw_flow *);
> +void ovs_flow_free(struct sw_flow *);
>
>  struct sw_flow_actions *ovs_flow_actions_alloc(const struct nlattr *);
>  void ovs_flow_deferred_free_acts(struct sw_flow_actions *);
>
> -void ovs_flow_hold(struct sw_flow *);
> -void ovs_flow_put(struct sw_flow *);
> -
>  int ovs_flow_extract(struct sk_buff *, u16 in_port, struct sw_flow_key *,
>                      int *key_lenp);
>  void ovs_flow_used(struct sw_flow *, struct sk_buff *);
> --
> 1.7.9.5
>
> otherwise looks good.
Acked-by: Pravin B Shelar <pshelar at nicira.com>


> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20121105/4ec7bcf0/attachment-0003.html>