Re: [PATCH v3 07/11] net: sched: implement reference counted action release

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Mon, May 28, 2018 at 12:17:25AM +0300, Vlad Buslov wrote:
> Implement helper delete function that uses new action ops 'delete', instead
> of destroying action directly. This is required so act API could delete
> actions by index, without holding any references to action that is being
> deleted.
> 
> Implement function __tcf_action_put() that releases reference to action and
> frees it, if necessary. Refactor action deletion code to use new put
> function and not to rely on rtnl lock. Remove rtnl lock assertions that are
> no longer needed.
> 
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
> Changes from V1 to V2:
> - Removed redundant actions ops lookup during delete.
> - Assume all actions have delete implemented and don't check for it
>   explicitly.
> - Rearrange variable definitions in tcf_action_delete.
> 
>  net/sched/act_api.c | 84 +++++++++++++++++++++++++++++++++++++++--------------
>  net/sched/cls_api.c |  1 -
>  2 files changed, 62 insertions(+), 23 deletions(-)
> 
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 0f31f09946ab..a023873db713 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -90,21 +90,39 @@ static void free_tcf(struct tc_action *p)
>  	kfree(p);
>  }
>  
> -static void tcf_idr_remove(struct tcf_idrinfo *idrinfo, struct tc_action *p)
> +static void tcf_action_cleanup(struct tc_action *p)
>  {
> -	spin_lock(&idrinfo->lock);
> -	idr_remove(&idrinfo->action_idr, p->tcfa_index);
> -	spin_unlock(&idrinfo->lock);
> +	if (p->ops->cleanup)
> +		p->ops->cleanup(p);
> +
>  	gen_kill_estimator(&p->tcfa_rate_est);
>  	free_tcf(p);
>  }
>  
> +static int __tcf_action_put(struct tc_action *p, bool bind)
> +{
> +	struct tcf_idrinfo *idrinfo = p->idrinfo;
> +
> +	if (refcount_dec_and_lock(&p->tcfa_refcnt, &idrinfo->lock)) {
> +		if (bind)
> +			atomic_dec(&p->tcfa_bindcnt);
> +		idr_remove(&idrinfo->action_idr, p->tcfa_index);
> +		spin_unlock(&idrinfo->lock);
> +
> +		tcf_action_cleanup(p);
> +		return 1;
> +	}
> +
> +	if (bind)
> +		atomic_dec(&p->tcfa_bindcnt);
> +
> +	return 0;
> +}
> +
>  int __tcf_idr_release(struct tc_action *p, bool bind, bool strict)
>  {
>  	int ret = 0;
>  
> -	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
> @@ -118,18 +136,11 @@ int __tcf_idr_release(struct tc_action *p, bool bind, bool strict)
>  	 * are acceptable.
>  	 */
>  	if (p) {
> -		if (bind)
> -			atomic_dec(&p->tcfa_bindcnt);
> -		else if (strict && atomic_read(&p->tcfa_bindcnt) > 0)
> +		if (!bind && strict && atomic_read(&p->tcfa_bindcnt) > 0)
>  			return -EPERM;
>  
> -		if (atomic_read(&p->tcfa_bindcnt) <= 0 &&
> -		    refcount_dec_and_test(&p->tcfa_refcnt)) {
> -			if (p->ops->cleanup)
> -				p->ops->cleanup(p);
> -			tcf_idr_remove(p->idrinfo, p);
> +		if (__tcf_action_put(p, bind))
>  			ret = ACT_P_DELETED;
> -		}
>  	}
>  
>  	return ret;
> @@ -340,11 +351,7 @@ int tcf_idr_delete_index(struct tc_action_net *tn, u32 index)
>  						p->tcfa_index));
>  			spin_unlock(&idrinfo->lock);
>  
> -			if (p->ops->cleanup)
> -				p->ops->cleanup(p);
> -
> -			gen_kill_estimator(&p->tcfa_rate_est);
> -			free_tcf(p);
> +			tcf_action_cleanup(p);
>  			module_put(owner);
>  			return 0;
>  		}
> @@ -615,6 +622,11 @@ int tcf_action_destroy(struct list_head *actions, int bind)
>  	return ret;
>  }
>  
> +static int tcf_action_put(struct tc_action *p)
> +{
> +	return __tcf_action_put(p, false);
> +}
> +
>  int
>  tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int bind, int ref)
>  {
> @@ -1092,6 +1104,35 @@ static int tca_action_flush(struct net *net, struct nlattr *nla,
>  	return err;
>  }
>  
> +static int tcf_action_delete(struct net *net, struct list_head *actions,
> +			     struct netlink_ext_ack *extack)
> +{
> +	struct tc_action *a, *tmp;
> +	u32 act_index;
> +	int ret;
> +
> +	list_for_each_entry_safe(a, tmp, actions, list) {
> +		const struct tc_action_ops *ops = a->ops;
> +
> +		/* Actions can be deleted concurrently so we must save their
> +		 * type and id to search again after reference is released.
> +		 */
> +		act_index = a->tcfa_index;
> +
> +		list_del(&a->list);
> +		if (tcf_action_put(a)) {
> +			/* last reference, action was deleted concurrently */
> +			module_put(ops->owner);
> +		} else  {
> +			/* now do the delete */
> +			ret = ops->delete(net, act_index);
> +			if (ret < 0)
> +				return ret;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int
>  tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
>  	       u32 portid, size_t attr_size, struct netlink_ext_ack *extack)
> @@ -1112,7 +1153,7 @@ tcf_del_notify(struct net *net, struct nlmsghdr *n, struct list_head *actions,
>  	}
>  
>  	/* now do the delete */
> -	ret = tcf_action_destroy(actions, 0);
> +	ret = tcf_action_delete(net, actions, extack);
>  	if (ret < 0) {
>  		NL_SET_ERR_MSG(extack, "Failed to delete TC action");
>  		kfree_skb(skb);
> @@ -1164,7 +1205,6 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
>  	if (event == RTM_GETACTION)
>  		ret = tcf_get_notify(net, portid, n, &actions, event, extack);
>  	else { /* delete */
> -		cleanup_a(&actions, 1); /* lookup took reference */
>  		ret = tcf_del_notify(net, n, &actions, portid, attr_size, extack);
>  		if (ret)
>  			goto err;
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index efbf01ce14c2..dd37d0ae3fce 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -1417,7 +1417,6 @@ void tcf_exts_destroy(struct tcf_exts *exts)
>  #ifdef CONFIG_NET_CLS_ACT
>  	LIST_HEAD(actions);
>  
> -	ASSERT_RTNL();
>  	tcf_exts_to_list(exts, &actions);
>  	tcf_action_destroy(&actions, TCA_ACT_UNBIND);
>  	kfree(exts->actions);
> -- 
> 2.7.5
>