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 <vla...@mellanox.com>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leit...@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 >