On Tue, May 23, 2017 at 11:37 PM, Jiri Pirko <j...@resnulli.us> wrote:
> Tue, May 23, 2017 at 06:42:37PM CEST, xiyou.wangc...@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 <j...@mojatatu.com>
>>Cc: Jiri Pirko <j...@mellanox.com>
>>Signed-off-by: Cong Wang <xiyou.wangc...@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.
