From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Ricardo Leitner Subject: Re: [PATCH v3 09/11] net: sched: use reference counting action init Date: Mon, 28 May 2018 18:38:48 -0300 Message-ID: <20180528213848.GO3787@localhost.localdomain> References: <20180527175739.GA3337@nanopsycho> <1527455849-22327-1-git-send-email-vladbu@mellanox.com> <1527455849-22327-10-git-send-email-vladbu@mellanox.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: jiri@resnulli.us, netdev@vger.kernel.org, jhs@mojatatu.com, xiyou.wangcong@gmail.com, davem@davemloft.net, ast@kernel.org, daniel@iogearbox.net, kliteyn@mellanox.com To: Vlad Buslov Return-path: Received: from mail-qk0-f196.google.com ([209.85.220.196]:39896 "EHLO mail-qk0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934294AbeE1Viw (ORCPT ); Mon, 28 May 2018 17:38:52 -0400 Received: by mail-qk0-f196.google.com with SMTP id z75-v6so10031520qkb.6 for ; Mon, 28 May 2018 14:38:51 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1527455849-22327-10-git-send-email-vladbu@mellanox.com> Sender: netdev-owner@vger.kernel.org List-ID: On Mon, May 28, 2018 at 12:17:27AM +0300, Vlad Buslov wrote: > Change action API to assume that action init function always takes > reference to action, even when overwriting existing action. This is > necessary because action API continues to use action pointer after init > function is done. At this point action becomes accessible for concurrent > modifications, so user must always hold reference to it. > > Implement helper put list function to atomically release list of actions > after action API init code is done using them. > > Signed-off-by: Vlad Buslov Reviewed-by: Marcelo Ricardo Leitner > --- > Changes from V1 to V2: > - Resplit action lookup/release code to prevent memory leaks in > individual patches. > > net/sched/act_api.c | 35 +++++++++++++++++------------------ > 1 file changed, 17 insertions(+), 18 deletions(-) > > diff --git a/net/sched/act_api.c b/net/sched/act_api.c > index f019f0464cec..eefe8c2fe667 100644 > --- a/net/sched/act_api.c > +++ b/net/sched/act_api.c > @@ -627,6 +627,18 @@ static int tcf_action_put(struct tc_action *p) > return __tcf_action_put(p, false); > } > > +static void tcf_action_put_lst(struct list_head *actions) > +{ > + struct tc_action *a, *tmp; > + > + list_for_each_entry_safe(a, tmp, actions, list) { > + const struct tc_action_ops *ops = a->ops; > + > + if (tcf_action_put(a)) > + module_put(ops->owner); > + } > +} > + > int > tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int bind, int ref) > { > @@ -835,17 +847,6 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, > return ERR_PTR(err); > } > > -static void cleanup_a(struct list_head *actions, int ovr) > -{ > - struct tc_action *a; > - > - if (!ovr) > - return; > - > - list_for_each_entry(a, actions, list) > - refcount_dec(&a->tcfa_refcnt); > -} > - > int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla, > struct nlattr *est, char *name, int ovr, int bind, > struct list_head *actions, size_t *attr_size, > @@ -874,11 +875,6 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla, > } > > *attr_size = tcf_action_full_attrs_size(sz); > - > - /* Remove the temp refcnt which was necessary to protect against > - * destroying an existing action which was being replaced > - */ > - cleanup_a(actions, ovr); > return 0; > > err: > @@ -1209,7 +1205,7 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n, > return ret; > } > err: > - tcf_action_destroy(&actions, 0); > + tcf_action_put_lst(&actions); > return ret; > } > > @@ -1251,8 +1247,11 @@ static int tcf_action_add(struct net *net, struct nlattr *nla, > &attr_size, true, extack); > if (ret) > return ret; > + ret = tcf_add_notify(net, n, &actions, portid, attr_size, extack); > + if (ovr) > + tcf_action_put_lst(&actions); > > - return tcf_add_notify(net, n, &actions, portid, attr_size, extack); > + return ret; > } > > static u32 tcaa_root_flags_allowed = TCA_FLAG_LARGE_DUMP_ON; > -- > 2.7.5 >