Re: [PATCH v3 02/11] net: sched: change type of reference and bind counters

[Marcelo Ricardo Leitner]

On Mon, May 28, 2018 at 12:17:20AM +0300, Vlad Buslov wrote:
> Change type of action reference counter to refcount_t.
> 
> Change type of action bind counter to atomic_t.
> This type is used to allow decrementing bind counter without testing
> for 0 result.
> 
> Signed-off-by: Vlad Buslov 
> Signed-off-by: Jiri Pirko 

Reviewed-by: Marcelo Ricardo Leitner 

> ---
>  include/net/act_api.h  |  5 +++--
>  net/sched/act_api.c| 32 ++--
>  net/sched/act_bpf.c|  4 ++--
>  net/sched/act_connmark.c   |  4 ++--
>  net/sched/act_csum.c   |  4 ++--
>  net/sched/act_gact.c   |  4 ++--
>  net/sched/act_ife.c|  4 ++--
>  net/sched/act_ipt.c|  4 ++--
>  net/sched/act_mirred.c |  4 ++--
>  net/sched/act_nat.c|  4 ++--
>  net/sched/act_pedit.c  |  4 ++--
>  net/sched/act_police.c |  4 ++--
>  net/sched/act_sample.c |  4 ++--
>  net/sched/act_simple.c |  4 ++--
>  net/sched/act_skbedit.c|  4 ++--
>  net/sched/act_skbmod.c |  4 ++--
>  net/sched/act_tunnel_key.c |  4 ++--
>  net/sched/act_vlan.c   |  4 ++--
>  18 files changed, 57 insertions(+), 44 deletions(-)
> 
> diff --git a/include/net/act_api.h b/include/net/act_api.h
> index f7b59ef7303d..e634014605cb 100644
> --- a/include/net/act_api.h
> +++ b/include/net/act_api.h
> @@ -6,6 +6,7 @@
>   * Public action API for classifiers/qdiscs
>  */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -26,8 +27,8 @@ struct tc_action {
>   struct tcf_idrinfo  *idrinfo;
>  
>   u32 tcfa_index;
> - int tcfa_refcnt;
> - int tcfa_bindcnt;
> + refcount_t  tcfa_refcnt;
> + atomic_ttcfa_bindcnt;
>   u32 tcfa_capab;
>   int tcfa_action;
>   struct tcf_ttcfa_tm;
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 02670c7489e3..4f064ecab882 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -105,14 +105,26 @@ int __tcf_idr_release(struct tc_action *p, bool bind, 
> bool strict)
>  
>   ASSERT_RTNL();
>  
> + /* Release with strict==1 and bind==0 is only called through act API
> +  * interface (classifiers always bind). Only case when action with
> +  * positive reference count and zero bind count can exist is when it was
> +  * also created with act API (unbinding last classifier will destroy the
> +  * action if it was created by classifier). So only case when bind count
> +  * can be changed after initial check is when unbound action is
> +  * destroyed by act API while classifier binds to action with same id
> +  * concurrently. This result either creation of new action(same behavior
> +  * as before), or reusing existing action if concurrent process
> +  * increments reference count before action is deleted. Both scenarios
> +  * are acceptable.
> +  */
>   if (p) {
>   if (bind)
> - p->tcfa_bindcnt--;
> - else if (strict && p->tcfa_bindcnt > 0)
> + atomic_dec(>tcfa_bindcnt);
> + else if (strict && atomic_read(>tcfa_bindcnt) > 0)
>   return -EPERM;
>  
> - p->tcfa_refcnt--;
> - if (p->tcfa_bindcnt <= 0 && p->tcfa_refcnt <= 0) {
> + if (atomic_read(>tcfa_bindcnt) <= 0 &&
> + refcount_dec_and_test(>tcfa_refcnt)) {
>   if (p->ops->cleanup)
>   p->ops->cleanup(p);
>   tcf_idr_remove(p->idrinfo, p);
> @@ -304,8 +316,8 @@ bool tcf_idr_check(struct tc_action_net *tn, u32 index, 
> struct tc_action **a,
>  
>   if (index && p) {
>   if (bind)
> - p->tcfa_bindcnt++;
> - p->tcfa_refcnt++;
> + atomic_inc(>tcfa_bindcnt);
> + refcount_inc(>tcfa_refcnt);
>   *a = p;
>   return true;
>   }
> @@ -324,9 +336,9 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, 
> struct nlattr *est,
>  
>   if (unlikely(!p))
>   return -ENOMEM;
> - p->tcfa_refcnt = 1;
> + refcount_set(>tcfa_refcnt, 1);
>   if (bind)
> - p->tcfa_bindcnt = 1;
> + atomic_set(>tcfa_bindcnt, 1);
>  
>   if (cpustats) {
>   p->cpu_bstats = netdev_alloc_pcpu_stats(struct 
> gnet_stats_basic_cpu);
> @@ -782,7 +794,7 @@ static void cleanup_a(struct list_head *actions, int ovr)
>   return;
>  
>   list_for_each_entry(a, actions, list)
> - a->tcfa_refcnt--;
> + refcount_dec(>tcfa_refcnt);
>  }
>  
>  int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr 
> *nla,
> @@ -810,7 +822,7 @@ int tcf_action_init(struct net *net, struct tcf_proto 
> *tp, 

[PATCH v3 02/11] net: sched: change type of reference and bind counters

[Vlad Buslov]

Change type of action reference counter to refcount_t.

Change type of action bind counter to atomic_t.
This type is used to allow decrementing bind counter without testing
for 0 result.

Signed-off-by: Vlad Buslov 
Signed-off-by: Jiri Pirko 
---
 include/net/act_api.h  |  5 +++--
 net/sched/act_api.c| 32 ++--
 net/sched/act_bpf.c|  4 ++--
 net/sched/act_connmark.c   |  4 ++--
 net/sched/act_csum.c   |  4 ++--
 net/sched/act_gact.c   |  4 ++--
 net/sched/act_ife.c|  4 ++--
 net/sched/act_ipt.c|  4 ++--
 net/sched/act_mirred.c |  4 ++--
 net/sched/act_nat.c|  4 ++--
 net/sched/act_pedit.c  |  4 ++--
 net/sched/act_police.c |  4 ++--
 net/sched/act_sample.c |  4 ++--
 net/sched/act_simple.c |  4 ++--
 net/sched/act_skbedit.c|  4 ++--
 net/sched/act_skbmod.c |  4 ++--
 net/sched/act_tunnel_key.c |  4 ++--
 net/sched/act_vlan.c   |  4 ++--
 18 files changed, 57 insertions(+), 44 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index f7b59ef7303d..e634014605cb 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -6,6 +6,7 @@
  * Public action API for classifiers/qdiscs
 */
 
+#include 
 #include 
 #include 
 #include 
@@ -26,8 +27,8 @@ struct tc_action {
struct tcf_idrinfo  *idrinfo;
 
u32 tcfa_index;
-   int tcfa_refcnt;
-   int tcfa_bindcnt;
+   refcount_t  tcfa_refcnt;
+   atomic_ttcfa_bindcnt;
u32 tcfa_capab;
int tcfa_action;
struct tcf_ttcfa_tm;
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 02670c7489e3..4f064ecab882 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -105,14 +105,26 @@ int __tcf_idr_release(struct tc_action *p, bool bind, 
bool strict)
 
ASSERT_RTNL();
 
+   /* Release with strict==1 and bind==0 is only called through act API
+* interface (classifiers always bind). Only case when action with
+* positive reference count and zero bind count can exist is when it was
+* also created with act API (unbinding last classifier will destroy the
+* action if it was created by classifier). So only case when bind count
+* can be changed after initial check is when unbound action is
+* destroyed by act API while classifier binds to action with same id
+* concurrently. This result either creation of new action(same behavior
+* as before), or reusing existing action if concurrent process
+* increments reference count before action is deleted. Both scenarios
+* are acceptable.
+*/
if (p) {
if (bind)
-   p->tcfa_bindcnt--;
-   else if (strict && p->tcfa_bindcnt > 0)
+   atomic_dec(>tcfa_bindcnt);
+   else if (strict && atomic_read(>tcfa_bindcnt) > 0)
return -EPERM;
 
-   p->tcfa_refcnt--;
-   if (p->tcfa_bindcnt <= 0 && p->tcfa_refcnt <= 0) {
+   if (atomic_read(>tcfa_bindcnt) <= 0 &&
+   refcount_dec_and_test(>tcfa_refcnt)) {
if (p->ops->cleanup)
p->ops->cleanup(p);
tcf_idr_remove(p->idrinfo, p);
@@ -304,8 +316,8 @@ bool tcf_idr_check(struct tc_action_net *tn, u32 index, 
struct tc_action **a,
 
if (index && p) {
if (bind)
-   p->tcfa_bindcnt++;
-   p->tcfa_refcnt++;
+   atomic_inc(>tcfa_bindcnt);
+   refcount_inc(>tcfa_refcnt);
*a = p;
return true;
}
@@ -324,9 +336,9 @@ int tcf_idr_create(struct tc_action_net *tn, u32 index, 
struct nlattr *est,
 
if (unlikely(!p))
return -ENOMEM;
-   p->tcfa_refcnt = 1;
+   refcount_set(>tcfa_refcnt, 1);
if (bind)
-   p->tcfa_bindcnt = 1;
+   atomic_set(>tcfa_bindcnt, 1);
 
if (cpustats) {
p->cpu_bstats = netdev_alloc_pcpu_stats(struct 
gnet_stats_basic_cpu);
@@ -782,7 +794,7 @@ static void cleanup_a(struct list_head *actions, int ovr)
return;
 
list_for_each_entry(a, actions, list)
-   a->tcfa_refcnt--;
+   refcount_dec(>tcfa_refcnt);
 }
 
 int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
@@ -810,7 +822,7 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, 
struct nlattr *nla,
act->order = i;
sz += tcf_action_fill_size(act);
if (ovr)
-   act->tcfa_refcnt++;
+