netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [Patch net-next] net_sched: only create filter chains for new filters/actions
@ 2017-05-23 16:42 Cong Wang
  2017-05-24  6:37 ` Jiri Pirko
  2017-05-25 16:14 ` David Miller
  0 siblings, 2 replies; 11+ messages in thread
From: Cong Wang @ 2017-05-23 16:42 UTC (permalink / raw)
  To: netdev; +Cc: Cong Wang, Jamal Hadi Salim, Jiri Pirko

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);
 	if (!chain) {
-		err = -ENOMEM;
+		err = n->nlmsg_type == RTM_NEWTFILTER ? -ENOMEM : -EINVAL;
 		goto errout;
 	}
 
-- 
2.5.5

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [Patch net-next] net_sched: only create filter chains for new filters/actions
  2017-05-23 16:42 [Patch net-next] net_sched: only create filter chains for new filters/actions Cong Wang
@ 2017-05-24  6:37 ` Jiri Pirko
  2017-05-24 15:53   ` Cong Wang
  2017-05-25 16:14 ` David Miller
  1 sibling, 1 reply; 11+ messages in thread
From: Jiri Pirko @ 2017-05-24  6:37 UTC (permalink / raw)
  To: Cong Wang; +Cc: netdev, Jamal Hadi Salim, Jiri Pirko

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.



> 	if (!chain) {
>-		err = -ENOMEM;
>+		err = n->nlmsg_type == RTM_NEWTFILTER ? -ENOMEM : -EINVAL;

Confusing. Please do not obfuscate the code for a corner cases. Thanks.



> 		goto errout;
> 	}
> 
>-- 
>2.5.5
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net-next] net_sched: only create filter chains for new filters/actions
  2017-05-24  6:37 ` Jiri Pirko
@ 2017-05-24 15:53   ` Cong Wang
  2017-05-24 20:23     ` Jiri Pirko
  0 siblings, 1 reply; 11+ messages in thread
From: Cong Wang @ 2017-05-24 15:53 UTC (permalink / raw)
  To: Jiri Pirko; +Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net-next] net_sched: only create filter chains for new filters/actions
  2017-05-24 15:53   ` Cong Wang
@ 2017-05-24 20:23     ` Jiri Pirko
  0 siblings, 0 replies; 11+ messages in thread
From: Jiri Pirko @ 2017-05-24 20:23 UTC (permalink / raw)
  To: Cong Wang; +Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net-next] net_sched: only create filter chains for new filters/actions
  2017-05-23 16:42 [Patch net-next] net_sched: only create filter chains for new filters/actions Cong Wang
  2017-05-24  6:37 ` Jiri Pirko
@ 2017-05-25 16:14 ` David Miller
  2017-05-26  5:53   ` Jiri Pirko
  1 sibling, 1 reply; 11+ messages in thread
From: David Miller @ 2017-05-25 16:14 UTC (permalink / raw)
  To: xiyou.wangcong; +Cc: netdev, jhs, jiri

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue, 23 May 2017 09:42:37 -0700

> 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>

Indeed, get and delete requests should not create new objects, ever.

I have pretty much no idea why Jiri is making such a big fuss about
this change, to be quite honest. :-)

Applied, thanks Cong.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net-next] net_sched: only create filter chains for new filters/actions
  2017-05-25 16:14 ` David Miller
@ 2017-05-26  5:53   ` Jiri Pirko
  2017-05-26 14:54     ` David Miller
  0 siblings, 1 reply; 11+ messages in thread
From: Jiri Pirko @ 2017-05-26  5:53 UTC (permalink / raw)
  To: David Miller; +Cc: xiyou.wangcong, netdev, jhs, jiri

Thu, May 25, 2017 at 06:14:56PM CEST, davem@davemloft.net wrote:
>From: Cong Wang <xiyou.wangcong@gmail.com>
>Date: Tue, 23 May 2017 09:42:37 -0700
>
>> 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>
>
>Indeed, get and delete requests should not create new objects, ever.
>
>I have pretty much no idea why Jiri is making such a big fuss about
>this change, to be quite honest. :-)

Because it makes already hard to read code even worse, for *no* benefit.
That's why.


>
>Applied, thanks Cong.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net-next] net_sched: only create filter chains for new filters/actions
  2017-05-26  5:53   ` Jiri Pirko
@ 2017-05-26 14:54     ` David Miller
  2017-05-26 16:55       ` Cong Wang
  2017-05-27 10:02       ` Jiri Pirko
  0 siblings, 2 replies; 11+ messages in thread
From: David Miller @ 2017-05-26 14:54 UTC (permalink / raw)
  To: jiri; +Cc: xiyou.wangcong, netdev, jhs, jiri

From: Jiri Pirko <jiri@resnulli.us>
Date: Fri, 26 May 2017 07:53:52 +0200

> Thu, May 25, 2017 at 06:14:56PM CEST, davem@davemloft.net wrote:
>>From: Cong Wang <xiyou.wangcong@gmail.com>
>>Date: Tue, 23 May 2017 09:42:37 -0700
>>
>>> 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>
>>
>>Indeed, get and delete requests should not create new objects, ever.
>>
>>I have pretty much no idea why Jiri is making such a big fuss about
>>this change, to be quite honest. :-)
> 
> Because it makes already hard to read code even worse, for *no* benefit.
> That's why.

Jiri, if you say the same thing 100 times, it doesn't help anyone
understand your arguments any better.

Creating new objects when a GET or a DEL is requested is flat out
wrong.

And Cong is fixing that.

And I also didn't find the boolean logic hard to understand at all.

It is in fact a very common pattern to pass a "create" boolean into
lookup functions, to tell them whether to create a new object on
lookup failure or not.  And then also to control that boolean via
what kind of netlink request we are processing.

So you tell me what's so bad about his change given the above?

Give me details and real facts, like I just did, rather than vague
statements about "benefit" and "hard to read".

Thank you.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net-next] net_sched: only create filter chains for new filters/actions
  2017-05-26 14:54     ` David Miller
@ 2017-05-26 16:55       ` Cong Wang
  2017-05-27 10:05         ` Jiri Pirko
  2017-05-27 10:02       ` Jiri Pirko
  1 sibling, 1 reply; 11+ messages in thread
From: Cong Wang @ 2017-05-26 16:55 UTC (permalink / raw)
  To: David Miller
  Cc: Jiri Pirko, Linux Kernel Network Developers, Jamal Hadi Salim,
	Jiri Pirko

On Fri, May 26, 2017 at 7:54 AM, David Miller <davem@davemloft.net> wrote:
> And I also didn't find the boolean logic hard to understand at all.
>
> It is in fact a very common pattern to pass a "create" boolean into
> lookup functions, to tell them whether to create a new object on
> lookup failure or not.  And then also to control that boolean via
> what kind of netlink request we are processing.

+10

It is a widely used pattern among the kernel source code.
I'd be surprised if an experienced kernel developer is not
aware of this pattern. ;)

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net-next] net_sched: only create filter chains for new filters/actions
  2017-05-26 14:54     ` David Miller
  2017-05-26 16:55       ` Cong Wang
@ 2017-05-27 10:02       ` Jiri Pirko
  1 sibling, 0 replies; 11+ messages in thread
From: Jiri Pirko @ 2017-05-27 10:02 UTC (permalink / raw)
  To: David Miller; +Cc: xiyou.wangcong, netdev, jhs, jiri

Fri, May 26, 2017 at 04:54:43PM CEST, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Fri, 26 May 2017 07:53:52 +0200
>
>> Thu, May 25, 2017 at 06:14:56PM CEST, davem@davemloft.net wrote:
>>>From: Cong Wang <xiyou.wangcong@gmail.com>
>>>Date: Tue, 23 May 2017 09:42:37 -0700
>>>
>>>> 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>
>>>
>>>Indeed, get and delete requests should not create new objects, ever.
>>>
>>>I have pretty much no idea why Jiri is making such a big fuss about
>>>this change, to be quite honest. :-)
>> 
>> Because it makes already hard to read code even worse, for *no* benefit.
>> That's why.
>
>Jiri, if you say the same thing 100 times, it doesn't help anyone
>understand your arguments any better.
>
>Creating new objects when a GET or a DEL is requested is flat out
>wrong.
 
Allright. I ack that.


>
>And Cong is fixing that.
>
>And I also didn't find the boolean logic hard to understand at all.
>
>It is in fact a very common pattern to pass a "create" boolean into
>lookup functions, to tell them whether to create a new object on
>lookup failure or not.  And then also to control that boolean via
>what kind of netlink request we are processing.
>
>So you tell me what's so bad about his change given the above?
>
>Give me details and real facts, like I just did, rather than vague
>statements about "benefit" and "hard to read".

What I don't like is the double "n->nlmsg_type == RTM_NEWTFILTER" check
and return value decusion according to the latter check. The code logic
is split into tcf_chain_get function and its caller. That is
at least odd.

Since you don't like the PTR_ERR approach, I'll try to figure out how to
do this another way.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net-next] net_sched: only create filter chains for new filters/actions
  2017-05-26 16:55       ` Cong Wang
@ 2017-05-27 10:05         ` Jiri Pirko
  2017-05-30 17:05           ` Cong Wang
  0 siblings, 1 reply; 11+ messages in thread
From: Jiri Pirko @ 2017-05-27 10:05 UTC (permalink / raw)
  To: Cong Wang
  Cc: David Miller, Linux Kernel Network Developers, Jamal Hadi Salim,
	Jiri Pirko

Fri, May 26, 2017 at 06:55:25PM CEST, xiyou.wangcong@gmail.com wrote:
>On Fri, May 26, 2017 at 7:54 AM, David Miller <davem@davemloft.net> wrote:
>> And I also didn't find the boolean logic hard to understand at all.
>>
>> It is in fact a very common pattern to pass a "create" boolean into
>> lookup functions, to tell them whether to create a new object on
>> lookup failure or not.  And then also to control that boolean via
>> what kind of netlink request we are processing.
>
>+10
>
>It is a widely used pattern among the kernel source code.
>I'd be surprised if an experienced kernel developer is not
>aware of this pattern. ;)

Cong, as you wisely put, I'm not aware of this pattern and I'm also
unaware of existence of ternary operator. Are this notes necessary?
Does that make you feel better?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [Patch net-next] net_sched: only create filter chains for new filters/actions
  2017-05-27 10:05         ` Jiri Pirko
@ 2017-05-30 17:05           ` Cong Wang
  0 siblings, 0 replies; 11+ messages in thread
From: Cong Wang @ 2017-05-30 17:05 UTC (permalink / raw)
  To: Jiri Pirko
  Cc: David Miller, Linux Kernel Network Developers, Jamal Hadi Salim,
	Jiri Pirko

On Sat, May 27, 2017 at 3:05 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> Cong, as you wisely put, I'm not aware of this pattern and I'm also
> unaware of existence of ternary operator. Are this notes necessary?
> Does that make you feel better?
>

You are more than just welcome to redirect my email to
your /dev/null. I am quite sure my email is waste for you.

Thanks!!

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-05-30 17:06 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-23 16:42 [Patch net-next] net_sched: only create filter chains for new filters/actions Cong Wang
2017-05-24  6:37 ` Jiri Pirko
2017-05-24 15:53   ` Cong Wang
2017-05-24 20:23     ` Jiri Pirko
2017-05-25 16:14 ` David Miller
2017-05-26  5:53   ` Jiri Pirko
2017-05-26 14:54     ` David Miller
2017-05-26 16:55       ` Cong Wang
2017-05-27 10:05         ` Jiri Pirko
2017-05-30 17:05           ` Cong Wang
2017-05-27 10:02       ` Jiri Pirko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).