[patch net-next 0/6] sched: cls_api: small cleanup

[patch net-next 0/6] sched: cls_api: small cleanup

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

This patchset makes couple of things in cls_api code a bit nicer and easier
for reader to digest.

Jiri Pirko (6):
  sched: rename tcf_destroy to tcf_destroy_proto
  sched: move tcf_proto_destroy and tcf_destroy_chain helpers into
    cls_api
  sched: push TC filter protocol creation into a separate function
  sched: move err set right before goto errout in tc_ctl_tfilter
  sched: add missing curly braces in else branch in tc_ctl_tfilter
  sched: check negative err value to safe one level of indent

 include/net/pkt_cls.h     |   2 +
 include/net/sch_generic.h |   2 -
 net/sched/cls_api.c       | 191 +++++++++++++++++++++++++++-------------------
 net/sched/sch_api.c       |  22 ------
 net/sched/sch_atm.c       |   1 +
 net/sched/sch_cbq.c       |   1 +
 net/sched/sch_choke.c     |   1 +
 net/sched/sch_dsmark.c    |   1 +
 net/sched/sch_fq_codel.c  |   1 +
 net/sched/sch_htb.c       |   1 +
 net/sched/sch_ingress.c   |   1 +
 net/sched/sch_multiq.c    |   2 +-
 net/sched/sch_prio.c      |   2 +-
 net/sched/sch_sfb.c       |   1 +
 net/sched/sch_sfq.c       |   1 +
 15 files changed, 126 insertions(+), 104 deletions(-)

-- 
2.7.4

[patch net-next 1/6] sched: rename tcf_destroy to tcf_destroy_proto

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

This function destroys TC filter protocol, not TC filter. So name it
accordingly.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/sch_generic.h | 2 +-
 net/sched/cls_api.c       | 8 ++++----
 net/sched/sch_api.c       | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index e2f426f..4533506 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -405,7 +405,7 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue,
 				const struct Qdisc_ops *ops, u32 parentid);
 void __qdisc_calculate_pkt_len(struct sk_buff *skb,
 			       const struct qdisc_size_table *stab);
-bool tcf_destroy(struct tcf_proto *tp, bool force);
+bool tcf_proto_destroy(struct tcf_proto *tp, bool force);
 void tcf_destroy_chain(struct tcf_proto __rcu **fl);
 int skb_do_redirect(struct sk_buff *);
 
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 1ecdf80..90536eb 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -323,7 +323,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
 
 			tfilter_notify(net, skb, n, tp, fh,
 				       RTM_DELTFILTER, false);
-			tcf_destroy(tp, true);
+			tcf_proto_destroy(tp, true);
 			err = 0;
 			goto errout;
 		}
@@ -338,7 +338,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
 			err = -EEXIST;
 			if (n->nlmsg_flags & NLM_F_EXCL) {
 				if (tp_created)
-					tcf_destroy(tp, true);
+					tcf_proto_destroy(tp, true);
 				goto errout;
 			}
 			break;
@@ -350,7 +350,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
 				tfilter_notify(net, skb, n, tp,
 					       t->tcm_handle,
 					       RTM_DELTFILTER, false);
-				if (tcf_destroy(tp, false))
+				if (tcf_proto_destroy(tp, false))
 					RCU_INIT_POINTER(*back, next);
 			}
 			goto errout;
@@ -374,7 +374,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
 		tfilter_notify(net, skb, n, tp, fh, RTM_NEWTFILTER, false);
 	} else {
 		if (tp_created)
-			tcf_destroy(tp, true);
+			tcf_proto_destroy(tp, true);
 	}
 
 errout:
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index ef53ede..f30b517 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1900,7 +1900,7 @@ int tc_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 }
 EXPORT_SYMBOL(tc_classify);
 
-bool tcf_destroy(struct tcf_proto *tp, bool force)
+bool tcf_proto_destroy(struct tcf_proto *tp, bool force)
 {
 	if (tp->ops->destroy(tp, force)) {
 		module_put(tp->ops->owner);
@@ -1917,7 +1917,7 @@ void tcf_destroy_chain(struct tcf_proto __rcu **fl)
 
 	while ((tp = rtnl_dereference(*fl)) != NULL) {
 		RCU_INIT_POINTER(*fl, tp->next);
-		tcf_destroy(tp, true);
+		tcf_proto_destroy(tp, true);
 	}
 }
 EXPORT_SYMBOL(tcf_destroy_chain);
-- 
2.7.4

Re: [patch net-next 1/6] sched: rename tcf_destroy to tcf_destroy_proto

[Jamal Hadi Salim]

On 17-02-09 08:38 AM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> This function destroys TC filter protocol, not TC filter. So name it
> accordingly.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>


Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>


cheers,
jamal

[patch net-next 2/6] sched: move tcf_proto_destroy and tcf_destroy_chain helpers into cls_api

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Creation is done in this file, move destruction to be at the same place.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 include/net/pkt_cls.h     |  2 ++
 include/net/sch_generic.h |  2 --
 net/sched/cls_api.c       | 21 +++++++++++++++++++++
 net/sched/sch_api.c       | 22 ----------------------
 net/sched/sch_atm.c       |  1 +
 net/sched/sch_cbq.c       |  1 +
 net/sched/sch_choke.c     |  1 +
 net/sched/sch_dsmark.c    |  1 +
 net/sched/sch_fq_codel.c  |  1 +
 net/sched/sch_htb.c       |  1 +
 net/sched/sch_ingress.c   |  1 +
 net/sched/sch_multiq.c    |  2 +-
 net/sched/sch_prio.c      |  2 +-
 net/sched/sch_sfb.c       |  1 +
 net/sched/sch_sfq.c       |  1 +
 15 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index dabb00a..71b266c 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -17,6 +17,8 @@ struct tcf_walker {
 int register_tcf_proto_ops(struct tcf_proto_ops *ops);
 int unregister_tcf_proto_ops(struct tcf_proto_ops *ops);
 
+void tcf_destroy_chain(struct tcf_proto __rcu **fl);
+
 static inline unsigned long
 __cls_set_class(unsigned long *clp, unsigned long cl)
 {
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 4533506..aeec408 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -405,8 +405,6 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue,
 				const struct Qdisc_ops *ops, u32 parentid);
 void __qdisc_calculate_pkt_len(struct sk_buff *skb,
 			       const struct qdisc_size_table *stab);
-bool tcf_proto_destroy(struct tcf_proto *tp, bool force);
-void tcf_destroy_chain(struct tcf_proto __rcu **fl);
 int skb_do_redirect(struct sk_buff *);
 
 static inline void skb_reset_tc(struct sk_buff *skb)
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 90536eb..4efa4df 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -127,6 +127,27 @@ static inline u32 tcf_auto_prio(struct tcf_proto *tp)
 	return first;
 }
 
+static bool tcf_proto_destroy(struct tcf_proto *tp, bool force)
+{
+	if (tp->ops->destroy(tp, force)) {
+		module_put(tp->ops->owner);
+		kfree_rcu(tp, rcu);
+		return true;
+	}
+	return false;
+}
+
+void tcf_destroy_chain(struct tcf_proto __rcu **fl)
+{
+	struct tcf_proto *tp;
+
+	while ((tp = rtnl_dereference(*fl)) != NULL) {
+		RCU_INIT_POINTER(*fl, tp->next);
+		tcf_proto_destroy(tp, true);
+	}
+}
+EXPORT_SYMBOL(tcf_destroy_chain);
+
 /* Add/change/delete/get a filter node */
 
 static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index f30b517..adeabae 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1900,28 +1900,6 @@ int tc_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 }
 EXPORT_SYMBOL(tc_classify);
 
-bool tcf_proto_destroy(struct tcf_proto *tp, bool force)
-{
-	if (tp->ops->destroy(tp, force)) {
-		module_put(tp->ops->owner);
-		kfree_rcu(tp, rcu);
-		return true;
-	}
-
-	return false;
-}
-
-void tcf_destroy_chain(struct tcf_proto __rcu **fl)
-{
-	struct tcf_proto *tp;
-
-	while ((tp = rtnl_dereference(*fl)) != NULL) {
-		RCU_INIT_POINTER(*fl, tp->next);
-		tcf_proto_destroy(tp, true);
-	}
-}
-EXPORT_SYMBOL(tcf_destroy_chain);
-
 #ifdef CONFIG_PROC_FS
 static int psched_show(struct seq_file *seq, void *v)
 {
diff --git a/net/sched/sch_atm.c b/net/sched/sch_atm.c
index 481e4f1..2209c2d 100644
--- a/net/sched/sch_atm.c
+++ b/net/sched/sch_atm.c
@@ -15,6 +15,7 @@
 #include <linux/file.h>		/* for fput */
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
 
 /*
  * The ATM queuing discipline provides a framework for invoking classifiers
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index f120758..d6ca18dc 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -19,6 +19,7 @@
 #include <linux/skbuff.h>
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
 
 
 /*	Class-Based Queueing (CBQ) algorithm.
diff --git a/net/sched/sch_choke.c b/net/sched/sch_choke.c
index 3b6d5bd..3b86a97 100644
--- a/net/sched/sch_choke.c
+++ b/net/sched/sch_choke.c
@@ -16,6 +16,7 @@
 #include <linux/skbuff.h>
 #include <linux/vmalloc.h>
 #include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
 #include <net/inet_ecn.h>
 #include <net/red.h>
 #include <net/flow_dissector.h>
diff --git a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
index 1308bbf..802ac7c 100644
--- a/net/sched/sch_dsmark.c
+++ b/net/sched/sch_dsmark.c
@@ -13,6 +13,7 @@
 #include <linux/rtnetlink.h>
 #include <linux/bitops.h>
 #include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
 #include <net/dsfield.h>
 #include <net/inet_ecn.h>
 #include <asm/byteorder.h>
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index 2f50e4c..9f3a884 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -23,6 +23,7 @@
 #include <linux/vmalloc.h>
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
 #include <net/codel.h>
 #include <net/codel_impl.h>
 #include <net/codel_qdisc.h>
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 760f39e..4cd5fb1 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -40,6 +40,7 @@
 #include <net/netlink.h>
 #include <net/sch_generic.h>
 #include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
 
 /* HTB algorithm.
     Author: devik@cdi.cz
diff --git a/net/sched/sch_ingress.c b/net/sched/sch_ingress.c
index 8fe6999..3bab5f6 100644
--- a/net/sched/sch_ingress.c
+++ b/net/sched/sch_ingress.c
@@ -16,6 +16,7 @@
 
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
 
 static struct Qdisc *ingress_leaf(struct Qdisc *sch, unsigned long arg)
 {
diff --git a/net/sched/sch_multiq.c b/net/sched/sch_multiq.c
index 9ffbb02..e7839a0 100644
--- a/net/sched/sch_multiq.c
+++ b/net/sched/sch_multiq.c
@@ -25,7 +25,7 @@
 #include <linux/skbuff.h>
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
-
+#include <net/pkt_cls.h>
 
 struct multiq_sched_data {
 	u16 bands;
diff --git a/net/sched/sch_prio.c b/net/sched/sch_prio.c
index 8f57589..d4d7db2 100644
--- a/net/sched/sch_prio.c
+++ b/net/sched/sch_prio.c
@@ -20,7 +20,7 @@
 #include <linux/skbuff.h>
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
-
+#include <net/pkt_cls.h>
 
 struct prio_sched_data {
 	int bands;
diff --git a/net/sched/sch_sfb.c b/net/sched/sch_sfb.c
index 20a350b..fe6963d 100644
--- a/net/sched/sch_sfb.c
+++ b/net/sched/sch_sfb.c
@@ -25,6 +25,7 @@
 #include <linux/jhash.h>
 #include <net/ip.h>
 #include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
 #include <net/inet_ecn.h>
 
 /*
diff --git a/net/sched/sch_sfq.c b/net/sched/sch_sfq.c
index 7f195ed..83d06e2 100644
--- a/net/sched/sch_sfq.c
+++ b/net/sched/sch_sfq.c
@@ -23,6 +23,7 @@
 #include <linux/vmalloc.h>
 #include <net/netlink.h>
 #include <net/pkt_sched.h>
+#include <net/pkt_cls.h>
 #include <net/red.h>
 
 
-- 
2.7.4

Re: [patch net-next 2/6] sched: move tcf_proto_destroy and tcf_destroy_chain helpers into cls_api

[Jamal Hadi Salim]

On 17-02-09 08:38 AM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Creation is done in this file, move destruction to be at the same place.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>


cheers,
jamal

Re: [patch net-next 2/6] sched: move tcf_proto_destroy and tcf_destroy_chain helpers into cls_api

[Sabrina Dubroca]

Hi Jiri,

2017-02-09, 14:38:56 +0100, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
> 
> Creation is done in this file, move destruction to be at the same place.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  include/net/pkt_cls.h     |  2 ++
>  include/net/sch_generic.h |  2 --
>  net/sched/cls_api.c       | 21 +++++++++++++++++++++
>  net/sched/sch_api.c       | 22 ----------------------
>  net/sched/sch_atm.c       |  1 +
>  net/sched/sch_cbq.c       |  1 +
>  net/sched/sch_choke.c     |  1 +
>  net/sched/sch_dsmark.c    |  1 +
>  net/sched/sch_fq_codel.c  |  1 +
>  net/sched/sch_htb.c       |  1 +
>  net/sched/sch_ingress.c   |  1 +
>  net/sched/sch_multiq.c    |  2 +-
>  net/sched/sch_prio.c      |  2 +-
>  net/sched/sch_sfb.c       |  1 +
>  net/sched/sch_sfq.c       |  1 +
>  15 files changed, 34 insertions(+), 26 deletions(-)

This broke the build for !NET_CLS:

net/built-in.o: In function `fq_codel_destroy':
/home/sab/linux/net-next/net/sched/sch_fq_codel.c:468: undefined reference to `tcf_destroy_chain'


-- 
Sabrina

Re: [patch net-next 2/6] sched: move tcf_proto_destroy and tcf_destroy_chain helpers into cls_api

[Jiri Pirko]

Tue, Feb 14, 2017 at 06:25:01PM CET, sd@queasysnail.net wrote:
>Hi Jiri,
>
>2017-02-09, 14:38:56 +0100, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> Creation is done in this file, move destruction to be at the same place.
>> 
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>  include/net/pkt_cls.h     |  2 ++
>>  include/net/sch_generic.h |  2 --
>>  net/sched/cls_api.c       | 21 +++++++++++++++++++++
>>  net/sched/sch_api.c       | 22 ----------------------
>>  net/sched/sch_atm.c       |  1 +
>>  net/sched/sch_cbq.c       |  1 +
>>  net/sched/sch_choke.c     |  1 +
>>  net/sched/sch_dsmark.c    |  1 +
>>  net/sched/sch_fq_codel.c  |  1 +
>>  net/sched/sch_htb.c       |  1 +
>>  net/sched/sch_ingress.c   |  1 +
>>  net/sched/sch_multiq.c    |  2 +-
>>  net/sched/sch_prio.c      |  2 +-
>>  net/sched/sch_sfb.c       |  1 +
>>  net/sched/sch_sfq.c       |  1 +
>>  15 files changed, 34 insertions(+), 26 deletions(-)
>
>This broke the build for !NET_CLS:
>
>net/built-in.o: In function `fq_codel_destroy':
>/home/sab/linux/net-next/net/sched/sch_fq_codel.c:468: undefined reference to `tcf_destroy_chain'

Will fix. Thanks.

[patch net-next 3/6] sched: push TC filter protocol creation into a separate function

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Make the long function tc_ctl_tfilter a little bit shorter and easier to
read. Also make the creation of filter proto symmetric to destruction.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/sched/cls_api.c | 110 ++++++++++++++++++++++++++++------------------------
 1 file changed, 59 insertions(+), 51 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 4efa4df..d378a0b 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -19,6 +19,7 @@
 #include <linux/kernel.h>
 #include <linux/string.h>
 #include <linux/errno.h>
+#include <linux/err.h>
 #include <linux/skbuff.h>
 #include <linux/init.h>
 #include <linux/kmod.h>
@@ -38,14 +39,14 @@ static DEFINE_RWLOCK(cls_mod_lock);
 
 /* Find classifier type by string name */
 
-static const struct tcf_proto_ops *tcf_proto_lookup_ops(struct nlattr *kind)
+static const struct tcf_proto_ops *tcf_proto_lookup_ops(const char *kind)
 {
 	const struct tcf_proto_ops *t, *res = NULL;
 
 	if (kind) {
 		read_lock(&cls_mod_lock);
 		list_for_each_entry(t, &tcf_proto_base, head) {
-			if (nla_strcmp(kind, t->kind) == 0) {
+			if (strcmp(kind, t->kind) == 0) {
 				if (try_module_get(t->owner))
 					res = t;
 				break;
@@ -127,6 +128,56 @@ static inline u32 tcf_auto_prio(struct tcf_proto *tp)
 	return first;
 }
 
+static struct tcf_proto *tcf_proto_create(const char *kind, u32 protocol,
+					  u32 prio, u32 parent, struct Qdisc *q)
+{
+	struct tcf_proto *tp;
+	int err;
+
+	tp = kzalloc(sizeof(*tp), GFP_KERNEL);
+	if (!tp)
+		return ERR_PTR(-ENOBUFS);
+
+	err = -ENOENT;
+	tp->ops = tcf_proto_lookup_ops(kind);
+	if (!tp->ops) {
+#ifdef CONFIG_MODULES
+		rtnl_unlock();
+		request_module("cls_%s", kind);
+		rtnl_lock();
+		tp->ops = tcf_proto_lookup_ops(kind);
+		/* We dropped the RTNL semaphore in order to perform
+		 * the module load. So, even if we succeeded in loading
+		 * the module we have to replay the request. We indicate
+		 * this using -EAGAIN.
+		 */
+		if (tp->ops) {
+			module_put(tp->ops->owner);
+			err = -EAGAIN;
+		} else {
+			err = -ENOENT;
+		}
+		goto errout;
+#endif
+	}
+	tp->classify = tp->ops->classify;
+	tp->protocol = protocol;
+	tp->prio = prio;
+	tp->classid = parent;
+	tp->q = q;
+
+	err = tp->ops->init(tp);
+	if (err) {
+		module_put(tp->ops->owner);
+		goto errout;
+	}
+	return tp;
+
+errout:
+	kfree(tp);
+	return ERR_PTR(err);
+}
+
 static bool tcf_proto_destroy(struct tcf_proto *tp, bool force)
 {
 	if (tp->ops->destroy(tp, force)) {
@@ -164,7 +215,6 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
 	struct tcf_proto __rcu **back;
 	struct tcf_proto __rcu **chain;
 	struct tcf_proto *tp;
-	const struct tcf_proto_ops *tp_ops;
 	const struct Qdisc_class_ops *cops;
 	unsigned long cl;
 	unsigned long fh;
@@ -279,58 +329,16 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
 		    !(n->nlmsg_flags & NLM_F_CREATE))
 			goto errout;
 
+		if (!nprio)
+			nprio = TC_H_MAJ(tcf_auto_prio(rtnl_dereference(*back)));
 
-		/* Create new proto tcf */
-
-		err = -ENOBUFS;
-		tp = kzalloc(sizeof(*tp), GFP_KERNEL);
-		if (tp == NULL)
-			goto errout;
-		err = -ENOENT;
-		tp_ops = tcf_proto_lookup_ops(tca[TCA_KIND]);
-		if (tp_ops == NULL) {
-#ifdef CONFIG_MODULES
-			struct nlattr *kind = tca[TCA_KIND];
-			char name[IFNAMSIZ];
-
-			if (kind != NULL &&
-			    nla_strlcpy(name, kind, IFNAMSIZ) < IFNAMSIZ) {
-				rtnl_unlock();
-				request_module("cls_%s", name);
-				rtnl_lock();
-				tp_ops = tcf_proto_lookup_ops(kind);
-				/* We dropped the RTNL semaphore in order to
-				 * perform the module load.  So, even if we
-				 * succeeded in loading the module we have to
-				 * replay the request.  We indicate this using
-				 * -EAGAIN.
-				 */
-				if (tp_ops != NULL) {
-					module_put(tp_ops->owner);
-					err = -EAGAIN;
-				}
-			}
-#endif
-			kfree(tp);
+		tp = tcf_proto_create(nla_data(tca[TCA_KIND]),
+				      protocol, nprio, parent, q);
+		if (IS_ERR(tp)) {
+			err = PTR_ERR(tp);
 			goto errout;
 		}
-		tp->ops = tp_ops;
-		tp->protocol = protocol;
-		tp->prio = nprio ? :
-			       TC_H_MAJ(tcf_auto_prio(rtnl_dereference(*back)));
-		tp->q = q;
-		tp->classify = tp_ops->classify;
-		tp->classid = parent;
-
-		err = tp_ops->init(tp);
-		if (err != 0) {
-			module_put(tp_ops->owner);
-			kfree(tp);
-			goto errout;
-		}
-
 		tp_created = 1;
-
 	} else if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], tp->ops->kind))
 		goto errout;
 
-- 
2.7.4

Re: [patch net-next 3/6] sched: push TC filter protocol creation into a separate function

[Jamal Hadi Salim]

On 17-02-09 08:38 AM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Make the long function tc_ctl_tfilter a little bit shorter and easier to
> read. Also make the creation of filter proto symmetric to destruction.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

[patch net-next 4/6] sched: move err set right before goto errout in tc_ctl_tfilter

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

This makes the reader to know right away what is the error value.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/sched/cls_api.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index d378a0b..f44378c 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -293,9 +293,10 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
 
 	/* And the last stroke */
 	chain = cops->tcf_chain(q, cl);
-	err = -EINVAL;
-	if (chain == NULL)
+	if (chain == NULL) {
+		err = -EINVAL;
 		goto errout;
+	}
 	if (n->nlmsg_type == RTM_DELTFILTER && prio == 0) {
 		tfilter_notify_chain(net, skb, n, chain, RTM_DELTFILTER);
 		tcf_destroy_chain(chain);
@@ -310,8 +311,10 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
 		if (tp->prio >= prio) {
 			if (tp->prio == prio) {
 				if (!nprio ||
-				    (tp->protocol != protocol && protocol))
+				    (tp->protocol != protocol && protocol)) {
+					err = -EINVAL;
 					goto errout;
+				}
 			} else
 				tp = NULL;
 			break;
@@ -321,13 +324,16 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
 	if (tp == NULL) {
 		/* Proto-tcf does not exist, create new one */
 
-		if (tca[TCA_KIND] == NULL || !protocol)
+		if (tca[TCA_KIND] == NULL || !protocol) {
+			err = -EINVAL;
 			goto errout;
+		}
 
-		err = -ENOENT;
 		if (n->nlmsg_type != RTM_NEWTFILTER ||
-		    !(n->nlmsg_flags & NLM_F_CREATE))
+		    !(n->nlmsg_flags & NLM_F_CREATE)) {
+			err = -ENOENT;
 			goto errout;
+		}
 
 		if (!nprio)
 			nprio = TC_H_MAJ(tcf_auto_prio(rtnl_dereference(*back)));
@@ -339,8 +345,10 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
 			goto errout;
 		}
 		tp_created = 1;
-	} else if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], tp->ops->kind))
+	} else if (tca[TCA_KIND] && nla_strcmp(tca[TCA_KIND], tp->ops->kind)) {
+		err = -EINVAL;
 		goto errout;
+	}
 
 	fh = tp->ops->get(tp, t->tcm_handle);
 
@@ -357,17 +365,18 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
 			goto errout;
 		}
 
-		err = -ENOENT;
 		if (n->nlmsg_type != RTM_NEWTFILTER ||
-		    !(n->nlmsg_flags & NLM_F_CREATE))
+		    !(n->nlmsg_flags & NLM_F_CREATE)) {
+			err = -ENOENT;
 			goto errout;
+		}
 	} else {
 		switch (n->nlmsg_type) {
 		case RTM_NEWTFILTER:
-			err = -EEXIST;
 			if (n->nlmsg_flags & NLM_F_EXCL) {
 				if (tp_created)
 					tcf_proto_destroy(tp, true);
+				err = -EEXIST;
 				goto errout;
 			}
 			break;
-- 
2.7.4

Re: [patch net-next 4/6] sched: move err set right before goto errout in tc_ctl_tfilter

[Jamal Hadi Salim]

On 17-02-09 08:38 AM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> This makes the reader to know right away what is the error value.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>


I think you meant "makes it easier to read"? Thats how i see
this change.

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

[patch net-next 5/6] sched: add missing curly braces in else branch in tc_ctl_tfilter

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Curly braces need to be there, for stylistic reasons.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/sched/cls_api.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index f44378c..48864ad 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -315,8 +315,9 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
 					err = -EINVAL;
 					goto errout;
 				}
-			} else
+			} else {
 				tp = NULL;
+			}
 			break;
 		}
 	}
-- 
2.7.4

Re: [patch net-next 5/6] sched: add missing curly braces in else branch in tc_ctl_tfilter

[Jamal Hadi Salim]

On 17-02-09 08:38 AM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Curly braces need to be there, for stylistic reasons.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  net/sched/cls_api.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index f44378c..48864ad 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -315,8 +315,9 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
>  					err = -EINVAL;
>  					goto errout;
>  				}
> -			} else
> +			} else {
>  				tp = NULL;
> +			}
>  			break;
>  		}
>  	}
>

Jiri, shall we engage in a long discussion about which rule says that
you can put braces around one line branching? ;->

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

Re: [patch net-next 5/6] sched: add missing curly braces in else branch in tc_ctl_tfilter

[Jiri Pirko]

Thu, Feb 09, 2017 at 07:27:10PM CET, jhs@mojatatu.com wrote:
>On 17-02-09 08:38 AM, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> Curly braces need to be there, for stylistic reasons.
>> 
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>  net/sched/cls_api.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index f44378c..48864ad 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -315,8 +315,9 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
>>  					err = -EINVAL;
>>  					goto errout;
>>  				}
>> -			} else
>> +			} else {
>>  				tp = NULL;
>> +			}
>>  			break;
>>  		}
>>  	}
>> 
>
>Jiri, shall we engage in a long discussion about which rule says that
>you can put braces around one line branching? ;->

scripts/checkpatch.pl :)


>
>Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
>
>cheers,
>jamal

[patch net-next 6/6] sched: check negative err value to safe one level of indent

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

As it is more common, check err for !0. That allows to safe one level of
indentation and makes the code easier to read. Also, make 'next' variable
global in function as it is used twice.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/sched/cls_api.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 48864ad..abe1fe1 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -214,6 +214,7 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
 	struct Qdisc  *q;
 	struct tcf_proto __rcu **back;
 	struct tcf_proto __rcu **chain;
+	struct tcf_proto *next;
 	struct tcf_proto *tp;
 	const struct Qdisc_class_ops *cops;
 	unsigned long cl;
@@ -355,10 +356,8 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
 
 	if (fh == 0) {
 		if (n->nlmsg_type == RTM_DELTFILTER && t->tcm_handle == 0) {
-			struct tcf_proto *next = rtnl_dereference(tp->next);
-
+			next = rtnl_dereference(tp->next);
 			RCU_INIT_POINTER(*back, next);
-
 			tfilter_notify(net, skb, n, tp, fh,
 				       RTM_DELTFILTER, false);
 			tcf_proto_destroy(tp, true);
@@ -383,16 +382,13 @@ static int tc_ctl_tfilter(struct sk_buff *skb, struct nlmsghdr *n)
 			break;
 		case RTM_DELTFILTER:
 			err = tp->ops->delete(tp, fh);
-			if (err == 0) {
-				struct tcf_proto *next = rtnl_dereference(tp->next);
-
-				tfilter_notify(net, skb, n, tp,
-					       t->tcm_handle,
-					       RTM_DELTFILTER, false);
-				if (tcf_proto_destroy(tp, false))
-					RCU_INIT_POINTER(*back, next);
-			}
-			goto errout;
+			if (err)
+				goto errout;
+			next = rtnl_dereference(tp->next);
+			tfilter_notify(net, skb, n, tp, t->tcm_handle,
+				       RTM_DELTFILTER, false);
+			if (tcf_proto_destroy(tp, false))
+				RCU_INIT_POINTER(*back, next);
 		case RTM_GETTFILTER:
 			err = tfilter_notify(net, skb, n, tp, fh,
 					     RTM_NEWTFILTER, true);
-- 
2.7.4

Re: [patch net-next 6/6] sched: check negative err value to safe one level of indent

[Jamal Hadi Salim]

On 17-02-09 08:39 AM, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> As it is more common, check err for !0. That allows to safe one level of
> indentation and makes the code easier to read. Also, make 'next' variable
> global in function as it is used twice.
>
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

BTW: Another nice cleanup is to separate for better
readability/maintainability DEL/GET/NEW into
their own functions. GET/DEL are very close but sometimes are
slightly different - in the action code they are already breaking.

cheers,
jamal

Re: [patch net-next 0/6] sched: cls_api: small cleanup

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Thu,  9 Feb 2017 14:38:54 +0100

> This patchset makes couple of things in cls_api code a bit nicer and easier
> for reader to digest.

Series applied, thanks Jiri.