| Message ID | 20180727074505.855-1-jiri@resnulli.us |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: sched: don't dump chains only held by actions | expand |
On Fri, Jul 27, 2018 at 12:47 AM Jiri Pirko <jiri@resnulli.us> wrote: > > From: Jiri Pirko <jiri@mellanox.com> > > In case a chain is empty and not explicitly created by a user, > such chain should not exist. The only exception is if there is > an action "goto chain" pointing to it. In that case, don't show the > chain in the dump. Track the chain references held by actions and > use them to find out if a chain should or should not be shown > in chain dump. > > Signed-off-by: Jiri Pirko <jiri@mellanox.com> Looks reasonable to me. Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
On Sat, Jul 28, 2018 at 10:20 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > > On Fri, Jul 27, 2018 at 12:47 AM Jiri Pirko <jiri@resnulli.us> wrote: > > > > From: Jiri Pirko <jiri@mellanox.com> > > > > In case a chain is empty and not explicitly created by a user, > > such chain should not exist. The only exception is if there is > > an action "goto chain" pointing to it. In that case, don't show the > > chain in the dump. Track the chain references held by actions and > > use them to find out if a chain should or should not be shown > > in chain dump. > > > > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > > Looks reasonable to me. > > Acked-by: Cong Wang <xiyou.wangcong@gmail.com> Hold on... If you increase the refcnt for a zombie chain on NEWCHAIN path, then it would become a non-zombie, this makes sense. However, if the action_refcnt gets increased again when another action uses it, it become a zombie again because refcnt==action_refcnt??
Sat, Jul 28, 2018 at 07:39:36PM CEST, xiyou.wangcong@gmail.com wrote: >On Sat, Jul 28, 2018 at 10:20 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: >> >> On Fri, Jul 27, 2018 at 12:47 AM Jiri Pirko <jiri@resnulli.us> wrote: >> > >> > From: Jiri Pirko <jiri@mellanox.com> >> > >> > In case a chain is empty and not explicitly created by a user, >> > such chain should not exist. The only exception is if there is >> > an action "goto chain" pointing to it. In that case, don't show the >> > chain in the dump. Track the chain references held by actions and >> > use them to find out if a chain should or should not be shown >> > in chain dump. >> > >> > Signed-off-by: Jiri Pirko <jiri@mellanox.com> >> >> Looks reasonable to me. >> >> Acked-by: Cong Wang <xiyou.wangcong@gmail.com> > >Hold on... > >If you increase the refcnt for a zombie chain on NEWCHAIN path, >then it would become a non-zombie, this makes sense. However, >if the action_refcnt gets increased again when another action uses it, >it become a zombie again because refcnt==action_refcnt?? No. action always increases both refcnt and action_refcnt
On Sun, Jul 29, 2018 at 12:54 AM Jiri Pirko <jiri@resnulli.us> wrote: > > Sat, Jul 28, 2018 at 07:39:36PM CEST, xiyou.wangcong@gmail.com wrote: > >On Sat, Jul 28, 2018 at 10:20 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > >> > >> On Fri, Jul 27, 2018 at 12:47 AM Jiri Pirko <jiri@resnulli.us> wrote: > >> > > >> > From: Jiri Pirko <jiri@mellanox.com> > >> > > >> > In case a chain is empty and not explicitly created by a user, > >> > such chain should not exist. The only exception is if there is > >> > an action "goto chain" pointing to it. In that case, don't show the > >> > chain in the dump. Track the chain references held by actions and > >> > use them to find out if a chain should or should not be shown > >> > in chain dump. > >> > > >> > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > >> > >> Looks reasonable to me. > >> > >> Acked-by: Cong Wang <xiyou.wangcong@gmail.com> > > > >Hold on... > > > >If you increase the refcnt for a zombie chain on NEWCHAIN path, > >then it would become a non-zombie, this makes sense. However, > >if the action_refcnt gets increased again when another action uses it, > >it become a zombie again because refcnt==action_refcnt?? > > No. action always increases both refcnt and action_refcnt Hmm, then the name zombie is confusing, with your definition all chains implicitly created by actions are zombies, unless touched by user explicitly. Please find a better name. Also, tcf_chain_get_by_act() could send out RTM_NEWCHAIN too, which is confusing too as it is still a "zombie".
Mon, Jul 30, 2018 at 08:19:56PM CEST, xiyou.wangcong@gmail.com wrote: >On Sun, Jul 29, 2018 at 12:54 AM Jiri Pirko <jiri@resnulli.us> wrote: >> >> Sat, Jul 28, 2018 at 07:39:36PM CEST, xiyou.wangcong@gmail.com wrote: >> >On Sat, Jul 28, 2018 at 10:20 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: >> >> >> >> On Fri, Jul 27, 2018 at 12:47 AM Jiri Pirko <jiri@resnulli.us> wrote: >> >> > >> >> > From: Jiri Pirko <jiri@mellanox.com> >> >> > >> >> > In case a chain is empty and not explicitly created by a user, >> >> > such chain should not exist. The only exception is if there is >> >> > an action "goto chain" pointing to it. In that case, don't show the >> >> > chain in the dump. Track the chain references held by actions and >> >> > use them to find out if a chain should or should not be shown >> >> > in chain dump. >> >> > >> >> > Signed-off-by: Jiri Pirko <jiri@mellanox.com> >> >> >> >> Looks reasonable to me. >> >> >> >> Acked-by: Cong Wang <xiyou.wangcong@gmail.com> >> > >> >Hold on... >> > >> >If you increase the refcnt for a zombie chain on NEWCHAIN path, >> >then it would become a non-zombie, this makes sense. However, >> >if the action_refcnt gets increased again when another action uses it, >> >it become a zombie again because refcnt==action_refcnt?? >> >> No. action always increases both refcnt and action_refcnt > >Hmm, then the name zombie is confusing, with your definition all >chains implicitly created by actions are zombies, unless touched >by user explicitly. Please find a better name. Okay. Perhaps chain_inactive? > >Also, tcf_chain_get_by_act() could send out RTM_NEWCHAIN too, >which is confusing too as it is still a "zombie". Will check.
On Tue, 31 Jul 2018 08:32:58 +0200, Jiri Pirko wrote: > Mon, Jul 30, 2018 at 08:19:56PM CEST, xiyou.wangcong@gmail.com wrote: > >On Sun, Jul 29, 2018 at 12:54 AM Jiri Pirko <jiri@resnulli.us> wrote: > >> > >> Sat, Jul 28, 2018 at 07:39:36PM CEST, xiyou.wangcong@gmail.com wrote: > >> >On Sat, Jul 28, 2018 at 10:20 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: > >> >> > >> >> On Fri, Jul 27, 2018 at 12:47 AM Jiri Pirko <jiri@resnulli.us> wrote: > >> >> > > >> >> > From: Jiri Pirko <jiri@mellanox.com> > >> >> > > >> >> > In case a chain is empty and not explicitly created by a user, > >> >> > such chain should not exist. The only exception is if there is > >> >> > an action "goto chain" pointing to it. In that case, don't show the > >> >> > chain in the dump. Track the chain references held by actions and > >> >> > use them to find out if a chain should or should not be shown > >> >> > in chain dump. > >> >> > > >> >> > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > >> >> > >> >> Looks reasonable to me. > >> >> > >> >> Acked-by: Cong Wang <xiyou.wangcong@gmail.com> > >> > > >> >Hold on... > >> > > >> >If you increase the refcnt for a zombie chain on NEWCHAIN path, > >> >then it would become a non-zombie, this makes sense. However, > >> >if the action_refcnt gets increased again when another action uses it, > >> >it become a zombie again because refcnt==action_refcnt?? > >> > >> No. action always increases both refcnt and action_refcnt > > > >Hmm, then the name zombie is confusing, with your definition all > >chains implicitly created by actions are zombies, unless touched > >by user explicitly. Please find a better name. > > Okay. Perhaps chain_inactive? FWIW to me active brings to mind that it's handling traffic. Brining in my suggestions from an off-list discussion: tcf_chain_act_refs_only() or tcf_chain_pure_act_target() or maybe tcf_chain_has_no_filters() ?
Tue, Jul 31, 2018 at 10:01:46AM CEST, jakub.kicinski@netronome.com wrote: >On Tue, 31 Jul 2018 08:32:58 +0200, Jiri Pirko wrote: >> Mon, Jul 30, 2018 at 08:19:56PM CEST, xiyou.wangcong@gmail.com wrote: >> >On Sun, Jul 29, 2018 at 12:54 AM Jiri Pirko <jiri@resnulli.us> wrote: >> >> >> >> Sat, Jul 28, 2018 at 07:39:36PM CEST, xiyou.wangcong@gmail.com wrote: >> >> >On Sat, Jul 28, 2018 at 10:20 AM Cong Wang <xiyou.wangcong@gmail.com> wrote: >> >> >> >> >> >> On Fri, Jul 27, 2018 at 12:47 AM Jiri Pirko <jiri@resnulli.us> wrote: >> >> >> > >> >> >> > From: Jiri Pirko <jiri@mellanox.com> >> >> >> > >> >> >> > In case a chain is empty and not explicitly created by a user, >> >> >> > such chain should not exist. The only exception is if there is >> >> >> > an action "goto chain" pointing to it. In that case, don't show the >> >> >> > chain in the dump. Track the chain references held by actions and >> >> >> > use them to find out if a chain should or should not be shown >> >> >> > in chain dump. >> >> >> > >> >> >> > Signed-off-by: Jiri Pirko <jiri@mellanox.com> >> >> >> >> >> >> Looks reasonable to me. >> >> >> >> >> >> Acked-by: Cong Wang <xiyou.wangcong@gmail.com> >> >> > >> >> >Hold on... >> >> > >> >> >If you increase the refcnt for a zombie chain on NEWCHAIN path, >> >> >then it would become a non-zombie, this makes sense. However, >> >> >if the action_refcnt gets increased again when another action uses it, >> >> >it become a zombie again because refcnt==action_refcnt?? >> >> >> >> No. action always increases both refcnt and action_refcnt >> > >> >Hmm, then the name zombie is confusing, with your definition all >> >chains implicitly created by actions are zombies, unless touched >> >by user explicitly. Please find a better name. >> >> Okay. Perhaps chain_inactive? > >FWIW to me active brings to mind that it's handling traffic. Brining in >my suggestions from an off-list discussion: > >tcf_chain_act_refs_only() or tcf_chain_pure_act_target() :/ > >or maybe tcf_chain_has_no_filters() ? That is not accurate, as explicitly created chain does not have any filters too. I think this is good: tcf_chain_held_by_acts_only()
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h index a3101582f642..6d02f31abba8 100644 --- a/include/net/pkt_cls.h +++ b/include/net/pkt_cls.h @@ -39,7 +39,10 @@ bool tcf_queue_work(struct rcu_work *rwork, work_func_t func); #ifdef CONFIG_NET_CLS struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index, bool create); +struct tcf_chain *tcf_chain_get_by_act(struct tcf_block *block, + u32 chain_index); void tcf_chain_put(struct tcf_chain *chain); +void tcf_chain_put_by_act(struct tcf_chain *chain); void tcf_block_netif_keep_dst(struct tcf_block *block); int tcf_block_get(struct tcf_block **p_block, struct tcf_proto __rcu **p_filter_chain, struct Qdisc *q, diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index 085c509c8674..c5432362dc26 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -314,6 +314,7 @@ struct tcf_chain { struct tcf_block *block; u32 index; /* chain index */ unsigned int refcnt; + unsigned int action_refcnt; bool explicitly_created; const struct tcf_proto_ops *tmplt_ops; void *tmplt_priv; diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 148a89ab789b..b43df1e25c6d 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -36,7 +36,7 @@ static int tcf_action_goto_chain_init(struct tc_action *a, struct tcf_proto *tp) if (!tp) return -EINVAL; - a->goto_chain = tcf_chain_get(tp->chain->block, chain_index, true); + a->goto_chain = tcf_chain_get_by_act(tp->chain->block, chain_index); if (!a->goto_chain) return -ENOMEM; return 0; @@ -44,7 +44,7 @@ static int tcf_action_goto_chain_init(struct tc_action *a, struct tcf_proto *tp) static void tcf_action_goto_chain_fini(struct tc_action *a) { - tcf_chain_put(a->goto_chain); + tcf_chain_put_by_act(a->goto_chain); } static void tcf_action_goto_chain_exec(const struct tc_action *a, diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 75cce2819de9..e20aad1987b8 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -262,6 +262,25 @@ static void tcf_chain_hold(struct tcf_chain *chain) ++chain->refcnt; } +static void tcf_chain_hold_by_act(struct tcf_chain *chain) +{ + ++chain->action_refcnt; +} + +static void tcf_chain_release_by_act(struct tcf_chain *chain) +{ + --chain->action_refcnt; +} + +static bool tcf_chain_is_zombie(struct tcf_chain *chain) +{ + /* In case all the references are action references, this + * chain is a zombie and should not be listed in the chain + * dump list. + */ + return chain->refcnt == chain->action_refcnt; +} + static struct tcf_chain *tcf_chain_lookup(struct tcf_block *block, u32 chain_index) { @@ -298,6 +317,15 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index, } EXPORT_SYMBOL(tcf_chain_get); +struct tcf_chain *tcf_chain_get_by_act(struct tcf_block *block, u32 chain_index) +{ + struct tcf_chain *chain = tcf_chain_get(block, chain_index, true); + + tcf_chain_hold_by_act(chain); + return chain; +} +EXPORT_SYMBOL(tcf_chain_get_by_act); + static void tc_chain_tmplt_del(struct tcf_chain *chain); void tcf_chain_put(struct tcf_chain *chain) @@ -310,6 +338,13 @@ void tcf_chain_put(struct tcf_chain *chain) } EXPORT_SYMBOL(tcf_chain_put); +void tcf_chain_put_by_act(struct tcf_chain *chain) +{ + tcf_chain_release_by_act(chain); + tcf_chain_put(chain); +} +EXPORT_SYMBOL(tcf_chain_put_by_act); + static void tcf_chain_put_explicitly_created(struct tcf_chain *chain) { if (chain->explicitly_created) @@ -1803,20 +1838,29 @@ static int tc_ctl_chain(struct sk_buff *skb, struct nlmsghdr *n, chain = tcf_chain_lookup(block, chain_index); if (n->nlmsg_type == RTM_NEWCHAIN) { if (chain) { - NL_SET_ERR_MSG(extack, "Filter chain already exists"); - return -EEXIST; - } - if (!(n->nlmsg_flags & NLM_F_CREATE)) { - NL_SET_ERR_MSG(extack, "Need both RTM_NEWCHAIN and NLM_F_CREATE to create a new chain"); - return -ENOENT; - } - chain = tcf_chain_create(block, chain_index); - if (!chain) { - NL_SET_ERR_MSG(extack, "Failed to create filter chain"); - return -ENOMEM; + if (tcf_chain_is_zombie(chain)) { + /* The chain exists only because there is + * some action referencing it, meaning it + * is a zombie. + */ + tcf_chain_hold(chain); + } else { + NL_SET_ERR_MSG(extack, "Filter chain already exists"); + return -EEXIST; + } + } else { + if (!(n->nlmsg_flags & NLM_F_CREATE)) { + NL_SET_ERR_MSG(extack, "Need both RTM_NEWCHAIN and NLM_F_CREATE to create a new chain"); + return -ENOENT; + } + chain = tcf_chain_create(block, chain_index); + if (!chain) { + NL_SET_ERR_MSG(extack, "Failed to create filter chain"); + return -ENOMEM; + } } } else { - if (!chain) { + if (!chain || tcf_chain_is_zombie(chain)) { NL_SET_ERR_MSG(extack, "Cannot find specified filter chain"); return -EINVAL; } @@ -1944,6 +1988,8 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb) index++; continue; } + if (tcf_chain_is_zombie(chain)) + continue; err = tc_chain_fill_node(chain, net, skb, block, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq, NLM_F_MULTI,