Re: [Patch net-next] net_sched: only create filter chains for new filters/actions

[Jiri Pirko <jiri@resnulli.us>]

Wed, May 24, 2017 at 05:53:42PM CEST, xiyou.wangcong@gmail.com wrote:
>On Tue, May 23, 2017 at 11:37 PM, Jiri Pirko <jiri@resnulli.us> wrote:
>> Tue, May 23, 2017 at 06:42:37PM CEST, xiyou.wangcong@gmail.com wrote:
>>>tcf_chain_get() always creates a new filter chain if not found
>>>in existing ones. This is totally unnecessary when we get or
>>>delete filters, new chain should be only created for new filters
>>>(or new actions).
>>>
>>>Fixes: 5bc1701881e3 ("net: sched: introduce multichain support for filters")
>>>Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>>>Cc: Jiri Pirko <jiri@mellanox.com>
>>>Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>>---
>>> include/net/pkt_cls.h |  3 ++-
>>> net/sched/act_api.c   |  2 +-
>>> net/sched/cls_api.c   | 13 +++++++++----
>>> 3 files changed, 12 insertions(+), 6 deletions(-)
>>>
>>>diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
>>>index 2c213a6..f776229 100644
>>>--- a/include/net/pkt_cls.h
>>>+++ b/include/net/pkt_cls.h
>>>@@ -18,7 +18,8 @@ int register_tcf_proto_ops(struct tcf_proto_ops *ops);
>>> int unregister_tcf_proto_ops(struct tcf_proto_ops *ops);
>>>
>>> #ifdef CONFIG_NET_CLS
>>>-struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index);
>>>+struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
>>>+                              bool create);
>>> void tcf_chain_put(struct tcf_chain *chain);
>>> int tcf_block_get(struct tcf_block **p_block,
>>>                 struct tcf_proto __rcu **p_filter_chain);
>>>diff --git a/net/sched/act_api.c b/net/sched/act_api.c
>>>index 0ecf2a8..aed6cf2 100644
>>>--- a/net/sched/act_api.c
>>>+++ b/net/sched/act_api.c
>>>@@ -34,7 +34,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);
>>>+      a->goto_chain = tcf_chain_get(tp->chain->block, chain_index, true);
>>>       if (!a->goto_chain)
>>>               return -ENOMEM;
>>>       return 0;
>>>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>>>index 01a8b8b..23d2236 100644
>>>--- a/net/sched/cls_api.c
>>>+++ b/net/sched/cls_api.c
>>>@@ -220,7 +220,8 @@ static void tcf_chain_destroy(struct tcf_chain *chain)
>>>       kfree(chain);
>>> }
>>>
>>>-struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index)
>>>+struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index,
>>>+                              bool create)
>>> {
>>>       struct tcf_chain *chain;
>>>
>>>@@ -230,7 +231,10 @@ struct tcf_chain *tcf_chain_get(struct tcf_block *block, u32 chain_index)
>>>                       return chain;
>>>               }
>>>       }
>>>-      return tcf_chain_create(block, chain_index);
>>>+      if (create)
>>>+              return tcf_chain_create(block, chain_index);
>>>+      else
>>>+              return NULL;
>>> }
>>> EXPORT_SYMBOL(tcf_chain_get);
>>>
>>>@@ -509,9 +513,10 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n,
>>>               err = -EINVAL;
>>>               goto errout;
>>>       }
>>>-      chain = tcf_chain_get(block, chain_index);
>>>+      chain = tcf_chain_get(block, chain_index,
>>>+                            n->nlmsg_type == RTM_NEWTFILTER);
>>
>> First of all, I really hate all these true/false arg dances. Totaly
>> confusing all the time.
>
>Sounds like you are able to understand the code at all.
>Sigh, I bet you never even read the changelog. ;)
>
>
>>
>>
>>
>>>       if (!chain) {
>>>-              err = -ENOMEM;
>>>+              err = n->nlmsg_type == RTM_NEWTFILTER ? -ENOMEM : -EINVAL;
>>
>> Confusing. Please do not obfuscate the code for a corner cases. Thanks.
>
>Either you don't understand the changelog or you don't understand
>ternary conditional operator. I can't help you if the latter.
>
>Sorry.

Heh. Really? All I say is, your patch is not needed at all. All it adds
makes to code harder to understand and no benefit.