Skip to contentSkip to conversation

PKT_SCHED: Generic classifier routines / cls_fw cleanup

PATCH In review Thomas Graf 3 participants 17 messages Oct 29, 2004 – Nov 2, 2004

Subsystems (3)
Networking Network drivers Traffic control (tc)
lore
Share & tools
lore

Thomas Graf

author
#0 lore raw

cover PKT_SCHED: Generic classifier routines / cls_fw cleanup

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

author
#1 lore raw

PATCH 1/6 PKT_SCHED: Add generic classifier routines

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

Signed-off-by Thomas Graf <[email protected]>
include/net/pkt_cls.h +187 -0
@@ -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
author
#2 lore raw

PATCH RESEND 1/6 PKT_SCHED: Add generic classifier routines

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 <[email protected]>
include/net/pkt_cls.h +186 -0
@@ -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
author
#3 lore raw

PATCH 2/6 cls_fw: Cleanup fw_classify

In reply to Thomas Graf

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 <[email protected]>
net/sched/cls_fw.c +24 -32
@@ -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;
 }
author
#4 lore raw

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

In reply to Thomas Graf

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 <[email protected]>
net/sched/cls_fw.c +59 -131
@@ -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);
author
#5 lore raw

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

In reply to Thomas Graf
Signed-off-by Thomas Graf <[email protected]>
net/sched/cls_fw.c +9 -39
@@ -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
author
#6 lore raw

PATCH 5/6 cls_fw: Whitespace/ifdef fixes

In reply to Thomas Graf
Signed-off-by Thomas Graf <[email protected]>
net/sched/cls_fw.c +30 -31
@@ -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:
author
#7 lore raw

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

In reply to Thomas Graf

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 <[email protected]>
net/sched/cls_fw.c +1 -1
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
@@ -338,7 +338,7 @@
 			}
 			if (arg->fn(tp, (unsigned long)f, arg) < 0) {
 				arg->stop = 1;
-				break;
+				return;
 			}
 			arg->count++;
 		}
net/sched/cls_route.c +1 -1
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
@@ -538,7 +538,7 @@
 					}
 					if (arg->fn(tp, (unsigned long)f, arg) < 0) {
 						arg->stop = 1;
-						break;
+						return;
 					}
 					arg->count++;
 				}
net/sched/cls_rsvp.h +1 -1
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
@@ -601,7 +601,7 @@
 					}
 					if (arg->fn(tp, (unsigned long)f, arg) < 0) {
 						arg->stop = 1;
-						break;
+						return;
 					}
 					arg->count++;
 				}
author
#8 lore raw

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

In reply to Thomas Graf
Signed-off-by Thomas Graf <[email protected]>
net/sched/cls_fw.c +14 -14
@@ -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)
#9 lore raw

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

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

author
#10 lore raw

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

* jamal <[email protected]> 2004-10-29 07:39

Quoted 1 line

Thomas, Can you submit equivalent patches for cls_u32.c?

Sure, already done and queued for submission. I will submit them today.

Quoted 2 lines

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?

#11 lore raw

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

Quoted 5 lines · Thomas Graf

* jamal <[email protected]> 2004-10-29 07:39

Quoted 1 line

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.

Quoted 5 lines

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

Quoted 4 lines

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?

author
#12 lore raw

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

Quoted 11 lines
Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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

Sure. Always planning ahead.

#13 lore raw

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

Quoted 3 lines · Thomas Graf

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 9 lines
Quoted 3 lines

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:

Quoted 9 lines

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.

Quoted 3 lines

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

Fine by me.

Quoted 3 lines

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

author
#14 lore raw

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

Quoted 3 lines

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

PS:- iproute2 patch needed.

Should be straight forward if my last patch made it in. Stephen?

Quoted 1 line

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?

#15 lore raw

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

Quoted 6 lines · Thomas Graf
Quoted 3 lines

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 7 lines
Quoted 1 line

PS:- iproute2 patch needed.

Should be straight forward if my last patch made it in. Stephen?

Quoted 1 line

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

include/linux/tc_act/tc_ipt.h new file +21 -0
@@ -0,0 +1,21 @@
+#ifndef __LINUX_TC_IPT_H
+#define __LINUX_TC_IPT_H
+
+#include <linux/pkt_cls.h>
+
+#define TCA_ACT_IPT 6
+
+enum
+{
+	TCA_IPT_UNSPEC,
+	TCA_IPT_TABLE,
+	TCA_IPT_HOOK,
+	TCA_IPT_INDEX,
+	TCA_IPT_CNT,
+	TCA_IPT_TM,
+	TCA_IPT_TARG,
+	__TCA_IPT_MAX
+};
+#define TCA_IPT_MAX (__TCA_IPT_MAX - 1)
+                                                                                
+#endif
include/net/tc_act/tc_ipt.h new file +16 -0
@@ -0,0 +1,16 @@
+#ifndef __NET_TC_IPT_H
+#define __NET_TC_IPT_H
+
+#include <net/pkt_sched.h>
+
+struct ipt_entry_target;
+
+struct tcf_ipt
+{
+	tca_gen(ipt);
+	u32 hook;
+	char *tname;
+	struct ipt_entry_target *t;
+};
+
+#endif
net/sched/ipt.c new file +392 -0

392 lines not on this page · raw message

net/sched/Makefile +1 -0
@@ -12,6 +12,7 @@
 obj-$(CONFIG_NET_CLS_POLICE)	+= police.o
 obj-$(CONFIG_NET_ACT_GACT)      += gact.o
 obj-$(CONFIG_NET_ACT_MIRRED)    += mirred.o
+obj-$(CONFIG_NET_ACT_IPT)       += ipt.o
 obj-$(CONFIG_NET_SCH_CBQ)	+= sch_cbq.o
 obj-$(CONFIG_NET_SCH_HTB)	+= sch_htb.o
 obj-$(CONFIG_NET_SCH_HPFQ)	+= sch_hpfq.o
net/sched/Kconfig +8 -0
@@ -406,3 +406,11 @@
         ---help---
         requires new iproute2
         This allows packets to be mirrored or redirected to netdevices
+
+config NET_ACT_IPT
+        tristate '      iptables Actions'
+        depends on NET_CLS_ACT && NETFILTER && IP_NF_IPTABLES
+        ---help---
+        requires new iproute2
+        This allows iptables targets to be used by tc filters
+
maintainer
#16 lore raw

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

In reply to Thomas Graf

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

I'll add Jamal's IPT action patch right now too.