[PATCH 1/2] net_sched: add struct net pointer to tcf_proto_ops->dump

[PATCH 1/2] net_sched: add struct net pointer to tcf_proto_ops->dump

[Cong Wang]

It will be needed by the next patch.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/sch_generic.h |  2 +-
 net/sched/cls_api.c       | 11 ++++++-----
 net/sched/cls_basic.c     |  2 +-
 net/sched/cls_bpf.c       |  2 +-
 net/sched/cls_cgroup.c    |  2 +-
 net/sched/cls_flow.c      |  2 +-
 net/sched/cls_fw.c        |  2 +-
 net/sched/cls_route.c     |  2 +-
 net/sched/cls_rsvp.h      |  2 +-
 net/sched/cls_tcindex.c   |  2 +-
 net/sched/cls_u32.c       |  2 +-
 11 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 013d96d..d062f81 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -204,7 +204,7 @@ struct tcf_proto_ops {
 	void			(*walk)(struct tcf_proto*, struct tcf_walker *arg);
 
 	/* rtnetlink specific */
-	int			(*dump)(struct tcf_proto*, unsigned long,
+	int			(*dump)(struct net*, struct tcf_proto*, unsigned long,
 					struct sk_buff *skb, struct tcmsg*);
 
 	struct module		*owner;
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 6b085cf..d07de5b 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -342,7 +342,7 @@ errout:
 	return err;
 }
 
-static int tcf_fill_node(struct sk_buff *skb, struct tcf_proto *tp,
+static int tcf_fill_node(struct net *net, struct sk_buff *skb, struct tcf_proto *tp,
 			 unsigned long fh, u32 portid, u32 seq, u16 flags, int event)
 {
 	struct tcmsg *tcm;
@@ -364,7 +364,7 @@ static int tcf_fill_node(struct sk_buff *skb, struct tcf_proto *tp,
 	tcm->tcm_handle = fh;
 	if (RTM_DELTFILTER != event) {
 		tcm->tcm_handle = 0;
-		if (tp->ops->dump && tp->ops->dump(tp, fh, skb, tcm) < 0)
+		if (tp->ops->dump && tp->ops->dump(net, tp, fh, skb, tcm) < 0)
 			goto nla_put_failure;
 	}
 	nlh->nlmsg_len = skb_tail_pointer(skb) - b;
@@ -387,7 +387,7 @@ static int tfilter_notify(struct net *net, struct sk_buff *oskb,
 	if (!skb)
 		return -ENOBUFS;
 
-	if (tcf_fill_node(skb, tp, fh, portid, n->nlmsg_seq, 0, event) <= 0) {
+	if (tcf_fill_node(net, skb, tp, fh, portid, n->nlmsg_seq, 0, event) <= 0) {
 		kfree_skb(skb);
 		return -EINVAL;
 	}
@@ -406,8 +406,9 @@ static int tcf_node_dump(struct tcf_proto *tp, unsigned long n,
 			 struct tcf_walker *arg)
 {
 	struct tcf_dump_args *a = (void *)arg;
+	struct net *net = sock_net(a->skb->sk);
 
-	return tcf_fill_node(a->skb, tp, n, NETLINK_CB(a->cb->skb).portid,
+	return tcf_fill_node(net, a->skb, tp, n, NETLINK_CB(a->cb->skb).portid,
 			     a->cb->nlh->nlmsg_seq, NLM_F_MULTI, RTM_NEWTFILTER);
 }
 
@@ -465,7 +466,7 @@ static int tc_dump_tfilter(struct sk_buff *skb, struct netlink_callback *cb)
 		if (t > s_t)
 			memset(&cb->args[1], 0, sizeof(cb->args)-sizeof(cb->args[0]));
 		if (cb->args[1] == 0) {
-			if (tcf_fill_node(skb, tp, 0, NETLINK_CB(cb->skb).portid,
+			if (tcf_fill_node(net, skb, tp, 0, NETLINK_CB(cb->skb).portid,
 					  cb->nlh->nlmsg_seq, NLM_F_MULTI,
 					  RTM_NEWTFILTER) <= 0)
 				break;
diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index b655203..af6bea1 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -241,7 +241,7 @@ skip:
 	}
 }
 
-static int basic_dump(struct tcf_proto *tp, unsigned long fh,
+static int basic_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 		      struct sk_buff *skb, struct tcmsg *t)
 {
 	struct basic_filter *f = (struct basic_filter *) fh;
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 00a5a58..8e3cf49 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -295,7 +295,7 @@ errout:
 	return ret;
 }
 
-static int cls_bpf_dump(struct tcf_proto *tp, unsigned long fh,
+static int cls_bpf_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 			struct sk_buff *skb, struct tcmsg *tm)
 {
 	struct cls_bpf_prog *prog = (struct cls_bpf_prog *) fh;
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index f9d21258..e06faf0 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -260,7 +260,7 @@ skip:
 	arg->count++;
 }
 
-static int cls_cgroup_dump(struct tcf_proto *tp, unsigned long fh,
+static int cls_cgroup_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 			   struct sk_buff *skb, struct tcmsg *t)
 {
 	struct cls_cgroup_head *head = tp->root;
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index dfd18a5..257029c 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -563,7 +563,7 @@ static void flow_put(struct tcf_proto *tp, unsigned long f)
 {
 }
 
-static int flow_dump(struct tcf_proto *tp, unsigned long fh,
+static int flow_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 		     struct sk_buff *skb, struct tcmsg *t)
 {
 	struct flow_filter *f = (struct flow_filter *)fh;
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index 3f9cece..d605285 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -324,7 +324,7 @@ static void fw_walk(struct tcf_proto *tp, struct tcf_walker *arg)
 	}
 }
 
-static int fw_dump(struct tcf_proto *tp, unsigned long fh,
+static int fw_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 		   struct sk_buff *skb, struct tcmsg *t)
 {
 	struct fw_head *head = (struct fw_head *)tp->root;
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index 2473953..c913613 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -551,7 +551,7 @@ static void route4_walk(struct tcf_proto *tp, struct tcf_walker *arg)
 	}
 }
 
-static int route4_dump(struct tcf_proto *tp, unsigned long fh,
+static int route4_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 		       struct sk_buff *skb, struct tcmsg *t)
 {
 	struct route4_filter *f = (struct route4_filter *)fh;
diff --git a/net/sched/cls_rsvp.h b/net/sched/cls_rsvp.h
index 4f25c2a..19f8e5d 100644
--- a/net/sched/cls_rsvp.h
+++ b/net/sched/cls_rsvp.h
@@ -594,7 +594,7 @@ static void rsvp_walk(struct tcf_proto *tp, struct tcf_walker *arg)
 	}
 }
 
-static int rsvp_dump(struct tcf_proto *tp, unsigned long fh,
+static int rsvp_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 		     struct sk_buff *skb, struct tcmsg *t)
 {
 	struct rsvp_filter *f = (struct rsvp_filter *)fh;
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
index ffad187..f575353 100644
--- a/net/sched/cls_tcindex.c
+++ b/net/sched/cls_tcindex.c
@@ -422,7 +422,7 @@ static void tcindex_destroy(struct tcf_proto *tp)
 }
 
 
-static int tcindex_dump(struct tcf_proto *tp, unsigned long fh,
+static int tcindex_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
     struct sk_buff *skb, struct tcmsg *t)
 {
 	struct tcindex_data *p = PRIV(tp);
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 20f2fb7..e25411a 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -712,7 +712,7 @@ static void u32_walk(struct tcf_proto *tp, struct tcf_walker *arg)
 	}
 }
 
-static int u32_dump(struct tcf_proto *tp, unsigned long fh,
+static int u32_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 		     struct sk_buff *skb, struct tcmsg *t)
 {
 	struct tc_u_knode *n = (struct tc_u_knode *)fh;
-- 
1.8.3.1

[PATCH 2/2] net_sched: optimize tcf_match_indev()

[Cong Wang]

tcf_match_indev() is called in fast path, it is not wise to
search for a netdev by ifindex and then compare by its name,
just compare the ifindex.

This will also save some bytes from the core struct of u32.

Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: David S. Miller <davem@davemloft.net>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/pkt_cls.h | 30 +++++++++++++++---------------
 net/sched/cls_fw.c    | 19 ++++++++++++-------
 net/sched/cls_u32.c   | 19 ++++++++++++-------
 3 files changed, 39 insertions(+), 29 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 50ea079..a2441fb 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -338,27 +338,27 @@ static inline int tcf_valid_offset(const struct sk_buff *skb,
 #include <net/net_namespace.h>
 
 static inline int
-tcf_change_indev(struct tcf_proto *tp, char *indev, struct nlattr *indev_tlv)
+tcf_change_indev(struct net *net, struct nlattr *indev_tlv)
 {
+	char indev[IFNAMSIZ];
+	struct net_device *dev;
+
 	if (nla_strlcpy(indev, indev_tlv, IFNAMSIZ) >= IFNAMSIZ)
 		return -EINVAL;
-	return 0;
+	dev = __dev_get_by_name(net, indev);
+	if (!dev)
+		return -ENODEV;
+	return dev->ifindex;
 }
 
-static inline int
-tcf_match_indev(struct sk_buff *skb, char *indev)
+static inline bool
+tcf_match_indev(struct sk_buff *skb, int ifindex)
 {
-	struct net_device *dev;
-
-	if (indev[0]) {
-		if  (!skb->skb_iif)
-			return 0;
-		dev = __dev_get_by_index(dev_net(skb->dev), skb->skb_iif);
-		if (!dev || strcmp(indev, dev->name))
-			return 0;
-	}
-
-	return 1;
+	if (!ifindex)
+		return true;
+	if  (!skb->skb_iif)
+		return false;
+	return ifindex == skb->skb_iif;
 }
 #endif /* CONFIG_NET_CLS_IND */
 
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index d605285..ca662aa 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -41,7 +41,7 @@ struct fw_filter {
 	u32			id;
 	struct tcf_result	res;
 #ifdef CONFIG_NET_CLS_IND
-	char			indev[IFNAMSIZ];
+	int			ifindex;
 #endif /* CONFIG_NET_CLS_IND */
 	struct tcf_exts		exts;
 };
@@ -86,7 +86,7 @@ static int fw_classify(struct sk_buff *skb, const struct tcf_proto *tp,
 			if (f->id == id) {
 				*res = f->res;
 #ifdef CONFIG_NET_CLS_IND
-				if (!tcf_match_indev(skb, f->indev))
+				if (!tcf_match_indev(skb, f->ifindex))
 					continue;
 #endif /* CONFIG_NET_CLS_IND */
 				r = tcf_exts_exec(skb, &f->exts, res);
@@ -207,9 +207,11 @@ fw_change_attrs(struct net *net, struct tcf_proto *tp, struct fw_filter *f,
 
 #ifdef CONFIG_NET_CLS_IND
 	if (tb[TCA_FW_INDEV]) {
-		err = tcf_change_indev(tp, f->indev, tb[TCA_FW_INDEV]);
-		if (err < 0)
+		int ret;
+		ret = tcf_change_indev(net, tb[TCA_FW_INDEV]);
+		if (ret < 0)
 			goto errout;
+		f->ifindex = ret;
 	}
 #endif /* CONFIG_NET_CLS_IND */
 
@@ -348,9 +350,12 @@ static int fw_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 	    nla_put_u32(skb, TCA_FW_CLASSID, f->res.classid))
 		goto nla_put_failure;
 #ifdef CONFIG_NET_CLS_IND
-	if (strlen(f->indev) &&
-	    nla_put_string(skb, TCA_FW_INDEV, f->indev))
-		goto nla_put_failure;
+	if (f->ifindex) {
+		struct net_device *dev;
+		dev = __dev_get_by_index(net, f->ifindex);
+		if (dev && nla_put_string(skb, TCA_FW_INDEV, dev->name))
+			goto nla_put_failure;
+	}
 #endif /* CONFIG_NET_CLS_IND */
 	if (head->mask != 0xFFFFFFFF &&
 	    nla_put_u32(skb, TCA_FW_MASK, head->mask))
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index e25411a..f509b79 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -48,7 +48,7 @@ struct tc_u_knode {
 	struct tc_u_hnode	*ht_up;
 	struct tcf_exts		exts;
 #ifdef CONFIG_NET_CLS_IND
-	char                     indev[IFNAMSIZ];
+	int			ifindex;
 #endif
 	u8			fshift;
 	struct tcf_result	res;
@@ -152,7 +152,7 @@ check_terminal:
 
 				*res = n->res;
 #ifdef CONFIG_NET_CLS_IND
-				if (!tcf_match_indev(skb, n->indev)) {
+				if (!tcf_match_indev(skb, n->ifindex)) {
 					n = n->next;
 					goto next_knode;
 				}
@@ -527,9 +527,11 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
 
 #ifdef CONFIG_NET_CLS_IND
 	if (tb[TCA_U32_INDEV]) {
-		err = tcf_change_indev(tp, n->indev, tb[TCA_U32_INDEV]);
-		if (err < 0)
+		int ret;
+		ret = tcf_change_indev(net, tb[TCA_U32_INDEV]);
+		if (ret < 0)
 			goto errout;
+		n->ifindex = ret;
 	}
 #endif
 	tcf_exts_change(tp, &n->exts, &e);
@@ -760,9 +762,12 @@ static int u32_dump(struct net *net, struct tcf_proto *tp, unsigned long fh,
 			goto nla_put_failure;
 
 #ifdef CONFIG_NET_CLS_IND
-		if (strlen(n->indev) &&
-		    nla_put_string(skb, TCA_U32_INDEV, n->indev))
-			goto nla_put_failure;
+		if (n->ifindex) {
+			struct net_device *dev;
+			dev = __dev_get_by_index(net, n->ifindex);
+			if (dev && nla_put_string(skb, TCA_U32_INDEV, dev->name))
+				goto nla_put_failure;
+		}
 #endif
 #ifdef CONFIG_CLS_U32_PERF
 		if (nla_put(skb, TCA_U32_PCNT,
-- 
1.8.3.1

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Eric Dumazet]

On Thu, 2013-12-19 at 17:34 -0800, Cong Wang wrote:
> tcf_match_indev() is called in fast path, it is not wise to
> search for a netdev by ifindex and then compare by its name,
> just compare the ifindex.
> 
> This will also save some bytes from the core struct of u32.
> 

This seems very suspect to me. How was it tested ?

BTW, latest net-next doesn't work anymore.

[   76.591023] BUG: unable to handle kernel paging request at ffffffffffffffff^M
[   76.598025] IP: [<ffffffffa008d882>] mirred_cleanup_module+0x13a/0x2a [act_mirred]^M
[   76.605624] PGD 180c067 PUD 180e067 PMD 0 ^M
[   76.609769] Oops: 0000 [#1] SMP ^M
[   76.613038] Modules linked in: act_mirred cls_tcindex sch_dsmark xt_multiport iptable_mangle w1_therm wire cdc_acm uhci_hcd ehci_hcd i2c_dev i2c_i801 i2c_core msr cpuid ib_uverbs mlx4_ib ib_sa ib_mad ib_core mlx4_en mlx4_core ipv6^M
[   76.633899] CPU: 27 PID: 10394 Comm: BweTCTree_DoWor Not tainted 3.13.0-smp-DEV #411^M
[   76.648567] task: ffff881030085670 ti: ffff881014088000 task.ti: ffff881014088000^M
[   76.656061] RIP: 0010:[<ffffffffa008d882>]  [<ffffffffa008d882>] mirred_cleanup_module+0x13a/0x2a [act_mirred]^M
[   76.666131] RSP: 0018:ffff8810140899b0  EFLAGS: 00010206^M
[   76.671455] RAX: 0000000000010000 RBX: ffff88101bfff240 RCX: 0000000000000000^M
[   76.678593] RDX: ffffffff818a4b50 RSI: ffffffffa0059790 RDI: ffff88101bfff240^M
[   76.685793] RBP: ffff881014089aa8 R08: ffff88107f4004c0 R09: ffff88101bfff240^M
[   76.692959] R10: ffff88102eb6eb40 R11: 00000000ffffff97 R12: ffff88102f2aa160^M
[   76.700133] R13: ffff88102f2aa000 R14: ffff88102f2aa000 R15: ffff881028802c00^M
[   76.707272] FS:  00007fddb20a0700(0000) GS:ffff88107fbe0000(0000) knlGS:0000000000000000^M
[   76.715429] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033^M
[   76.721209] CR2: ffffffffffffffff CR3: 000000007919e000 CR4: 00000000001407e0^M
[   76.728364] Stack:^M
[   76.730378]  ffffffff814dd0e1 0000000000000028 ffffffff818a1940 ffffffff818a4b50^M
[   76.737845]  ffff88102eb6eb40 ffff881028802c24 0000000800000000 ffffffff818a1940^M
[   76.745367]  0000000000000000 ffffffff8169ca80 0001000000010000 ffff881014089a68^M
[   76.752860] Call Trace:^M
[   76.755320]  [<ffffffff814dd0e1>] ? tc_ctl_tfilter+0x4a1/0x760^M
[   76.761153]  [<ffffffff814e6866>] ? __netlink_sendskb+0x56/0x130^M
[   76.767165]  [<ffffffff814caa04>] rtnetlink_rcv_msg+0xa4/0x240^M
[   76.773026]  [<ffffffff814ca960>] ? __rtnl_unlock+0x20/0x20^M
[   76.778604]  [<ffffffff814e9741>] netlink_rcv_skb+0xb1/0xc0^M
[   76.784180]  [<ffffffff814c7635>] rtnetlink_rcv+0x25/0x40^M
[   76.789606]  [<ffffffff814e8fed>] netlink_unicast+0x14d/0x1f0^M
[   76.795359]  [<ffffffff814e9393>] netlink_sendmsg+0x303/0x410^M
[   76.801109]  [<ffffffff814a048c>] sock_sendmsg+0x9c/0xd0^M
[   76.806444]  [<ffffffff814a0da0>] ? move_addr_to_kernel+0x40/0xa0^M
[   76.812552]  [<ffffffff814add21>] ? verify_iovec+0x51/0xd0^M
[   76.818065]  [<ffffffff814a0c21>] ___sys_sendmsg+0x3c1/0x3d0^M
[   76.823740]  [<ffffffff8156897c>] ? __do_page_fault+0x26c/0x500^M
[   76.829706]  [<ffffffff811d23f6>] ? mntput+0x26/0x40^M
[   76.834718]  [<ffffffff811b4ce1>] ? __fput+0x161/0x230^M
[   76.839879]  [<ffffffff814a1809>] __sys_sendmsg+0x49/0x90^M
[   76.845299]  [<ffffffff814a1862>] SyS_sendmsg+0x12/0x20^M
[   76.850564]  [<ffffffff8156d252>] system_call_fastpath+0x16/0x1b^M

I wonder how you tested "net_sched: init struct tcf_hashinfo at register time",
it is completely buggy.

static inline int tcf_hashinfo_init(struct tcf_hashinfo *hf, unsigned int mask)

mask is a mask, yet all callers pass 'MASK+1'

include/net/act_api.h:49:static inline int tcf_hashinfo_init(struct tcf_hashinfo *hf, unsigned int mask)
net/sched/act_csum.c:589:       int err = tcf_hashinfo_init(&csum_hash_info, CSUM_TAB_MASK+1);
net/sched/act_gact.c:211:       int err = tcf_hashinfo_init(&gact_hash_info, GACT_TAB_MASK+1);
net/sched/act_ipt.c:317:        err = tcf_hashinfo_init(&ipt_hash_info, IPT_TAB_MASK+1);
net/sched/act_mirred.c:279:     err = tcf_hashinfo_init(&mirred_hash_info, MIRRED_TAB_MASK+1);
net/sched/act_nat.c:313:        int err = tcf_hashinfo_init(&nat_hash_info, NAT_TAB_MASK+1);
net/sched/act_pedit.c:249:      int err = tcf_hashinfo_init(&pedit_hash_info, PEDIT_TAB_MASK+1);
net/sched/act_police.c:399:     int err = tcf_hashinfo_init(&police_hash_info, POL_TAB_MASK+1);
net/sched/act_simple.c:206:     err = tcf_hashinfo_init(&simp_hash_info, SIMP_TAB_MASK+1);
net/sched/act_skbedit.c:206:    int err = tcf_hashinfo_init(&skbedit_hash_info, SKBEDIT_TAB_MASK+1);

I'll send a fix

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Cong Wang]

On Thu, Dec 19, 2013 at 10:22 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2013-12-19 at 17:34 -0800, Cong Wang wrote:
>> tcf_match_indev() is called in fast path, it is not wise to
>> search for a netdev by ifindex and then compare by its name,
>> just compare the ifindex.
>>
>> This will also save some bytes from the core struct of u32.
>>
>
> This seems very suspect to me. How was it tested ?

Why? I tested it with 1000 u32 filters with mirred actions
to a dummy interface, it is the test case I used from the beginning.

>
> BTW, latest net-next doesn't work anymore.
>
> [   76.591023] BUG: unable to handle kernel paging request at ffffffffffffffff^M
> [   76.598025] IP: [<ffffffffa008d882>] mirred_cleanup_module+0x13a/0x2a [act_mirred]^M


All of my kernel modules are built-in, this is why I can't catch this bug.
So why not share your test case with me?

> I wonder how you tested "net_sched: init struct tcf_hashinfo at register time",
> it is completely buggy.
>

Since I compile everything as built-in, so just boot and shutdown the VM,
and of course, run some test cases like above one.

> static inline int tcf_hashinfo_init(struct tcf_hashinfo *hf, unsigned int mask)
>
> mask is a mask, yet all callers pass 'MASK+1'
>

Hmm, off-by-one error, stupid me.

Again, it is always welcome if you can share your test case.

Thanks.

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Eric Dumazet]

On Thu, 2013-12-19 at 22:36 -0800, Cong Wang wrote:

> Hmm, off-by-one error, stupid me.

There is something else.

Its late here, I'll let you find and fix the problems.

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Cong Wang]

On Thu, Dec 19, 2013 at 10:54 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2013-12-19 at 22:36 -0800, Cong Wang wrote:
>
>> Hmm, off-by-one error, stupid me.
>
> There is something else.
>
> Its late here, I'll let you find and fix the problems.
>

I just sent a fix for the problem I see, not sure it fixes
the bug you reported too.

I will test it as a module tomorrow, if you don't send a fix
before that.

Thanks.

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Jamal Hadi Salim]

On 12/20/13 03:12, Cong Wang wrote:
> On Thu, Dec 19, 2013 at 10:54 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Thu, 2013-12-19 at 22:36 -0800, Cong Wang wrote:
>>
>>> Hmm, off-by-one error, stupid me.
>>
>> There is something else.
>>
>> Its late here, I'll let you find and fix the problems.
>>
>
> I just sent a fix for the problem I see, not sure it fixes
> the bug you reported too.
>
> I will test it as a module tomorrow, if you don't send a fix
> before that.
>

Very impressive catch Eric. I would have 100% caught this with testing
(but you caught it by inspecting code!).
A simple test that dumps all the actions of a specific type would do.
Example in your case, Cong:
tc actions ls action mirred

[I dont want to sound passive aggressive (a Canadian right, of course),
but this is why i get nervous about big changes like these which dont 
undergo more testing time.]

cheers,
jamal

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Cong Wang]

On Fri, Dec 20, 2013 at 4:14 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> Very impressive catch Eric. I would have 100% caught this with testing
> (but you caught it by inspecting code!).
> A simple test that dumps all the actions of a specific type would do.
> Example in your case, Cong:
> tc actions ls action mirred
>

Are you sure?? It is in my test case right after David merged it,
I still don't see any bug it reports so far.

For completeness, here is my one of test cases:

ip li add dev dummy0 type dummy
ip li add dev dummy1 type dummy
ip li add dev dummy2 type dummy
tc qdisc del dev $1 ingress
tc qdisc add dev $1 ingress
tc filter add dev $1 parent ffff: protocol ip u32 match ip protocol 1
0xff action mirred egress mirror dev dummy0 action mirred egress
mirror dev dummy1
tc filter add dev $1 parent ffff: protocol ip u32 match ip protocol 1
0xff action mirred egress mirror dev dummy0 action mirred egress
mirror dev dummy2
tc filter show dev $1 parent ffff: && tc action ls action mirred
ip li del dummy0
tc filter show dev $1 parent ffff: && tc action ls action mirred
ip li del dummy1
tc filter show dev $1 parent ffff: && tc action ls action mirred

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Jamal Hadi Salim]

On 12/20/13 13:55, Cong Wang wrote:
> On Fri, Dec 20, 2013 at 4:14 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>
>> Very impressive catch Eric. I would have 100% caught this with testing
>> (but you caught it by inspecting code!).
>> A simple test that dumps all the actions of a specific type would do.
>> Example in your case, Cong:
>> tc actions ls action mirred
>>
>
> Are you sure??

About 100%

>It is in my test case right after David merged it,
> I still don't see any bug it reports so far.
>

Run before your changes and after your changes and
it will be obvious.

cheers,
jamal

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Cong Wang]

On Fri, Dec 20, 2013 at 12:21 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
>> It is in my test case right after David merged it,
>> I still don't see any bug it reports so far.
>>
>
> Run before your changes and after your changes and
> it will be obvious.
>

I thought we were still talking about the crash Eric reported. :)

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Jamal Hadi Salim]

On 12/20/13 16:36, Cong Wang wrote:
> On Fri, Dec 20, 2013 at 12:21 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>>
>>> It is in my test case right after David merged it,
>>> I still don't see any bug it reports so far.
>>>
>>
>> Run before your changes and after your changes and
>> it will be obvious.
>>
>
> I thought we were still talking about the crash Eric reported. :)
>

You lost me.
You said you were running all those tests and didnt see any
bugs. I said if infact you ran exactly what you said you test
did before and after your changes you will see obvious bugs.

cheers,
jamal

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Cong Wang]

On Fri, Dec 20, 2013 at 3:00 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> You lost me.
> You said you were running all those tests and didnt see any
> bugs. I said if infact you ran exactly what you said you test
> did before and after your changes you will see obvious bugs.
>

I said I never triggered Eric's crash with `tc action ls ...`, nor
with any of my other test case.

And I also said I added 'tc action ls...' _after_ David merged it,
so failed to notice the difference.

Clear?

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Jamal Hadi Salim]

On 12/20/13 18:07, Cong Wang wrote:

>
> I said I never triggered Eric's crash with `tc action ls ...`, nor
> with any of my other test case.
>
> And I also said I added 'tc action ls...' _after_ David merged it,
> so failed to notice the difference.
>

You didnt say that last part. That was the confusing part.

> Clear?
>

Yes, making a lot more sense ;->

cheers,
jamal

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Cong Wang]

On Thu, Dec 19, 2013 at 5:34 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> tcf_match_indev() is called in fast path, it is not wise to
> search for a netdev by ifindex and then compare by its name,
> just compare the ifindex.
>
> This will also save some bytes from the core struct of u32.
>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: David S. Miller <davem@davemloft.net>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>


Jamal, any comments on this patch itself?

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Eric Dumazet]

On Fri, 2013-12-20 at 15:49 -0800, Cong Wang wrote:
> On Thu, Dec 19, 2013 at 5:34 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > tcf_match_indev() is called in fast path, it is not wise to
> > search for a netdev by ifindex and then compare by its name,
> > just compare the ifindex.
> >
> > This will also save some bytes from the core struct of u32.
> >
> > Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> > Cc: David S. Miller <davem@davemloft.net>
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> 
> 
> Jamal, any comments on this patch itself?

How device renaming (dev_change_name()) is handled ?

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Cong Wang]

On Fri, Dec 20, 2013 at 3:59 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Fri, 2013-12-20 at 15:49 -0800, Cong Wang wrote:
>> On Thu, Dec 19, 2013 at 5:34 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> > tcf_match_indev() is called in fast path, it is not wise to
>> > search for a netdev by ifindex and then compare by its name,
>> > just compare the ifindex.
>> >
>> > This will also save some bytes from the core struct of u32.
>> >
>> > Cc: Jamal Hadi Salim <jhs@mojatatu.com>
>> > Cc: David S. Miller <davem@davemloft.net>
>> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
>>
>>
>> Jamal, any comments on this patch itself?
>
> How device renaming (dev_change_name()) is handled ?
>
>

Good point.

Before my patch, when the dev name gets changes, indev will not
be matched any more. After my patch, it can still be matched
and will be displayed with the new name (ifindex doesn't change).

So, I think we probably need the latter behavior, otherwise we
have to rename the indev manually too in order to match its new
name.

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Jamal Hadi Salim]

On 12/20/13 18:49, Cong Wang wrote:

> Jamal, any comments on this patch itself?
>

I think the idea is good. Sorry havent paid close
attention - starting to look at what is already
in net-next. I could pass you a test or two to run
for this change...

cheers,
jamal

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Jamal Hadi Salim]

On 12/21/13 16:09, Jamal Hadi Salim wrote:
> On 12/20/13 18:49, Cong Wang wrote:
>
>> Jamal, any comments on this patch itself?
>>
>
> I think the idea is good. Sorry havent paid close
> attention - starting to look at what is already
> in net-next. I could pass you a test or two to run
> for this change...
>


BTW: This indev feature was supposed to be "temporary";->
but has stuck around all these years. Not sure how widely it is
used. I will send you a testcase shortly. If it passes for you
please add my ACKed-by

cheers,
jamal

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Jamal Hadi Salim]

[-- Attachment #1: Type: text/plain, Size: 349 bytes --]

On 12/22/13 11:26, Jamal Hadi Salim wrote:

> BTW: This indev feature was supposed to be "temporary";->
> but has stuck around all these years. Not sure how widely it is
> used. I will send you a testcase shortly. If it passes for you
> please add my ACKed-by


Attached.
Please run before and after your changes and compare results.

cheers,
jamal

[-- Attachment #2: indev-test1 --]
[-- Type: text/plain, Size: 1327 bytes --]

#
#
sudo ifconfig dummy0 up
#
sudo tc qdisc del dev dummy0 root handle 1: prio
sudo tc filter del dev dummy0 protocol ip pref 1 parent 1: handle 1 fw
#
sudo tc qdisc add dev dummy0 root handle 1: prio
#
sudo tc filter add dev dummy0 protocol ip pref 1 parent 1: handle 1 fw \
classid 1:1 indev eth0 \
action drop
#
sudo tc filter add dev dummy0 protocol ip pref 2 parent 1: handle 1 fw \
classid 1:2 indev lo \
action ok
#-- catch all ---
sudo tc filter add dev dummy0 protocol ip pref 3 parent 1: handle 1 fw \
classid 1:3 \
action ok
#
#
sudo tc qdisc del dev eth0 ingress
sudo tc qdisc add dev eth0 ingress
sudo tc qdisc del dev lo ingress
sudo tc qdisc add dev lo ingress
#
sudo tc filter add dev lo parent ffff: protocol ip \
u32 match ip protocol 1 0xff \
flowid 1:1 \
action skbedit mark 1 \
action mirred egress mirror dev dummy0 
#
#
sudo tc filter add dev eth0 parent ffff: protocol ip \
u32 match ip src 8.8.8.8/32 \
flowid 1:11 \
action skbedit mark 1 \
action mirred egress mirror dev dummy0 
# ping -c 1 127.0.0.1
# ping -c 1 8.8.8.8
sudo tc -s actions ls action skbedit
sudo tc -s actions ls action mirred
sudo tc -s actions ls action gact
sudo tc -s filter ls dev dummy0 parent 1: protocol ip  
sudo tc -s filter ls dev eth0 parent ffff: protocol ip  
sudo tc -s filter ls dev lo parent ffff: protocol ip  
#
#

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Jamal Hadi Salim]

[-- Attachment #1: Type: text/plain, Size: 440 bytes --]

On 12/22/13 12:04, Jamal Hadi Salim wrote:
> On 12/22/13 11:26, Jamal Hadi Salim wrote:
>
>> BTW: This indev feature was supposed to be "temporary";->
>> but has stuck around all these years. Not sure how widely it is
>> used. I will send you a testcase shortly. If it passes for you
>> please add my ACKed-by
>
>
> Attached.
> Please run before and after your changes and compare results.
>

And here's another one for u32

cheers,
jamal


[-- Attachment #2: indev-tests2 --]
[-- Type: text/plain, Size: 1474 bytes --]

#
#
sudo ifconfig dummy0 up
#
sudo tc qdisc del dev dummy0 root handle 1: prio
sudo tc qdisc add dev dummy0 root handle 1: prio
# All traffic originating from eth0 will be accounted
# by the action
sudo tc filter add dev dummy0 protocol ip pref 1 parent 1: \
u32 match u32 0 0 \
classid 1:1 indev eth0 \
action ok
#
# All traffic originating from lo will be accounted
# by the action
sudo tc filter add dev dummy0 protocol ip pref 2 parent 1:  \
u32 match u32 0 0 \
classid 1:2 indev lo \
action ok
#-- catch all if we screwed up ---
sudo tc filter add dev dummy0 protocol ip pref 3 parent 1: \
u32 match u32 0 0 \
classid 1:3 \
action ok
#
#
sudo tc qdisc del dev eth0 ingress
sudo tc qdisc add dev eth0 ingress
sudo tc qdisc del dev lo ingress
sudo tc qdisc add dev lo ingress
#
sudo tc filter add dev lo parent ffff: protocol ip \
u32 match ip protocol 1 0xff \
flowid 1:1 \
action skbedit mark 1 \
action mirred egress mirror dev dummy0 
#
#
sudo tc filter add dev eth0 parent ffff: protocol ip \
u32 match ip src 8.8.8.8/32 \
flowid 1:11 \
action skbedit mark 1 \
action mirred egress mirror dev dummy0 
# ping -c 1 127.0.0.1
# ping -c 1 8.8.8.8
# Then lets make sure all stats are correct
sudo tc -s actions ls action skbedit
sudo tc -s actions ls action mirred
sudo tc -s actions ls action gact
sudo tc -s filter ls dev dummy0 parent 1: protocol ip  
sudo tc -s filter ls dev eth0 parent ffff: protocol ip  
sudo tc -s filter ls dev lo parent ffff: protocol ip  
#
#

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Cong Wang]

On Sun, Dec 22, 2013 at 8:26 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 12/21/13 16:09, Jamal Hadi Salim wrote:
>>
>> On 12/20/13 18:49, Cong Wang wrote:
>>
>>> Jamal, any comments on this patch itself?
>>>
>>
>> I think the idea is good. Sorry havent paid close
>> attention - starting to look at what is already
>> in net-next. I could pass you a test or two to run
>> for this change...
>>
>
>
> BTW: This indev feature was supposed to be "temporary";->
> but has stuck around all these years. Not sure how widely it is

I knew, ematch does much better. :)
But once you added an API, it is hard to remove it...

> used. I will send you a testcase shortly. If it passes for you
> please add my ACKed-by
>

Will do.

Thanks.

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Jamal Hadi Salim]

For completion: I cant find any issues with whats already in net-next.
I am finding other issues but nothing related to the submission.
Thanks for you patience Cong.

cheers,
jamal

PS:- I am on travel mode, so my week may not be as responsive
other than to email.

Re: [PATCH 2/2] net_sched: optimize tcf_match_indev()

[Cong Wang]

On Mon, Dec 23, 2013 at 5:10 AM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> For completion: I cant find any issues with whats already in net-next.

Oh, you can't?! I found several:

1) ematch could replace indev
2) indev is not friendly to device renaming, so is ematch
3) device removal too

At very least my patch fixes 2) for indev. :)