[PATCHSET 0/6] PKT_SCHED: Generic classifier routines / cls_fw cleanup

17 messages, 3 authors, 2004-11-02 · open the first message on its own page

[PATCHSET 0/6] PKT_SCHED: Generic classifier routines / cls_fw cleanup

From: Thomas Graf <tgraf@suug.ch>
Date: 2004-10-29 00:21:13

Dave,

The following patchset is basically what we discussed before
but without the tcf_filter bits because it's not really worth
it. It will save us ~100 lines of code per classifier.

I tested the already splitted patchset this time :->

Jamal, I tried to test the action stuff as good as possible, but
can you give it a run with your test scripts?

I will submit a 2.4 version for patch 6 in a few minutes.

Cheers

[PATCH 1/6] PKT_SCHED: Add generic classifier routines

From: Thomas Graf <tgraf@suug.ch>
Date: 2004-10-29 00:22:29

Adds generic routines used by classifier to:
 - bind/unbind to classes
 - configure action/police/indev
 - dump action/police

Signed-off-by: Thomas Graf <tgraf@suug.ch>
--- linux-2.6.10-rc1-bk7.orig/include/net/pkt_cls.h	2004-10-28 22:03:20.000000000 +0200
+++ linux-2.6.10-rc1-bk7/include/net/pkt_cls.h	2004-10-28 23:53:05.000000000 +0200
@@ -3,6 +3,7 @@
 
 #include <linux/pkt_cls.h>
 #include <net/sch_generic.h>
+#include <net/act_api.h>
 
 /* Basic packet classifier frontend definitions. */
 
@@ -41,4 +42,190 @@
 	return old_cl;
 }
 
+static inline void
+tcf_bind_filter(struct tcf_proto *tp, struct tcf_result *r, unsigned long base)
+{
+	unsigned long cl;
+
+	cl = tp->q->ops->cl_ops->bind_tcf(tp->q, base, r->classid);
+	cl = cls_set_class(tp, &r->class, cl);
+	if (cl)
+		tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
+}
+
+static inline void
+tcf_unbind_filter(struct tcf_proto *tp, struct tcf_result *r)
+{
+	unsigned long cl;
+
+	if ((cl = __cls_set_class(&r->class, 0)) != 0)
+		tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
+}
+
+#ifdef CONFIG_NET_CLS_ACT
+static inline int
+tcf_change_act_police(struct tcf_proto *tp, struct tc_action **action,
+	struct rtattr *act_police_tlv, struct rtattr *rate_tlv)
+{
+	int ret;
+	struct tc_action *act;
+
+	act = kmalloc(sizeof(*act), GFP_KERNEL);
+	if (NULL == act)
+		return -ENOMEM;
+	memset(act, 0, sizeof(*act));
+	
+	ret = tcf_action_init_1(act_police_tlv, rate_tlv, act, "police",
+		TCA_ACT_NOREPLACE, TCA_ACT_BIND);
+	if (ret < 0) {
+		tcf_action_destroy(act, TCA_ACT_UNBIND);
+		return ret;
+	}
+
+	act->type = TCA_OLD_COMPAT;
+
+	if (*action) {
+		tcf_tree_lock(tp);
+		act = xchg(action, act);
+		tcf_tree_unlock(tp);
+
+		tcf_action_destroy(act, TCA_ACT_UNBIND);
+	} else
+		*action = act;
+
+	return 0;
+}
+
+static inline int
+tcf_change_act(struct tcf_proto *tp, struct tc_action **action,
+	struct rtattr *act_tlv, struct rtattr *rate_tlv)
+{
+	int ret;
+	struct tc_action *act;
+
+	act = kmalloc(sizeof(*act), GFP_KERNEL);
+	if (NULL == act)
+		return -ENOMEM;
+	memset(act, 0, sizeof(*act));
+
+	ret = tcf_action_init(act_tlv, rate_tlv, act, NULL,
+		TCA_ACT_NOREPLACE, TCA_ACT_BIND);
+	if (ret < 0) {
+		tcf_action_destroy(act, TCA_ACT_UNBIND);
+		return ret;
+	}
+
+	if (*action) {
+		tcf_tree_lock(tp);
+		act = xchg(action, act);
+		tcf_tree_unlock(tp);
+
+		tcf_action_destroy(act, TCA_ACT_UNBIND);
+	} else
+		*action = act;
+
+	return 0;
+}
+
+static inline int
+tcf_dump_act(struct sk_buff *skb, struct tc_action *action,
+	int act_type, int compat_type)
+{
+	/*
+	 * again for backward compatible mode - we want
+	 * to work with both old and new modes of entering
+	 * tc data even if iproute2  was newer - jhs
+	 */
+	if (action) {
+		struct rtattr * p_rta = (struct rtattr*) skb->tail;
+
+		if (action->type != TCA_OLD_COMPAT) {
+			RTA_PUT(skb, act_type, 0, NULL);
+			if (tcf_action_dump(skb, action, 0, 0) < 0)
+				goto rtattr_failure;
+		} else {
+			RTA_PUT(skb, compat_type, 0, NULL);
+			if (tcf_action_dump_old(skb, action, 0, 0) < 0)
+				goto rtattr_failure;
+		}
+		
+		p_rta->rta_len = skb->tail - (u8*)p_rta;
+	}
+	return 0;
+
+rtattr_failure:
+	return -1;
+}
+
+#ifdef CONFIG_NET_CLS_IND
+static inline int
+tcf_change_indev(struct tcf_proto *tp, char *indev, struct rtattr *indev_tlv)
+{
+	if (RTA_PAYLOAD(indev_tlv) >= IFNAMSIZ) {
+		printk("cls: bad indev name %s\n", (char *) RTA_DATA(indev_tlv));
+		return -EINVAL;
+	}
+
+	memset(indev, 0, IFNAMSIZ);
+	sprintf(indev, "%s", (char *) RTA_DATA(indev_tlv));
+
+	return 0;
+}
+
+static inline int
+tcf_match_indev(struct sk_buff *skb, char *indev)
+{
+	if (0 != indev[0]) {
+		if  (NULL == skb->input_dev)
+			return 0;
+		else if (0 != strcmp(indev, skb->input_dev->name))
+			return 0;
+	}
+
+	return 1;
+}
+#endif /* CONFIG_NET_CLS_IND */
+#endif /* CONFIG_NET_CLS_ACT */
+
+
+#ifdef CONFIG_NET_CLS_POLICE
+static inline int
+tcf_change_police(struct tcf_proto *tp, struct tcf_police **police,
+	struct rtattr *police_tlv, struct rtattr *rate_tlv)
+{
+	struct tcf_police *p = tcf_police_locate(police_tlv, rate_tlv);
+
+	if (*police) {
+		tcf_tree_lock(tp);
+		p = xchg(police, p);
+		tcf_tree_unlock(tp);
+
+		tcf_police_release(p, TCA_ACT_UNBIND);
+	} else
+		*police = p;
+
+	return 0;
+}
+
+static inline int
+tcf_dump_police(struct sk_buff *skb, struct tcf_police *police,
+	int police_type)
+{
+	if (police) {
+		struct rtattr * p_rta = (struct rtattr*) skb->tail;
+
+		RTA_PUT(skb, police_type, 0, NULL);
+
+		if (tcf_police_dump(skb, police) < 0)
+			goto rtattr_failure;
+
+		p_rta->rta_len = skb->tail - (u8*)p_rta;
+	}
+	return 0;
+
+rtattr_failure:
+	return -1;
+}
+#endif /* CONFIG_NET_CLS_POLICE */
+
 #endif

[PATCH 2/6] cls_fw: Cleanup fw_classify

From: Thomas Graf <tgraf@suug.ch>
Date: 2004-10-29 00:23:18

Cleans up fw_classify by using the generic routines and
adds a additional but unneeded "continue" to document that
an action may overrule the filter's match result.

Signed-off-by: Thomas Graf <tgraf@suug.ch>
--- linux-2.6.10-rc1-bk7.orig/net/sched/cls_fw.c	2004-10-28 22:03:29.000000000 +0200
+++ linux-2.6.10-rc1-bk7/net/sched/cls_fw.c	2004-10-29 00:00:50.000000000 +0200
@@ -84,47 +84,39 @@
 	u32 id = 0;
 #endif
 
-	if (head == NULL)
-		goto old_method;
-
-	for (f=head->ht[fw_hash(id)]; f; f=f->next) {
-		if (f->id == id) {
-			*res = f->res;
+	if (head != NULL) {
+		for (f=head->ht[fw_hash(id)]; f; f=f->next) {
+			if (f->id == id) {
+				*res = f->res;
 #ifdef CONFIG_NET_CLS_ACT
 #ifdef CONFIG_NET_CLS_IND
-			if (0 != f->indev[0]) {
-				if  (NULL == skb->input_dev) {
+				if (!tcf_match_indev(skb, f->indev))
+					continue;
+#endif /* CONFIG_NET_CLS_IND */
+				if (f->action) {
+					int act_res = tcf_action_exec(skb, f->action, res);
+					if (act_res >= 0)
+						return act_res;
 					continue;
-				} else {
-					if (0 != strcmp(f->indev, skb->input_dev->name)) {
-						continue;
-					}
 				}
-			}
-#endif
-                               if (f->action) {
-                                       int pol_res = tcf_action_exec(skb, f->action, res);
-                                       if (pol_res >= 0)
-                                               return pol_res;
-                               } else
-#else
+#else /* CONFIG_NET_CLS_ACT */
 #ifdef CONFIG_NET_CLS_POLICE
-			if (f->police)
-				return tcf_police(skb, f->police);
-#endif
-#endif
+				if (f->police)
+					return tcf_police(skb, f->police);
+#endif /* CONFIG_NET_CLS_POLICE */
+#endif /* CONFIG_NET_CLS_ACT */
+				return 0;
+			}
+		}
+	} else {
+		/* old method */
+		if (id && (TC_H_MAJ(id) == 0 || !(TC_H_MAJ(id^tp->q->handle)))) {
+			res->classid = id;
+			res->class = 0;
 			return 0;
 		}
 	}
-	return -1;
 
-old_method:
-	if (id && (TC_H_MAJ(id) == 0 ||
-		     !(TC_H_MAJ(id^tp->q->handle)))) {
-		res->classid = id;
-		res->class = 0;
-		return 0;
-	}
 	return -1;
 }
 

[PATCH 3/6] cls_fw: Use generic routines to configure action/policer

From: Thomas Graf <tgraf@suug.ch>
Date: 2004-10-29 00:24:04

Adds a new function fw_change_attr using the new generic routines which
can be used to change attribute but also to initially set them to avoid
duplicated code.

Signed-off-by: Thomas Graf <tgraf@suug.ch>
--- linux-2.6.10-rc1-bk7.orig/net/sched/cls_fw.c	2004-10-29 00:27:10.000000000 +0200
+++ linux-2.6.10-rc1-bk7/net/sched/cls_fw.c	2004-10-29 00:27:19.000000000 +0200
@@ -155,11 +155,8 @@
 
 	for (h=0; h<256; h++) {
 		while ((f=head->ht[h]) != NULL) {
-			unsigned long cl;
 			head->ht[h] = f->next;
-
-			if ((cl = __cls_set_class(&f->res.class, 0)) != 0)
-				tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
+			tcf_unbind_filter(tp, &f->res);
 #ifdef CONFIG_NET_CLS_ACT
        if (f->action) {
                tcf_action_destroy(f->action,TCA_ACT_UNBIND);
@@ -187,14 +184,10 @@
 
 	for (fp=&head->ht[fw_hash(f->id)]; *fp; fp = &(*fp)->next) {
 		if (*fp == f) {
-			unsigned long cl;
-
 			tcf_tree_lock(tp);
 			*fp = f->next;
 			tcf_tree_unlock(tp);
-
-			if ((cl = cls_set_class(tp, &f->res.class, 0)) != 0)
-				tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
+			tcf_unbind_filter(tp, &f->res);
 #ifdef CONFIG_NET_CLS_ACT
        if (f->action) {
                tcf_action_destroy(f->action,TCA_ACT_UNBIND);
@@ -212,21 +205,67 @@
 	return -EINVAL;
 }
 
+static int
+fw_change_attrs(struct tcf_proto *tp, struct fw_filter *f,
+	struct rtattr **tb, struct rtattr **tca, unsigned long base)
+{
+	int err = -EINVAL;
+
+	if (tb[TCA_FW_CLASSID-1]) {
+		if (RTA_PAYLOAD(tb[TCA_FW_CLASSID-1]) != sizeof(u32))
+			goto errout;
+		f->res.classid = *(u32*)RTA_DATA(tb[TCA_FW_CLASSID-1]);
+		tcf_bind_filter(tp, &f->res, base);
+	}
+
+#ifdef CONFIG_NET_CLS_ACT
+	if (tb[TCA_FW_POLICE-1]) {
+		err = tcf_change_act_police(tp, &f->action, tb[TCA_FW_POLICE-1],
+			tca[TCA_RATE-1]);
+		if (err < 0)
+			goto errout;
+	}
+
+	if (tb[TCA_FW_ACT-1]) {
+		err = tcf_change_act(tp, &f->action, tb[TCA_FW_ACT-1],
+			tca[TCA_RATE-1]);
+		if (err < 0)
+			goto errout;
+	}
+
+#ifdef CONFIG_NET_CLS_IND
+	if (tb[TCA_FW_INDEV-1]) {
+		err = tcf_change_indev(tp, f->indev, tb[TCA_FW_INDEV-1]);
+		if (err < 0)
+			goto errout;
+	}
+#endif /* CONFIG_NET_CLS_IND */
+#else /* CONFIG_NET_CLS_ACT */
+#ifdef CONFIG_NET_CLS_POLICE
+	if (tb[TCA_FW_POLICE-1]) {
+		err = tcf_change_police(tp, &f->police, tb[TCA_FW_POLICE-1],
+			tca[TCA_RATE-1]);
+		if (err < 0)
+			goto errout;
+	}
+#endif /* CONFIG_NET_CLS_POLICE */
+#endif /* CONFIG_NET_CLS_ACT */
+
+	err = 0;
+errout:
+	return err;
+}
+
 static int fw_change(struct tcf_proto *tp, unsigned long base,
 		     u32 handle,
 		     struct rtattr **tca,
 		     unsigned long *arg)
 {
 	struct fw_head *head = (struct fw_head*)tp->root;
-	struct fw_filter *f;
+	struct fw_filter *f = (struct fw_filter *) *arg;
 	struct rtattr *opt = tca[TCA_OPTIONS-1];
 	struct rtattr *tb[TCA_FW_MAX];
 	int err;
-#ifdef CONFIG_NET_CLS_ACT
-       struct tc_action *act = NULL;
-       int ret;
-#endif
-
 
 	if (!opt)
 		return handle ? -EINVAL : 0;
@@ -234,85 +273,10 @@
 	if (rtattr_parse(tb, TCA_FW_MAX, RTA_DATA(opt), RTA_PAYLOAD(opt)) < 0)
 		return -EINVAL;
 
-	if ((f = (struct fw_filter*)*arg) != NULL) {
-		/* Node exists: adjust only classid */
-
+	if (f != NULL) {
 		if (f->id != handle && handle)
 			return -EINVAL;
-		if (tb[TCA_FW_CLASSID-1]) {
-			unsigned long cl;
-
-			f->res.classid = *(u32*)RTA_DATA(tb[TCA_FW_CLASSID-1]);
-			cl = tp->q->ops->cl_ops->bind_tcf(tp->q, base, f->res.classid);
-			cl = cls_set_class(tp, &f->res.class, cl);
-			if (cl)
-				tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
-		}
-#ifdef CONFIG_NET_CLS_ACT
-		if (tb[TCA_FW_POLICE-1]) {
-			act = kmalloc(sizeof(*act),GFP_KERNEL);
-			if (NULL == act)
-				return -ENOMEM;
-
-			memset(act,0,sizeof(*act));
-			ret = tcf_action_init_1(tb[TCA_FW_POLICE-1], tca[TCA_RATE-1] ,act,"police",TCA_ACT_NOREPLACE,TCA_ACT_BIND);
-			if (0 > ret){
-				tcf_action_destroy(act,TCA_ACT_UNBIND);
-				return ret;
-			}
-			act->type = TCA_OLD_COMPAT;
-
-			sch_tree_lock(tp->q);
-			act = xchg(&f->action, act);
-			sch_tree_unlock(tp->q);
-
-			tcf_action_destroy(act,TCA_ACT_UNBIND);
-
-		}
-
-		if(tb[TCA_FW_ACT-1]) {
-			act = kmalloc(sizeof(*act),GFP_KERNEL);
-			if (NULL == act)
-				return -ENOMEM;
-			memset(act,0,sizeof(*act));
-			ret = tcf_action_init(tb[TCA_FW_ACT-1], tca[TCA_RATE-1],act,NULL, TCA_ACT_NOREPLACE,TCA_ACT_BIND);
-			if (0 > ret) {
-				tcf_action_destroy(act,TCA_ACT_UNBIND);
-				return ret;
-			}
-
-			sch_tree_lock(tp->q);
-			act = xchg(&f->action, act);
-			sch_tree_unlock(tp->q);
-
-			tcf_action_destroy(act,TCA_ACT_UNBIND);
-		}
-#ifdef CONFIG_NET_CLS_IND
-		if(tb[TCA_FW_INDEV-1]) {
-			struct rtattr *idev = tb[TCA_FW_INDEV-1];
-			if (RTA_PAYLOAD(idev) >= IFNAMSIZ) {
-				printk("cls_fw: bad indev name %s\n",(char*)RTA_DATA(idev));
-				err = -EINVAL;
-				goto errout;
-			}
-			memset(f->indev,0,IFNAMSIZ);
-			sprintf(f->indev, "%s", (char*)RTA_DATA(idev));
-		}
-#endif
-#else /* only POLICE defined */
-#ifdef CONFIG_NET_CLS_POLICE
-		if (tb[TCA_FW_POLICE-1]) {
-			struct tcf_police *police = tcf_police_locate(tb[TCA_FW_POLICE-1], tca[TCA_RATE-1]);
-
-			tcf_tree_lock(tp);
-			police = xchg(&f->police, police);
-			tcf_tree_unlock(tp);
-
-			tcf_police_release(police,TCA_ACT_UNBIND);
-		}
-#endif
-#endif
-		return 0;
+		return fw_change_attrs(tp, f, tb, tca, base);
 	}
 
 	if (!handle)
@@ -336,45 +300,9 @@
 
 	f->id = handle;
 
-	if (tb[TCA_FW_CLASSID-1]) {
-		err = -EINVAL;
-		if (RTA_PAYLOAD(tb[TCA_FW_CLASSID-1]) != 4)
-			goto errout;
-		f->res.classid = *(u32*)RTA_DATA(tb[TCA_FW_CLASSID-1]);
-		cls_set_class(tp, &f->res.class, tp->q->ops->cl_ops->bind_tcf(tp->q, base, f->res.classid));
-	}
-
-#ifdef CONFIG_NET_CLS_ACT
-	if(tb[TCA_FW_ACT-1]) {
-		act = kmalloc(sizeof(*act),GFP_KERNEL);
-		if (NULL == act)
-			return -ENOMEM;
-		memset(act,0,sizeof(*act));
-		ret = tcf_action_init(tb[TCA_FW_ACT-1], tca[TCA_RATE-1],act,NULL,TCA_ACT_NOREPLACE,TCA_ACT_BIND);
-		if (0 > ret) {
-			tcf_action_destroy(act,TCA_ACT_UNBIND);
-			return ret;
-		}
-		f->action= act;
-	}
-#ifdef CONFIG_NET_CLS_IND
-		if(tb[TCA_FW_INDEV-1]) {
-			struct rtattr *idev = tb[TCA_FW_INDEV-1];
-			if (RTA_PAYLOAD(idev) >= IFNAMSIZ) {
-				printk("cls_fw: bad indev name %s\n",(char*)RTA_DATA(idev));
-				err = -EINVAL;
-				goto errout;
-			}
-			memset(f->indev,0,IFNAMSIZ);
-			sprintf(f->indev, "%s", (char*)RTA_DATA(idev));
-		}
-#endif
-#else
-#ifdef CONFIG_NET_CLS_POLICE
-	if (tb[TCA_FW_POLICE-1])
-		f->police = tcf_police_locate(tb[TCA_FW_POLICE-1], tca[TCA_RATE-1]);
-#endif
-#endif
+	err = fw_change_attrs(tp, f, tb, tca, base);
+	if (err < 0)
+		goto errout;
 
 	f->next = head->ht[fw_hash(handle)];
 	tcf_tree_lock(tp);

[PATCH 4/6] cls_fw: Use generic routines to dump action/policer

From: Thomas Graf <tgraf@suug.ch>
Date: 2004-10-29 00:24:59

Signed-off-by: Thomas Graf <tgraf@suug.ch>
--- linux-2.6.10-rc1-bk7.orig/net/sched/cls_fw.c	2004-10-29 00:28:59.000000000 +0200
+++ linux-2.6.10-rc1-bk7/net/sched/cls_fw.c	2004-10-29 00:35:27.000000000 +0200
@@ -375,48 +375,18 @@
 	if (f->res.classid)
 		RTA_PUT(skb, TCA_FW_CLASSID, 4, &f->res.classid);
 #ifdef CONFIG_NET_CLS_ACT
-               /* again for backward compatible mode - we want
-               *  to work with both old and new modes of entering
-               *  tc data even if iproute2  was newer - jhs
-               */
-	if (f->action) {
-		struct rtattr * p_rta = (struct rtattr*)skb->tail;
-
-		if (f->action->type != TCA_OLD_COMPAT) {
-			RTA_PUT(skb, TCA_FW_ACT, 0, NULL);
-			if (tcf_action_dump(skb,f->action,0,0) < 0) {
-				goto rtattr_failure;
-			}
-		} else {
-			RTA_PUT(skb, TCA_FW_POLICE, 0, NULL);
-			if (tcf_action_dump_old(skb,f->action,0,0) < 0) {
-				goto rtattr_failure;
-			}
-		}
-
-		p_rta->rta_len = skb->tail - (u8*)p_rta;
-	}
+	if (tcf_dump_act(skb, f->action, TCA_FW_ACT, TCA_FW_POLICE) < 0)
+		goto rtattr_failure;
 #ifdef CONFIG_NET_CLS_IND
-	if(strlen(f->indev)) {
-		struct rtattr * p_rta = (struct rtattr*)skb->tail;
+	if (strlen(f->indev))
 		RTA_PUT(skb, TCA_FW_INDEV, IFNAMSIZ, f->indev);
-		p_rta->rta_len = skb->tail - (u8*)p_rta;
-	}
-#endif
-#else
+#endif /* CONFIG_NET_CLS_IND */
+#else /* CONFIG_NET_CLS_ACT */
 #ifdef CONFIG_NET_CLS_POLICE
-	if (f->police) {
-		struct rtattr * p_rta = (struct rtattr*)skb->tail;
-
-		RTA_PUT(skb, TCA_FW_POLICE, 0, NULL);
-
-		if (tcf_police_dump(skb, f->police) < 0)
-			goto rtattr_failure;
-
-		p_rta->rta_len = skb->tail - (u8*)p_rta;
-	}
-#endif
-#endif
+	if (tcf_dump_police(skb, f->police, TCA_FW_POLICE) < 0)
+		goto rtattr_failure;
+#endif /* CONFIG_NET_CLS_POLICE */
+#endif /* CONFIG_NET_CLS_ACT */
 
 	rta->rta_len = skb->tail - b;
 #ifdef CONFIG_NET_CLS_ACT

[PATCH 5/6] cls_fw: Whitespace/ifdef fixes

From: Thomas Graf <tgraf@suug.ch>
Date: 2004-10-29 00:25:48

Signed-off-by: Thomas Graf <tgraf@suug.ch>
--- linux-2.6.10-rc1-bk7.orig/net/sched/cls_fw.c	2004-10-29 00:37:34.000000000 +0200
+++ linux-2.6.10-rc1-bk7/net/sched/cls_fw.c	2004-10-29 00:38:46.000000000 +0200
@@ -57,15 +57,15 @@
 	u32			id;
 	struct tcf_result	res;
 #ifdef CONFIG_NET_CLS_ACT
-       struct tc_action        *action;
+	struct tc_action        *action;
 #ifdef CONFIG_NET_CLS_IND
-       char			indev[IFNAMSIZ];
-#endif
-#else
+	char			indev[IFNAMSIZ];
+#endif /* CONFIG_NET_CLS_IND */
+#else /* CONFIG_NET_CLS_ACT */
 #ifdef CONFIG_NET_CLS_POLICE
 	struct tcf_police	*police;
-#endif
-#endif
+#endif /* CONFIG_NET_CLS_POLICE */
+#endif /* CONFIG_NET_CLS_ACT */
 };
 
 static __inline__ int fw_hash(u32 handle)
@@ -158,14 +158,14 @@
 			head->ht[h] = f->next;
 			tcf_unbind_filter(tp, &f->res);
 #ifdef CONFIG_NET_CLS_ACT
-       if (f->action) {
-               tcf_action_destroy(f->action,TCA_ACT_UNBIND);
-       }
-#else
+			if (f->action)
+				tcf_action_destroy(f->action, TCA_ACT_UNBIND);
+#else /* CONFIG_NET_CLS_ACT */
 #ifdef CONFIG_NET_CLS_POLICE
-			tcf_police_release(f->police,TCA_ACT_UNBIND);
-#endif
-#endif
+			if (f->police)
+				tcf_police_release(f->police, TCA_ACT_UNBIND);
+#endif /* CONFIG_NET_CLS_POLICE */
+#endif /* CONFIG_NET_CLS_ACT */
 
 			kfree(f);
 		}
@@ -189,14 +189,13 @@
 			tcf_tree_unlock(tp);
 			tcf_unbind_filter(tp, &f->res);
 #ifdef CONFIG_NET_CLS_ACT
-       if (f->action) {
-               tcf_action_destroy(f->action,TCA_ACT_UNBIND);
-       }
-#else
+			if (f->action)
+				tcf_action_destroy(f->action,TCA_ACT_UNBIND);
+#else /* CONFIG_NET_CLS_ACT */
 #ifdef CONFIG_NET_CLS_POLICE
 			tcf_police_release(f->police,TCA_ACT_UNBIND);
-#endif
-#endif
+#endif /* CONFIG_NET_CLS_POLICE */
+#endif /* CONFIG_NET_CLS_ACT */
 			kfree(f);
 			return 0;
 		}
@@ -358,15 +357,15 @@
 
 	t->tcm_handle = f->id;
 
-       if (!f->res.classid
+	if (!f->res.classid
 #ifdef CONFIG_NET_CLS_ACT
-           && !f->action
+		&& !f->action
 #else
 #ifdef CONFIG_NET_CLS_POLICE
-           && !f->police
+		&& !f->police
 #endif
 #endif
-           )
+		)
 		return skb->len;
 
 	rta = (struct rtattr*)b;
@@ -390,19 +389,19 @@
 
 	rta->rta_len = skb->tail - b;
 #ifdef CONFIG_NET_CLS_ACT
-       if (f->action && f->action->type == TCA_OLD_COMPAT) {
-               if (tcf_action_copy_stats(skb,f->action))
-                       goto rtattr_failure;
-       }
-#else
+	if (f->action && f->action->type == TCA_OLD_COMPAT) {
+		if (tcf_action_copy_stats(skb,f->action))
+			goto rtattr_failure;
+	}
+#else /* CONFIG_NET_CLS_ACT */
 #ifdef CONFIG_NET_CLS_POLICE
 	if (f->police) {
 		if (qdisc_copy_stats(skb, &f->police->stats,
-				     f->police->stats_lock))
+			f->police->stats_lock))
 			goto rtattr_failure;
 	}
-#endif
-#endif
+#endif /* CONFIG_NET_CLS_POLICE */
+#endif /* CONFIG_NET_CLS_ACT */
 	return skb->len;
 
 rtattr_failure:

[PATCH 6/6] PKT_SCHED: break is not enough to stop walking

From: Thomas Graf <tgraf@suug.ch>
Date: 2004-10-29 00:26:28

break is not enough to escape from the walking loops, since
multiple encapsulated loops are used to traverse the hash tables.

Signed-off-by: Thomas Graf <tgraf@suug.ch>

diff -Nru linux-2.6.10-rc1-bk7.orig/net/sched/cls_fw.c linux-2.6.10-rc1-bk7/net/sched/cls_fw.c
--- linux-2.6.10-rc1-bk7.orig/net/sched/cls_fw.c	2004-10-29 00:43:38.000000000 +0200
+++ linux-2.6.10-rc1-bk7/net/sched/cls_fw.c	2004-10-29 00:50:29.000000000 +0200
@@ -338,7 +338,7 @@
 			}
 			if (arg->fn(tp, (unsigned long)f, arg) < 0) {
 				arg->stop = 1;
-				break;
+				return;
 			}
 			arg->count++;
 		}
diff -Nru linux-2.6.10-rc1-bk7.orig/net/sched/cls_route.c linux-2.6.10-rc1-bk7/net/sched/cls_route.c
--- linux-2.6.10-rc1-bk7.orig/net/sched/cls_route.c	2004-10-28 22:03:29.000000000 +0200
+++ linux-2.6.10-rc1-bk7/net/sched/cls_route.c	2004-10-29 01:31:12.000000000 +0200
@@ -538,7 +538,7 @@
 					}
 					if (arg->fn(tp, (unsigned long)f, arg) < 0) {
 						arg->stop = 1;
-						break;
+						return;
 					}
 					arg->count++;
 				}
diff -Nru linux-2.6.10-rc1-bk7.orig/net/sched/cls_rsvp.h linux-2.6.10-rc1-bk7/net/sched/cls_rsvp.h
--- linux-2.6.10-rc1-bk7.orig/net/sched/cls_rsvp.h	2004-10-28 22:03:29.000000000 +0200
+++ linux-2.6.10-rc1-bk7/net/sched/cls_rsvp.h	2004-10-29 01:31:49.000000000 +0200
@@ -601,7 +601,7 @@
 					}
 					if (arg->fn(tp, (unsigned long)f, arg) < 0) {
 						arg->stop = 1;
-						break;
+						return;
 					}
 					arg->count++;
 				}

[RESEND 1/6] PKT_SCHED: Add generic classifier routines

From: Thomas Graf <tgraf@suug.ch>
Date: 2004-10-29 10:23:27

CONFIG_NET_CLS_IND was dependent on CONFIG_NET_CLS_ACT in cls_fw.c which
is not correct according to Kconfig and wouldn't make sense. This is a
revised patch which has this dependency removed.

Adds generic routines used by classifier to:
 - bind/unbind to classes
 - configure action/police/indev
 - dump action/police

Signed-off-by: Thomas Graf <tgraf@suug.ch>
--- linux-2.6.10-rc1-bk7.orig/include/net/pkt_cls.h	2004-10-28 22:03:20.000000000 +0200
+++ linux-2.6.10-rc1-bk7/include/net/pkt_cls.h	2004-10-29 12:11:54.000000000 +0200
@@ -3,6 +3,7 @@
 
 #include <linux/pkt_cls.h>
 #include <net/sch_generic.h>
+#include <net/act_api.h>
 
 /* Basic packet classifier frontend definitions. */
 
@@ -41,4 +42,189 @@
 	return old_cl;
 }
 
+static inline void
+tcf_bind_filter(struct tcf_proto *tp, struct tcf_result *r, unsigned long base)
+{
+	unsigned long cl;
+
+	cl = tp->q->ops->cl_ops->bind_tcf(tp->q, base, r->classid);
+	cl = cls_set_class(tp, &r->class, cl);
+	if (cl)
+		tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
+}
+
+static inline void
+tcf_unbind_filter(struct tcf_proto *tp, struct tcf_result *r)
+{
+	unsigned long cl;
+
+	if ((cl = __cls_set_class(&r->class, 0)) != 0)
+		tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
+}
+
+#ifdef CONFIG_NET_CLS_ACT
+static inline int
+tcf_change_act_police(struct tcf_proto *tp, struct tc_action **action,
+	struct rtattr *act_police_tlv, struct rtattr *rate_tlv)
+{
+	int ret;
+	struct tc_action *act;
+
+	act = kmalloc(sizeof(*act), GFP_KERNEL);
+	if (NULL == act)
+		return -ENOMEM;
+	memset(act, 0, sizeof(*act));
+	
+	ret = tcf_action_init_1(act_police_tlv, rate_tlv, act, "police",
+		TCA_ACT_NOREPLACE, TCA_ACT_BIND);
+	if (ret < 0) {
+		tcf_action_destroy(act, TCA_ACT_UNBIND);
+		return ret;
+	}
+
+	act->type = TCA_OLD_COMPAT;
+
+	if (*action) {
+		tcf_tree_lock(tp);
+		act = xchg(action, act);
+		tcf_tree_unlock(tp);
+
+		tcf_action_destroy(act, TCA_ACT_UNBIND);
+	} else
+		*action = act;
+
+	return 0;
+}
+
+static inline int
+tcf_change_act(struct tcf_proto *tp, struct tc_action **action,
+	struct rtattr *act_tlv, struct rtattr *rate_tlv)
+{
+	int ret;
+	struct tc_action *act;
+
+	act = kmalloc(sizeof(*act), GFP_KERNEL);
+	if (NULL == act)
+		return -ENOMEM;
+	memset(act, 0, sizeof(*act));
+
+	ret = tcf_action_init(act_tlv, rate_tlv, act, NULL,
+		TCA_ACT_NOREPLACE, TCA_ACT_BIND);
+	if (ret < 0) {
+		tcf_action_destroy(act, TCA_ACT_UNBIND);
+		return ret;
+	}
+
+	if (*action) {
+		tcf_tree_lock(tp);
+		act = xchg(action, act);
+		tcf_tree_unlock(tp);
+
+		tcf_action_destroy(act, TCA_ACT_UNBIND);
+	} else
+		*action = act;
+
+	return 0;
+}
+
+static inline int
+tcf_dump_act(struct sk_buff *skb, struct tc_action *action,
+	int act_type, int compat_type)
+{
+	/*
+	 * again for backward compatible mode - we want
+	 * to work with both old and new modes of entering
+	 * tc data even if iproute2  was newer - jhs
+	 */
+	if (action) {
+		struct rtattr * p_rta = (struct rtattr*) skb->tail;
+
+		if (action->type != TCA_OLD_COMPAT) {
+			RTA_PUT(skb, act_type, 0, NULL);
+			if (tcf_action_dump(skb, action, 0, 0) < 0)
+				goto rtattr_failure;
+		} else {
+			RTA_PUT(skb, compat_type, 0, NULL);
+			if (tcf_action_dump_old(skb, action, 0, 0) < 0)
+				goto rtattr_failure;
+		}
+		
+		p_rta->rta_len = skb->tail - (u8*)p_rta;
+	}
+	return 0;
+
+rtattr_failure:
+	return -1;
+}
+#endif /* CONFIG_NET_CLS_ACT */
+
+#ifdef CONFIG_NET_CLS_IND
+static inline int
+tcf_change_indev(struct tcf_proto *tp, char *indev, struct rtattr *indev_tlv)
+{
+	if (RTA_PAYLOAD(indev_tlv) >= IFNAMSIZ) {
+		printk("cls: bad indev name %s\n", (char *) RTA_DATA(indev_tlv));
+		return -EINVAL;
+	}
+
+	memset(indev, 0, IFNAMSIZ);
+	sprintf(indev, "%s", (char *) RTA_DATA(indev_tlv));
+
+	return 0;
+}
+
+static inline int
+tcf_match_indev(struct sk_buff *skb, char *indev)
+{
+	if (0 != indev[0]) {
+		if  (NULL == skb->input_dev)
+			return 0;
+		else if (0 != strcmp(indev, skb->input_dev->name))
+			return 0;
+	}
+
+	return 1;
+}
+#endif /* CONFIG_NET_CLS_IND */
+
+#ifdef CONFIG_NET_CLS_POLICE
+static inline int
+tcf_change_police(struct tcf_proto *tp, struct tcf_police **police,
+	struct rtattr *police_tlv, struct rtattr *rate_tlv)
+{
+	struct tcf_police *p = tcf_police_locate(police_tlv, rate_tlv);
+
+	if (*police) {
+		tcf_tree_lock(tp);
+		p = xchg(police, p);
+		tcf_tree_unlock(tp);
+
+		tcf_police_release(p, TCA_ACT_UNBIND);
+	} else
+		*police = p;
+
+	return 0;
+}
+
+static inline int
+tcf_dump_police(struct sk_buff *skb, struct tcf_police *police,
+	int police_type)
+{
+	if (police) {
+		struct rtattr * p_rta = (struct rtattr*) skb->tail;
+
+		RTA_PUT(skb, police_type, 0, NULL);
+
+		if (tcf_police_dump(skb, police) < 0)
+			goto rtattr_failure;
+
+		p_rta->rta_len = skb->tail - (u8*)p_rta;
+	}
+	return 0;
+
+rtattr_failure:
+	return -1;
+}
+#endif /* CONFIG_NET_CLS_POLICE */
+
 #endif

[PATCH 7/6] cls_fw: CONFIG_NET_CLS_IND is not dependant on CONFIG_NET_CLS_ACT

From: Thomas Graf <tgraf@suug.ch>
Date: 2004-10-29 10:26:20

Signed-off-by: Thomas Graf <tgraf@suug.ch>
--- linux-2.6.10-rc1-bk7.orig/net/sched/cls_fw.c	2004-10-29 12:14:07.000000000 +0200
+++ linux-2.6.10-rc1-bk7/net/sched/cls_fw.c	2004-10-29 12:15:27.000000000 +0200
@@ -56,11 +56,11 @@
 	struct fw_filter	*next;
 	u32			id;
 	struct tcf_result	res;
-#ifdef CONFIG_NET_CLS_ACT
-	struct tc_action        *action;
 #ifdef CONFIG_NET_CLS_IND
 	char			indev[IFNAMSIZ];
 #endif /* CONFIG_NET_CLS_IND */
+#ifdef CONFIG_NET_CLS_ACT
+	struct tc_action        *action;
 #else /* CONFIG_NET_CLS_ACT */
 #ifdef CONFIG_NET_CLS_POLICE
 	struct tcf_police	*police;
@@ -88,11 +88,11 @@
 		for (f=head->ht[fw_hash(id)]; f; f=f->next) {
 			if (f->id == id) {
 				*res = f->res;
-#ifdef CONFIG_NET_CLS_ACT
 #ifdef CONFIG_NET_CLS_IND
 				if (!tcf_match_indev(skb, f->indev))
 					continue;
 #endif /* CONFIG_NET_CLS_IND */
+#ifdef CONFIG_NET_CLS_ACT
 				if (f->action) {
 					int act_res = tcf_action_exec(skb, f->action, res);
 					if (act_res >= 0)
@@ -217,6 +217,14 @@
 		tcf_bind_filter(tp, &f->res, base);
 	}
 
+#ifdef CONFIG_NET_CLS_IND
+	if (tb[TCA_FW_INDEV-1]) {
+		err = tcf_change_indev(tp, f->indev, tb[TCA_FW_INDEV-1]);
+		if (err < 0)
+			goto errout;
+	}
+#endif /* CONFIG_NET_CLS_IND */
+
 #ifdef CONFIG_NET_CLS_ACT
 	if (tb[TCA_FW_POLICE-1]) {
 		err = tcf_change_act_police(tp, &f->action, tb[TCA_FW_POLICE-1],
@@ -231,14 +239,6 @@
 		if (err < 0)
 			goto errout;
 	}
-
-#ifdef CONFIG_NET_CLS_IND
-	if (tb[TCA_FW_INDEV-1]) {
-		err = tcf_change_indev(tp, f->indev, tb[TCA_FW_INDEV-1]);
-		if (err < 0)
-			goto errout;
-	}
-#endif /* CONFIG_NET_CLS_IND */
 #else /* CONFIG_NET_CLS_ACT */
 #ifdef CONFIG_NET_CLS_POLICE
 	if (tb[TCA_FW_POLICE-1]) {
@@ -373,13 +373,13 @@
 
 	if (f->res.classid)
 		RTA_PUT(skb, TCA_FW_CLASSID, 4, &f->res.classid);
-#ifdef CONFIG_NET_CLS_ACT
-	if (tcf_dump_act(skb, f->action, TCA_FW_ACT, TCA_FW_POLICE) < 0)
-		goto rtattr_failure;
 #ifdef CONFIG_NET_CLS_IND
 	if (strlen(f->indev))
 		RTA_PUT(skb, TCA_FW_INDEV, IFNAMSIZ, f->indev);
 #endif /* CONFIG_NET_CLS_IND */
+#ifdef CONFIG_NET_CLS_ACT
+	if (tcf_dump_act(skb, f->action, TCA_FW_ACT, TCA_FW_POLICE) < 0)
+		goto rtattr_failure;
 #else /* CONFIG_NET_CLS_ACT */
 #ifdef CONFIG_NET_CLS_POLICE
 	if (tcf_dump_police(skb, f->police, TCA_FW_POLICE) < 0)

Re: [PATCH 7/6] cls_fw: CONFIG_NET_CLS_IND is not dependant on CONFIG_NET_CLS_ACT

From: jamal <hidden>
Date: 2004-10-29 11:39:40

Thomas, Can you submit equivalent patches for cls_u32.c?
Dave, these all look fine.
Thomas, I will let Dave suck these in first and test against resulting
bk.

cheers,
jamal

Re: [PATCH 7/6] cls_fw: CONFIG_NET_CLS_IND is not dependant on CONFIG_NET_CLS_ACT

From: Thomas Graf <tgraf@suug.ch>
Date: 2004-10-29 11:53:33

* jamal [off-list ref] 2004-10-29 07:39
Thomas, Can you submit equivalent patches for cls_u32.c?
Sure, already done and queued for submission. I will submit them today.
Thomas, I will let Dave suck these in first and test against resulting
bk.
OK.

Regarding the generic statistics for actions:

I thought about dumping the most common stats basic,queue,rate_est
in tcf_action_copy_stats and let the action module dump additional
stats in its get_stats implementation. Any objections in creating
struct tca_act_gen with the content of tca_gen and use it to
access a->priv from act_api.c to get my hands on the statistics
and stats_lock?

Re: [PATCH 7/6] cls_fw: CONFIG_NET_CLS_IND is not dependant on CONFIG_NET_CLS_ACT

From: jamal <hidden>
Date: 2004-10-29 12:35:06

On Fri, 2004-10-29 at 07:53, Thomas Graf wrote:
* jamal [off-list ref] 2004-10-29 07:39
quoted
Thomas, Can you submit equivalent patches for cls_u32.c?
Sure, already done and queued for submission. I will submit them today.
Ok, that will be good because it will help do a one shot test on bk.
Regarding the generic statistics for actions:

I thought about dumping the most common stats basic,queue,rate_est
in tcf_action_copy_stats and let the action module dump additional
stats in its get_stats implementation. 
I think the action code seems clean to me as is.
Here are my thoughts:
Most of them just need queue stats (actually at the moment all of them)
so in tca_gen, struct tc_stats stats needs replacement to make sure they
only have a queue stat because it is generic. All actions call
qdisc_copy_stats calls in their stats dumpers. Replace that call with
the magic you have in the new stats dumping scheme. I think the same
lock can be used for all stats in an action (if new ones are added by an
action).
Any objections in creating
struct tca_act_gen with the content of tca_gen and use it to
access a->priv from act_api.c to get my hands on the statistics
and stats_lock?
Refer to above.

cheers,
jamal

PS:- I hope this is after the bk snapshot with current cleanups you have
is tested?

Re: [PATCH 7/6] cls_fw: CONFIG_NET_CLS_IND is not dependant on CONFIG_NET_CLS_ACT

From: Thomas Graf <tgraf@suug.ch>
Date: 2004-10-29 12:53:56

quoted
Regarding the generic statistics for actions:

I thought about dumping the most common stats basic,queue,rate_est
in tcf_action_copy_stats and let the action module dump additional
stats in its get_stats implementation. 
I think the action code seems clean to me as is.
Here are my thoughts:
Most of them just need queue stats (actually at the moment all of them)
so in tca_gen, struct tc_stats stats needs replacement to make sure they
only have a queue stat because it is generic.
What about the rate estimator and basic stats in pkt_act.h? tca_gen
requires at least basic,queue and rate_est for me to compile.
All actions call
qdisc_copy_stats calls in their stats dumpers. Replace that call with
the magic you have in the new stats dumping scheme.
That's a lot of duplicated code which could be avoided easly. It's
only a cosmetic thing but I don't see any reason for duplicating code :->

So this would be my way of doing it:

tcf_action_copy_stats {
  start_copy()
  copy_basic_stats()
  copy_queue_stats()
  copy_rate_est()
  call get_stats() and let the action module dump additional stats
  finish_copy
}

I'd also suggest to make a new TLV type TCA_ACT_STATS and not reuse
TCA_STATS.

Another issue... do we want the compatibility stuff and provide the
old tc_stats? I'd say no but it's your call.
PS:- I hope this is after the bk snapshot with current cleanups you have
is tested?
Sure. Always planning ahead.

Re: [PATCH 7/6] cls_fw: CONFIG_NET_CLS_IND is not dependant on CONFIG_NET_CLS_ACT

From: jamal <hidden>
Date: 2004-10-29 13:12:41

On Fri, 2004-10-29 at 08:53, Thomas Graf wrote:
What about the rate estimator and basic stats in pkt_act.h? tca_gen
requires at least basic,queue and rate_est for me to compile.
Ok. tca_gen sub for current stats. 
Also most of them have no use for the estimator. I suppose if you ifdef
in tca_gen, then thats fine. 
quoted
All actions call
qdisc_copy_stats calls in their stats dumpers. Replace that call with
the magic you have in the new stats dumping scheme.
That's a lot of duplicated code which could be avoided easly. It's
only a cosmetic thing but I don't see any reason for duplicating code :->

So this would be my way of doing it:

tcf_action_copy_stats {
  start_copy()
  copy_basic_stats()
  copy_queue_stats()
  copy_rate_est()
  call get_stats() and let the action module dump additional stats
  finish_copy
}
Good idea.
I'd also suggest to make a new TLV type TCA_ACT_STATS and not reuse
TCA_STATS.
Fine by me.
Another issue... do we want the compatibility stuff and provide the
old tc_stats? I'd say no but it's your call.
no need for backward compat here. We have an opportunity since its not
widely deployed. I wont feel sorry for any apps that somehow depend on
old stats.

cheers,
jamal

PS:- iproute2 patch needed.
PPS:- I hope all the actions i queued to Dave are in ;->

Re: [PATCH 7/6] cls_fw: CONFIG_NET_CLS_IND is not dependant on CONFIG_NET_CLS_ACT

From: Thomas Graf <tgraf@suug.ch>
Date: 2004-10-29 14:24:11

no need for backward compat here. We have an opportunity since its not
widely deployed. I wont feel sorry for any apps that somehow depend on
old stats.
My thoughts as well.
PS:- iproute2 patch needed.
Should be straight forward if my last patch made it in. Stephen?
PPS:- I hope all the actions i queued to Dave are in ;->
So far gact, mirred and pedit, any more I need to take care of?

Re: [PATCH 7/6] cls_fw: CONFIG_NET_CLS_IND is not dependant on CONFIG_NET_CLS_ACT

From: jamal <hidden>
Date: 2004-10-29 14:53:58

On Fri, 2004-10-29 at 10:24, Thomas Graf wrote:
quoted
no need for backward compat here. We have an opportunity since its not
widely deployed. I wont feel sorry for any apps that somehow depend on
old stats.
My thoughts as well.
quoted
PS:- iproute2 patch needed.
Should be straight forward if my last patch made it in. Stephen?
quoted
PPS:- I hope all the actions i queued to Dave are in ;->
So far gact, mirred and pedit, any more I need to take care of?
ipt  - You could wait for Dave to push the one he has in
or use the attached one - Depends on Dave but should be fair to update
attached one before he wakes up ;->

cheers,
jamal

Re: [PATCH 7/6] cls_fw: CONFIG_NET_CLS_IND is not dependant on CONFIG_NET_CLS_ACT

From: "David S. Miller" <davem@davemloft.net>
Date: 2004-11-02 00:39:01

All 7 (of 6!) patches applied, thanks Thomas :-)

I'll add Jamal's IPT action patch right now too.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help