[PATCH 0/9] PKT_SCHED: tcf_exts API & make classifier changes consistent upon failure

[PATCH 0/9] PKT_SCHED: tcf_exts API & make classifier changes consistent upon failure

[Thomas Graf]

Dave,

The following patchset adds tcf_exts API to remove the ifdef clutter,
helps make changes consistent upon failures, makes adding action bits
to yet unsupported classifiers as easy as adding a few lines and
generally saves a lot of code per classifier. The patches to make use
of the API add action support to all classifiers and makes changes
consistent except for indev failures (to be removed soon) and rsvp
which needs a closer look since i haven't had time to perfectly
understand it yet.

I gave it extensive testing for 2 weeks, patches 5-6 are non-trivial
though and it's quite hard to test everything. I will post a request
to the lartc mailinglist once these changes are in a bk release so
everyone can try it out and I have time fix any remaining issues
before the next rc release. I could provide the patchset but its
quite some work for the testers and I guess there will be more people
trying it out if they can use an official release.

Cheers, Thomas

[PATCH 1/9] PKT_SCHED: rtattr_parse shortcut for nested TLVs

[Thomas Graf]

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

--- linux-2.6.10.orig/include/linux/rtnetlink.h	2004-12-25 23:21:18.000000000 +0100
+++ linux-2.6.10/include/linux/rtnetlink.h	2004-12-26 19:52:21.000000000 +0100
@@ -756,6 +756,9 @@
 
 extern int rtattr_parse(struct rtattr *tb[], int maxattr, struct rtattr *rta, int len);
 
+#define rtattr_parse_nested(tb, max, rta) \
+	rtattr_parse((tb), (max), RTA_DATA((rta)), RTA_PAYLOAD((rta)))
+
 extern struct sock *rtnl;
 
 struct rtnetlink_link

[PATCH 2/9] PKT_SCHED: tc filter extension API

[Thomas Graf]

The tcf_exts API abstracts extensions such as actions/policers
into a generic layer and reduces the knowledge inside classifiers
to the minimum required. It isolates the validation code into
its own function to allow classifiers to validate all input
data before making changes and thus avoids the need to undo changes
if a extension configuration request cannot be fullfilled.

As a nice side effect, using this API removes the existing
ifdef clutter.

Usage:
  The classifier holds struct tcf_exts which may be empty if no
  extensions are compiled in. It then calls tcf_exts_validate
  when a new change request was received and provides a temporary
  tcf_exts copy to store the change requests. Given it succeeded
  the classifier may change its own parameters and at the end
  call tcf_exts_change to commit the changes and replace the
  existing extension configuration with the new one. The classifier
  is responsible to destroy his temporary copy if any of its own
  validation checks fail.

  The classifier specific TLV types must be exported to the extensions
  API via tcf_ext_map.

  Destroying the extensions is as easy as calling tcf_exts_destroy.

  The extensions are executed by the classifier by calling tcf_exts_exec
  which must be done as the last thing after making sure the
  filter matches. Note: A classifier might take further actions after
  the execution to tcf_exts_exec such as correcting its own cache to
  avoid caching results which could have been influenced by the extensions.

  tcf_exts_exec returns a negative error code if the filter must be
  considered unmatched, 0 on normal execution or a positive classifier
  return code (TC_ACT_*) which must be returned to the underlying layer
  as-is.

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

--- linux-2.6.10-bk2.orig/include/net/pkt_cls.h	2004-12-30 01:22:01.000000000 +0100
+++ linux-2.6.10-bk2/include/net/pkt_cls.h	2004-12-30 01:22:39.000000000 +0100
@@ -62,6 +62,93 @@
 		tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
 }
 
+struct tcf_exts
+{
+#ifdef CONFIG_NET_CLS_ACT
+	struct tc_action *action;
+#elif defined CONFIG_NET_CLS_POLICE
+	struct tcf_police *police;
+#endif
+};
+
+/* Map to export classifier specific extension TLV types to the
+ * generic extensions API. Unsupported extensions must be set to 0.
+ */
+struct tcf_ext_map
+{
+	int action;
+	int police;
+};
+
+/**
+ * tcf_exts_is_predicative - check if a predicative extension is present
+ * @exts: tc filter extensions handle
+ *
+ * Returns 1 if a predicative extension is present, i.e. an extension which
+ * might cause further actions and thus overrule the regular tcf_result.
+ */
+static inline int
+tcf_exts_is_predicative(struct tcf_exts *exts)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	return !!exts->action;
+#elif defined CONFIG_NET_CLS_POLICE
+	return !!exts->police;
+#else
+	return 0;
+#endif
+}
+
+/**
+ * tcf_exts_is_available - check if at least one extension is present
+ * @exts: tc filter extensions handle
+ *
+ * Returns 1 if at least one extension is present.
+ */
+static inline int
+tcf_exts_is_available(struct tcf_exts *exts)
+{
+	/* All non-predicative extensions must be added here. */
+	return tcf_exts_is_predicative(exts);
+}
+
+/**
+ * tcf_exts_exec - execute tc filter extensions
+ * @skb: socket buffer
+ * @exts: tc filter extensions handle
+ * @res: desired result
+ *
+ * Executes all configured extensions. Returns 0 on a normal execution,
+ * a negative number if the filter must be considered unmatched or
+ * a positive action code (TC_ACT_*) which must be returned to the
+ * underlying layer.
+ */
+static inline int
+tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts,
+	       struct tcf_result *res)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (exts->action)
+		return tcf_action_exec(skb, exts->action, res);
+#elif defined CONFIG_NET_CLS_POLICE
+	if (exts->police)
+		return tcf_police(skb, exts->police);
+#endif
+
+	return 0;
+}
+
+extern int tcf_exts_validate(struct tcf_proto *tp, struct rtattr **tb,
+	                     struct rtattr *rate_tlv, struct tcf_exts *exts,
+	                     struct tcf_ext_map *map);
+extern void tcf_exts_destroy(struct tcf_proto *tp, struct tcf_exts *exts);
+extern void tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
+	                     struct tcf_exts *src);
+extern int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts,
+	                 struct tcf_ext_map *map);
+extern int tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts,
+	                       struct tcf_ext_map *map);
+
 #ifdef CONFIG_NET_CLS_ACT
 static inline int
 tcf_change_act_police(struct tcf_proto *tp, struct tc_action **action,
--- linux-2.6.10-bk2.orig/net/sched/cls_api.c	2004-12-30 01:22:01.000000000 +0100
+++ linux-2.6.10-bk2/net/sched/cls_api.c	2004-12-30 00:47:06.000000000 +0100
@@ -439,6 +439,162 @@
 	return skb->len;
 }
 
+void
+tcf_exts_destroy(struct tcf_proto *tp, struct tcf_exts *exts)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (exts->action) {
+		tcf_action_destroy(exts->action, TCA_ACT_UNBIND);
+		exts->action = NULL;
+	}
+#elif defined CONFIG_NET_CLS_POLICE
+	if (exts->police) {
+		tcf_police_release(exts->police, TCA_ACT_UNBIND);
+		exts->police = NULL;
+	}
+#endif
+}
+
+
+int
+tcf_exts_validate(struct tcf_proto *tp, struct rtattr **tb,
+	          struct rtattr *rate_tlv, struct tcf_exts *exts,
+	          struct tcf_ext_map *map)
+{
+	memset(exts, 0, sizeof(*exts));
+	
+#ifdef CONFIG_NET_CLS_ACT
+	int err;
+	struct tc_action *act;
+
+	if (map->police && tb[map->police-1] && rate_tlv) {
+		act = tcf_action_init_1(tb[map->police-1], rate_tlv, "police",
+			TCA_ACT_NOREPLACE, TCA_ACT_BIND, &err);
+		if (NULL == act)
+			return err;
+
+		act->type = TCA_OLD_COMPAT;
+		exts->action = act;
+	} else if (map->action && tb[map->action-1] && rate_tlv) {
+		act = tcf_action_init(tb[map->action-1], rate_tlv, NULL,
+			TCA_ACT_NOREPLACE, TCA_ACT_BIND, &err);
+		if (NULL == act)
+			return err;
+
+		exts->action = act;
+	}
+#elif defined CONFIG_NET_CLS_POLICE
+	if (map->police && tb[map->police-1] && rate_tlv) {
+		struct tcf_police *p;
+		
+		p = tcf_police_locate(tb[map->police-1], rate_tlv);
+		if (NULL == p)
+			return -EINVAL;
+
+		exts->police = p;
+	} else if (map->action && tb[map->action-1])
+		return -EOPNOTSUPP;
+#else
+	if ((map->action && tb[map->action-1]) ||
+	    (map->police && tb[map->police-1]))
+		return -EOPNOTSUPP;
+#endif
+
+	return 0;
+}
+
+void
+tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
+	        struct tcf_exts *src)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (src->action) {
+		if (dst->action) {
+			struct tc_action *act;
+
+			tcf_tree_lock(tp);
+			act = xchg(&dst->action, src->action);
+			tcf_tree_unlock(tp);
+
+			tcf_action_destroy(act, TCA_ACT_UNBIND);
+		} else
+			dst->action = src->action;
+	}
+#elif defined CONFIG_NET_CLS_POLICE
+	if (src->police) {
+		if (dst->police) {
+			struct tcf_police *p;
+
+			tcf_tree_lock(tp);
+			p = xchg(&dst->police, src->police);
+			tcf_tree_unlock(tp);
+
+			tcf_police_release(p, TCA_ACT_UNBIND);
+		} else
+			dst->police = src->police;
+	}
+#endif
+}
+
+int
+tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts,
+	      struct tcf_ext_map *map)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (map->action && exts->action) {
+		/*
+		 * 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
+		 */
+		struct rtattr * p_rta = (struct rtattr*) skb->tail;
+
+		if (exts->action->type != TCA_OLD_COMPAT) {
+			RTA_PUT(skb, map->action, 0, NULL);
+			if (tcf_action_dump(skb, exts->action, 0, 0) < 0)
+				goto rtattr_failure;
+			p_rta->rta_len = skb->tail - (u8*)p_rta;
+		} else if (map->police) {
+			RTA_PUT(skb, map->police, 0, NULL);
+			if (tcf_action_dump_old(skb, exts->action, 0, 0) < 0)
+				goto rtattr_failure;
+			p_rta->rta_len = skb->tail - (u8*)p_rta;
+		}
+	}
+#elif defined CONFIG_NET_CLS_POLICE
+	if (map->police && exts->police) {
+		struct rtattr * p_rta = (struct rtattr*) skb->tail;
+
+		RTA_PUT(skb, map->police, 0, NULL);
+
+		if (tcf_police_dump(skb, exts->police) < 0)
+			goto rtattr_failure;
+
+		p_rta->rta_len = skb->tail - (u8*)p_rta;
+	}
+#endif
+	return 0;
+rtattr_failure:
+	return -1;
+}
+
+int
+tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts,
+	            struct tcf_ext_map *map)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (exts->action)
+		if (tcf_action_copy_stats(skb, exts->action) < 0)
+			goto rtattr_failure;
+#elif defined CONFIG_NET_CLS_POLICE
+	if (exts->police)
+		if (tcf_police_dump_stats(skb, exts->police) < 0)
+			goto rtattr_failure;
+#endif
+	return 0;
+rtattr_failure:
+	return -1;
+}
 
 static int __init tc_filter_init(void)
 {
@@ -461,3 +617,8 @@
 
 EXPORT_SYMBOL(register_tcf_proto_ops);
 EXPORT_SYMBOL(unregister_tcf_proto_ops);
+EXPORT_SYMBOL(tcf_exts_validate);
+EXPORT_SYMBOL(tcf_exts_destroy);
+EXPORT_SYMBOL(tcf_exts_change);
+EXPORT_SYMBOL(tcf_exts_dump);
+EXPORT_SYMBOL(tcf_exts_dump_stats);

[PATCH 3/9] PKT_SCHED: u32: make use of tcf_exts API

[Thomas Graf]

Transforms u32 to use tcf_exts API. Makes the u32 changing
procedure consistent upon failures except for indev failures but
indev will be removed very soon.

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

--- linux-2.6.10-bk2.orig/net/sched/cls_u32.c	2004-12-29 20:16:24.000000000 +0100
+++ linux-2.6.10-bk2/net/sched/cls_u32.c	2004-12-29 20:19:49.000000000 +0100
@@ -61,9 +61,9 @@
 
 struct tc_u32_mark
 {
-	__u32		val;
-	__u32		mask;
-	__u32		success;
+	u32		val;
+	u32		mask;
+	u32		success;
 };
 
 struct tc_u_knode
@@ -71,13 +71,7 @@
 	struct tc_u_knode	*next;
 	u32			handle;
 	struct tc_u_hnode	*ht_up;
-#ifdef CONFIG_NET_CLS_ACT
-	struct tc_action	*action;
-#else
-#ifdef CONFIG_NET_CLS_POLICE
-	struct tcf_police	*police;
-#endif
-#endif
+	struct tcf_exts		exts;
 #ifdef CONFIG_NET_CLS_IND
 	char                     indev[IFNAMSIZ];
 #endif
@@ -112,6 +106,11 @@
 	u32			hgenerator;
 };
 
+static struct tcf_ext_map u32_ext_map = {
+	.action = TCA_U32_ACT,
+	.police = TCA_U32_POLICE
+};
+
 static struct tc_u_common *u32_list;
 
 static __inline__ unsigned u32_hash_fold(u32 key, struct tc_u32_sel *sel, u8 fshift)
@@ -137,7 +136,7 @@
 #ifdef CONFIG_CLS_U32_PERF
 	int j;
 #endif
-	int i;
+	int i, r;
 
 next_ht:
 	n = ht->ht[sel];
@@ -185,22 +184,13 @@
 #ifdef CONFIG_CLS_U32_PERF
 				n->pf->rhit +=1;
 #endif
-#ifdef CONFIG_NET_CLS_ACT
-				if (n->action) {
-					int pol_res = tcf_action_exec(skb, n->action, res);
-					if (pol_res >= 0)
-						return pol_res;
-				} else
-#else
-#ifdef CONFIG_NET_CLS_POLICE
-				if (n->police) {
-					int pol_res = tcf_police(skb, n->police);
-					if (pol_res >= 0)
-						return pol_res;
-				} else
-#endif
-#endif
-					return 0;
+				r = tcf_exts_exec(skb, &n->exts, res);
+				if (r < 0) {
+					n = n->next;
+					goto next_knode;
+				}
+
+				return r;
 			}
 			n = n->next;
 			goto next_knode;
@@ -359,15 +349,7 @@
 static int u32_destroy_key(struct tcf_proto *tp, struct tc_u_knode *n)
 {
 	tcf_unbind_filter(tp, &n->res);
-#ifdef CONFIG_NET_CLS_ACT
-	if (n->action) {
-		tcf_action_destroy(n->action, TCA_ACT_UNBIND);
-	}
-#else
-#ifdef CONFIG_NET_CLS_POLICE
-	tcf_police_release(n->police, TCA_ACT_UNBIND);
-#endif
-#endif
+	tcf_exts_destroy(tp, &n->exts);
 	if (n->ht_down)
 		n->ht_down->refcnt--;
 #ifdef CONFIG_CLS_U32_PERF
@@ -509,18 +491,26 @@
 			 struct tc_u_knode *n, struct rtattr **tb,
 			 struct rtattr *est)
 {
+	int err;
+	struct tcf_exts e;
+
+	err = tcf_exts_validate(tp, tb, est, &e, &u32_ext_map);
+	if (err < 0)
+		return err;
+
+	err = -EINVAL;
 	if (tb[TCA_U32_LINK-1]) {
 		u32 handle = *(u32*)RTA_DATA(tb[TCA_U32_LINK-1]);
 		struct tc_u_hnode *ht_down = NULL;
 
 		if (TC_U32_KEY(handle))
-			return -EINVAL;
+			goto errout;
 
 		if (handle) {
 			ht_down = u32_lookup_ht(ht->tp_c, handle);
 
 			if (ht_down == NULL)
-				return -EINVAL;
+				goto errout;
 			ht_down->refcnt++;
 		}
 
@@ -535,36 +525,20 @@
 		n->res.classid = *(u32*)RTA_DATA(tb[TCA_U32_CLASSID-1]);
 		tcf_bind_filter(tp, &n->res, base);
 	}
-#ifdef CONFIG_NET_CLS_ACT
-	if (tb[TCA_U32_POLICE-1]) {
-		int err = tcf_change_act_police(tp, &n->action, tb[TCA_U32_POLICE-1], est);
-		if (err < 0)
-			return err;
-	}
 
-	if (tb[TCA_U32_ACT-1]) {
-		int err = tcf_change_act(tp, &n->action, tb[TCA_U32_ACT-1], est);
-		if (err < 0)
-			return err;
-	}
-#else
-#ifdef CONFIG_NET_CLS_POLICE
-	if (tb[TCA_U32_POLICE-1]) {
-		int err = tcf_change_police(tp, &n->police, tb[TCA_U32_POLICE-1], est);
-		if (err < 0)
-			return err;
-	}
-#endif
-#endif
 #ifdef CONFIG_NET_CLS_IND
 	if (tb[TCA_U32_INDEV-1]) {
 		int err = tcf_change_indev(tp, n->indev, tb[TCA_U32_INDEV-1]);
 		if (err < 0)
-			return err;
+			goto errout;
 	}
 #endif
+	tcf_exts_change(tp, &n->exts, &e);
 
 	return 0;
+errout:
+	tcf_exts_destroy(tp, &e);
+	return err;
 }
 
 static int u32_change(struct tcf_proto *tp, unsigned long base, u32 handle,
@@ -584,7 +558,7 @@
 	if (opt == NULL)
 		return handle ? -EINVAL : 0;
 
-	if (rtattr_parse(tb, TCA_U32_MAX, RTA_DATA(opt), RTA_PAYLOAD(opt)) < 0)
+	if (rtattr_parse_nested(tb, TCA_U32_MAX, opt) < 0)
 		return -EINVAL;
 
 	if ((n = (struct tc_u_knode*)*arg) != NULL) {
@@ -657,12 +631,12 @@
 
 	memset(n, 0, sizeof(*n) + s->nkeys*sizeof(struct tc_u32_key));
 #ifdef CONFIG_CLS_U32_PERF
-	n->pf = kmalloc(sizeof(struct tc_u32_pcnt) + s->nkeys*sizeof(__u64), GFP_KERNEL);
+	n->pf = kmalloc(sizeof(struct tc_u32_pcnt) + s->nkeys*sizeof(u64), GFP_KERNEL);
 	if (n->pf == NULL) {
 		kfree(n);
 		return -ENOBUFS;
 	}
-	memset(n->pf, 0, sizeof(struct tc_u32_pcnt) + s->nkeys*sizeof(__u64));
+	memset(n->pf, 0, sizeof(struct tc_u32_pcnt) + s->nkeys*sizeof(u64));
 #endif
 
 	memcpy(&n->sel, s, sizeof(*s) + s->nkeys*sizeof(struct tc_u32_key));
@@ -783,15 +757,8 @@
 			RTA_PUT(skb, TCA_U32_MARK, sizeof(n->mark), &n->mark);
 #endif
 
-#ifdef CONFIG_NET_CLS_ACT
-		if (tcf_dump_act(skb, n->action, TCA_U32_ACT, TCA_U32_POLICE) < 0)
-			goto rtattr_failure;
-#else
-#ifdef CONFIG_NET_CLS_POLICE
-		if (tcf_dump_police(skb, n->police, TCA_U32_POLICE) < 0)
+		if (tcf_exts_dump(skb, &n->exts, &u32_ext_map) < 0)
 			goto rtattr_failure;
-#endif
-#endif
 
 #ifdef CONFIG_NET_CLS_IND
 		if(strlen(n->indev))
@@ -799,26 +766,15 @@
 #endif
 #ifdef CONFIG_CLS_U32_PERF
 		RTA_PUT(skb, TCA_U32_PCNT, 
-		sizeof(struct tc_u32_pcnt) + n->sel.nkeys*sizeof(__u64),
+		sizeof(struct tc_u32_pcnt) + n->sel.nkeys*sizeof(u64),
 			n->pf);
 #endif
 	}
 
 	rta->rta_len = skb->tail - b;
-#ifdef CONFIG_NET_CLS_ACT
-	if (TC_U32_KEY(n->handle) != 0) {
-		if (TC_U32_KEY(n->handle) && n->action && n->action->type == TCA_OLD_COMPAT) {
-			if (tcf_action_copy_stats(skb,n->action))
-				goto rtattr_failure;
-		}
-	}
-#else
-#ifdef CONFIG_NET_CLS_POLICE
-	if (TC_U32_KEY(n->handle) && n->police)
-		if (tcf_police_dump_stats(skb, n->police) < 0)
+	if (TC_U32_KEY(n->handle))
+		if (tcf_exts_dump_stats(skb, &n->exts, &u32_ext_map) < 0)
 			goto rtattr_failure;
-#endif
-#endif
 	return skb->len;
 
 rtattr_failure:

[PATCH 4/9] PKT_SCHED: fw: make use of tcf_exts API

[Thomas Graf]

Transforms fw to use tcf_exts API. Makes the fw changing
procedure consistent upon failures except for indev failures but
indev will be removed very soon.

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

--- linux-2.6.10-bk2.orig/net/sched/cls_fw.c	2004-12-29 20:16:24.000000000 +0100
+++ linux-2.6.10-bk2/net/sched/cls_fw.c	2004-12-29 20:20:26.000000000 +0100
@@ -59,13 +59,12 @@
 #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;
-#endif /* CONFIG_NET_CLS_POLICE */
-#endif /* CONFIG_NET_CLS_ACT */
+	struct tcf_exts		exts;
+};
+
+static struct tcf_ext_map fw_ext_map = {
+	.action = TCA_FW_ACT,
+	.police = TCA_FW_POLICE
 };
 
 static __inline__ int fw_hash(u32 handle)
@@ -78,6 +77,7 @@
 {
 	struct fw_head *head = (struct fw_head*)tp->root;
 	struct fw_filter *f;
+	int r;
 #ifdef CONFIG_NETFILTER
 	u32 id = skb->nfmark;
 #else
@@ -92,20 +92,11 @@
 				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)
-						return act_res;
+				r = tcf_exts_exec(skb, &f->exts, res);
+				if (r < 0)
 					continue;
-				}
-#else /* CONFIG_NET_CLS_ACT */
-#ifdef CONFIG_NET_CLS_POLICE
-				if (f->police)
-					return tcf_police(skb, f->police);
-#endif /* CONFIG_NET_CLS_POLICE */
-#endif /* CONFIG_NET_CLS_ACT */
-				return 0;
+
+				return r;
 			}
 		}
 	} else {
@@ -144,6 +135,14 @@
 	return 0;
 }
 
+static inline void
+fw_delete_filter(struct tcf_proto *tp, struct fw_filter *f)
+{
+	tcf_unbind_filter(tp, &f->res);
+	tcf_exts_destroy(tp, &f->exts);
+	kfree(f);
+}
+
 static void fw_destroy(struct tcf_proto *tp)
 {
 	struct fw_head *head = (struct fw_head*)xchg(&tp->root, NULL);
@@ -156,18 +155,7 @@
 	for (h=0; h<256; h++) {
 		while ((f=head->ht[h]) != NULL) {
 			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 /* CONFIG_NET_CLS_ACT */
-#ifdef CONFIG_NET_CLS_POLICE
-			if (f->police)
-				tcf_police_release(f->police, TCA_ACT_UNBIND);
-#endif /* CONFIG_NET_CLS_POLICE */
-#endif /* CONFIG_NET_CLS_ACT */
-
-			kfree(f);
+			fw_delete_filter(tp, f);
 		}
 	}
 	kfree(head);
@@ -187,16 +175,7 @@
 			tcf_tree_lock(tp);
 			*fp = f->next;
 			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 /* CONFIG_NET_CLS_ACT */
-#ifdef CONFIG_NET_CLS_POLICE
-			tcf_police_release(f->police,TCA_ACT_UNBIND);
-#endif /* CONFIG_NET_CLS_POLICE */
-#endif /* CONFIG_NET_CLS_ACT */
-			kfree(f);
+			fw_delete_filter(tp, f);
 			return 0;
 		}
 	}
@@ -208,8 +187,14 @@
 fw_change_attrs(struct tcf_proto *tp, struct fw_filter *f,
 	struct rtattr **tb, struct rtattr **tca, unsigned long base)
 {
-	int err = -EINVAL;
+	struct tcf_exts e;
+	int err;
 
+	err = tcf_exts_validate(tp, tb, tca[TCA_RATE-1], &e, &fw_ext_map);
+	if (err < 0)
+		return err;
+
+	err = -EINVAL;
 	if (tb[TCA_FW_CLASSID-1]) {
 		if (RTA_PAYLOAD(tb[TCA_FW_CLASSID-1]) != sizeof(u32))
 			goto errout;
@@ -225,33 +210,11 @@
 	}
 #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],
-			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;
-	}
-#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 */
+	tcf_exts_change(tp, &f->exts, &e);
 
-	err = 0;
+	return 0;
 errout:
+	tcf_exts_destroy(tp, &e);
 	return err;
 }
 
@@ -269,7 +232,7 @@
 	if (!opt)
 		return handle ? -EINVAL : 0;
 
-	if (rtattr_parse(tb, TCA_FW_MAX, RTA_DATA(opt), RTA_PAYLOAD(opt)) < 0)
+	if (rtattr_parse_nested(tb, TCA_FW_MAX, opt) < 0)
 		return -EINVAL;
 
 	if (f != NULL) {
@@ -357,15 +320,7 @@
 
 	t->tcm_handle = f->id;
 
-	if (!f->res.classid
-#ifdef CONFIG_NET_CLS_ACT
-		&& !f->action
-#else
-#ifdef CONFIG_NET_CLS_POLICE
-		&& !f->police
-#endif
-#endif
-		)
+	if (!f->res.classid && !tcf_exts_is_available(&f->exts))
 		return skb->len;
 
 	rta = (struct rtattr*)b;
@@ -377,29 +332,15 @@
 	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)
+
+	if (tcf_exts_dump(skb, &f->exts, &fw_ext_map) < 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
-	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 (tcf_police_dump_stats(skb, f->police) < 0)
-			goto rtattr_failure;
-#endif /* CONFIG_NET_CLS_POLICE */
-#endif /* CONFIG_NET_CLS_ACT */
+
+	if (tcf_exts_dump_stats(skb, &f->exts, &fw_ext_map) < 0)
+		goto rtattr_failure;
+
 	return skb->len;
 
 rtattr_failure:

[PATCH 5/9] PKT_SCHED: route: allow changing parameters for existing filters and use tcf_exts API

[Thomas Graf]

Transforms route to use tcf_exts API and thus adds support for
actions. Replaces the existing change implementation with a new one
supporting changes for existing filters which allows to change a
classifier without letting a single packet pass by unclassified.

Fixes various cases where a error is returned but the filter was
changed already.

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

--- linux-2.6.10-bk1.orig/include/linux/pkt_cls.h	2004-12-27 21:34:52.000000000 +0100
+++ linux-2.6.10-bk1/include/linux/pkt_cls.h	2004-12-27 21:46:40.000000000 +0100
@@ -280,6 +280,7 @@
 	TCA_ROUTE4_FROM,
 	TCA_ROUTE4_IIF,
 	TCA_ROUTE4_POLICE,
+	TCA_ROUTE4_ACT,
 	__TCA_ROUTE4_MAX
 };
 
--- linux-2.6.10-bk2.orig/net/sched/cls_route.c	2004-12-29 20:16:24.000000000 +0100
+++ linux-2.6.10-bk2/net/sched/cls_route.c	2004-12-29 20:21:15.000000000 +0100
@@ -59,6 +59,7 @@
 
 struct route4_bucket
 {
+	/* 16 FROM buckets + 16 IIF buckets + 1 wildcard bucket */
 	struct route4_filter	*ht[16+16+1];
 };
 
@@ -69,22 +70,25 @@
 	int			iif;
 
 	struct tcf_result	res;
-#ifdef CONFIG_NET_CLS_POLICE
-	struct tcf_police	*police;
-#endif
-
+	struct tcf_exts		exts;
 	u32			handle;
 	struct route4_bucket	*bkt;
 };
 
 #define ROUTE4_FAILURE ((struct route4_filter*)(-1L))
 
+static struct tcf_ext_map route_ext_map = {
+	.police = TCA_ROUTE4_POLICE,
+	.action = TCA_ROUTE4_ACT
+};
+
 static __inline__ int route4_fastmap_hash(u32 id, int iif)
 {
 	return id&0xF;
 }
 
-static void route4_reset_fastmap(struct net_device *dev, struct route4_head *head, u32 id)
+static inline
+void route4_reset_fastmap(struct net_device *dev, struct route4_head *head, u32 id)
 {
 	spin_lock_bh(&dev->queue_lock);
 	memset(head->fastmap, 0, sizeof(head->fastmap));
@@ -121,19 +125,20 @@
 	return 32;
 }
 
-#ifdef CONFIG_NET_CLS_POLICE
-#define IF_ROUTE_POLICE \
-if (f->police) { \
-	int pol_res = tcf_police(skb, f->police); \
-	if (pol_res >= 0) return pol_res; \
-	dont_cache = 1; \
-	continue; \
-} \
-if (!dont_cache)
-#else
-#define IF_ROUTE_POLICE
-#endif
-
+#define ROUTE4_APPLY_RESULT()						\
+	do {								\
+		*res = f->res;						\
+		if (tcf_exts_is_available(&f->exts)) {			\
+			int r = tcf_exts_exec(skb, &f->exts, res);	\
+			if (r < 0) {					\
+				dont_cache = 1;				\
+				continue;				\
+			}						\
+			return r;					\
+		} else if (!dont_cache)					\
+			route4_set_fastmap(head, id, iif, f);		\
+		return 0;						\
+	} while(0)
 
 static int route4_classify(struct sk_buff *skb, struct tcf_proto *tp,
 			   struct tcf_result *res)
@@ -142,11 +147,8 @@
 	struct dst_entry *dst;
 	struct route4_bucket *b;
 	struct route4_filter *f;
-#ifdef CONFIG_NET_CLS_POLICE
-	int dont_cache = 0;
-#endif
 	u32 id, h;
-	int iif;
+	int iif, dont_cache = 0;
 
 	if ((dst = skb->dst) == NULL)
 		goto failure;
@@ -172,29 +174,16 @@
 
 restart:
 	if ((b = head->table[h]) != NULL) {
-		f = b->ht[route4_hash_from(id)];
-
-		for ( ; f; f = f->next) {
-			if (f->id == id) {
-				*res = f->res;
-				IF_ROUTE_POLICE route4_set_fastmap(head, id, iif, f);
-				return 0;
-			}
-		}
-
-		for (f = b->ht[route4_hash_iif(iif)]; f; f = f->next) {
-			if (f->iif == iif) {
-				*res = f->res;
-				IF_ROUTE_POLICE route4_set_fastmap(head, id, iif, f);
-				return 0;
-			}
-		}
+		for (f = b->ht[route4_hash_from(id)]; f; f = f->next)
+			if (f->id == id)
+				ROUTE4_APPLY_RESULT();
+
+		for (f = b->ht[route4_hash_iif(iif)]; f; f = f->next)
+			if (f->iif == iif)
+				ROUTE4_APPLY_RESULT();
 
-		for (f = b->ht[route4_hash_wild()]; f; f = f->next) {
-			*res = f->res;
-			IF_ROUTE_POLICE route4_set_fastmap(head, id, iif, f);
-			return 0;
-		}
+		for (f = b->ht[route4_hash_wild()]; f; f = f->next)
+			ROUTE4_APPLY_RESULT();
 
 	}
 	if (h < 256) {
@@ -203,9 +192,7 @@
 		goto restart;
 	}
 
-#ifdef CONFIG_NET_CLS_POLICE
 	if (!dont_cache)
-#endif
 		route4_set_fastmap(head, id, iif, ROUTE4_FAILURE);
 failure:
 	return -1;
@@ -220,7 +207,7 @@
 	return -1;
 }
 
-static u32 to_hash(u32 id)
+static inline u32 to_hash(u32 id)
 {
 	u32 h = id&0xFF;
 	if (id&0x8000)
@@ -228,7 +215,7 @@
 	return h;
 }
 
-static u32 from_hash(u32 id)
+static inline u32 from_hash(u32 id)
 {
 	id &= 0xFFFF;
 	if (id == 0xFFFF)
@@ -276,6 +263,14 @@
 	return 0;
 }
 
+static inline void
+route4_delete_filter(struct tcf_proto *tp, struct route4_filter *f)
+{
+	tcf_unbind_filter(tp, &f->res);
+	tcf_exts_destroy(tp, &f->exts);
+	kfree(f);
+}
+
 static void route4_destroy(struct tcf_proto *tp)
 {
 	struct route4_head *head = xchg(&tp->root, NULL);
@@ -293,11 +288,7 @@
 
 				while ((f = b->ht[h2]) != NULL) {
 					b->ht[h2] = f->next;
-					tcf_unbind_filter(tp, &f->res);
-#ifdef CONFIG_NET_CLS_POLICE
-					tcf_police_release(f->police,TCA_ACT_UNBIND);
-#endif
-					kfree(f);
+					route4_delete_filter(tp, f);
 				}
 			}
 			kfree(b);
@@ -327,11 +318,7 @@
 			tcf_tree_unlock(tp);
 
 			route4_reset_fastmap(tp->q->dev, head, f->id);
-			tcf_unbind_filter(tp, &f->res);
-#ifdef CONFIG_NET_CLS_POLICE
-			tcf_police_release(f->police,TCA_ACT_UNBIND);
-#endif
-			kfree(f);
+			route4_delete_filter(tp, f);
 
 			/* Strip tree */
 
@@ -351,108 +338,63 @@
 	return 0;
 }
 
-static int route4_change(struct tcf_proto *tp, unsigned long base,
-		       u32 handle,
-		       struct rtattr **tca,
-		       unsigned long *arg)
+static int route4_set_parms(struct tcf_proto *tp, unsigned long base,
+	struct route4_filter *f, u32 handle, struct route4_head *head,
+	struct rtattr **tb, struct rtattr *est, int new)
 {
-	struct route4_head *head = tp->root;
-	struct route4_filter *f, *f1, **ins_f;
-	struct route4_bucket *b;
-	struct rtattr *opt = tca[TCA_OPTIONS-1];
-	struct rtattr *tb[TCA_ROUTE4_MAX];
-	unsigned h1, h2;
 	int err;
+	u32 id = 0, to = 0, nhandle = 0x8000;
+	struct route4_filter *fp;
+	unsigned int h1;
+	struct route4_bucket *b;
+	struct tcf_exts e;
 
-	if (opt == NULL)
-		return handle ? -EINVAL : 0;
-
-	if (rtattr_parse(tb, TCA_ROUTE4_MAX, RTA_DATA(opt), RTA_PAYLOAD(opt)) < 0)
-		return -EINVAL;
-
-	if ((f = (struct route4_filter*)*arg) != NULL) {
-		if (f->handle != handle && handle)
-			return -EINVAL;
-		if (tb[TCA_ROUTE4_CLASSID-1]) {
-			f->res.classid = *(u32*)RTA_DATA(tb[TCA_ROUTE4_CLASSID-1]);
-			tcf_bind_filter(tp, &f->res, base);
-		}
-#ifdef CONFIG_NET_CLS_POLICE
-		if (tb[TCA_ROUTE4_POLICE-1]) {
-			err = tcf_change_police(tp, &f->police,
-				tb[TCA_ROUTE4_POLICE-1], tca[TCA_RATE-1]);
-			if (err < 0)
-				return err;
-		}
-#endif
-		return 0;
-	}
-
-	/* Now more serious part... */
-
-	if (head == NULL) {
-		head = kmalloc(sizeof(struct route4_head), GFP_KERNEL);
-		if (head == NULL)
-			return -ENOBUFS;
-		memset(head, 0, sizeof(struct route4_head));
-
-		tcf_tree_lock(tp);
-		tp->root = head;
-		tcf_tree_unlock(tp);
-	}
-
-	f = kmalloc(sizeof(struct route4_filter), GFP_KERNEL);
-	if (f == NULL)
-		return -ENOBUFS;
-
-	memset(f, 0, sizeof(*f));
+	err = tcf_exts_validate(tp, tb, est, &e, &route_ext_map);
+	if (err < 0)
+		return err;
 
 	err = -EINVAL;
-	f->handle = 0x8000;
+	if (tb[TCA_ROUTE4_CLASSID-1])
+		if (RTA_PAYLOAD(tb[TCA_ROUTE4_CLASSID-1]) < sizeof(u32))
+			goto errout;
+
 	if (tb[TCA_ROUTE4_TO-1]) {
-		if (handle&0x8000)
+		if (new && handle & 0x8000)
 			goto errout;
-		if (RTA_PAYLOAD(tb[TCA_ROUTE4_TO-1]) < 4)
+		if (RTA_PAYLOAD(tb[TCA_ROUTE4_TO-1]) < sizeof(u32))
 			goto errout;
-		f->id = *(u32*)RTA_DATA(tb[TCA_ROUTE4_TO-1]);
-		if (f->id > 0xFF)
+		to = *(u32*)RTA_DATA(tb[TCA_ROUTE4_TO-1]);
+		if (to > 0xFF)
 			goto errout;
-		f->handle = f->id;
+		nhandle = to;
 	}
+
 	if (tb[TCA_ROUTE4_FROM-1]) {
-		u32 sid;
 		if (tb[TCA_ROUTE4_IIF-1])
 			goto errout;
-		if (RTA_PAYLOAD(tb[TCA_ROUTE4_FROM-1]) < 4)
+		if (RTA_PAYLOAD(tb[TCA_ROUTE4_FROM-1]) < sizeof(u32))
 			goto errout;
-		sid = (*(u32*)RTA_DATA(tb[TCA_ROUTE4_FROM-1]));
-		if (sid > 0xFF)
+		id = *(u32*)RTA_DATA(tb[TCA_ROUTE4_FROM-1]);
+		if (id > 0xFF)
 			goto errout;
-		f->handle |= sid<<16;
-		f->id |= sid<<16;
+		nhandle |= id << 16;
 	} else if (tb[TCA_ROUTE4_IIF-1]) {
-		if (RTA_PAYLOAD(tb[TCA_ROUTE4_IIF-1]) < 4)
+		if (RTA_PAYLOAD(tb[TCA_ROUTE4_IIF-1]) < sizeof(u32))
 			goto errout;
-		f->iif = *(u32*)RTA_DATA(tb[TCA_ROUTE4_IIF-1]);
-		if (f->iif > 0x7FFF)
+		id = *(u32*)RTA_DATA(tb[TCA_ROUTE4_IIF-1]);
+		if (id > 0x7FFF)
 			goto errout;
-		f->handle |= (f->iif|0x8000)<<16;
+		nhandle = (id | 0x8000) << 16;
 	} else
-		f->handle |= 0xFFFF<<16;
+		nhandle = 0xFFFF << 16;
 
-	if (handle) {
-		f->handle |= handle&0x7F00;
-		if (f->handle != handle)
+	if (handle && new) {
+		nhandle |= handle & 0x7F00;
+		if (nhandle != handle)
 			goto errout;
 	}
 
-	if (tb[TCA_ROUTE4_CLASSID-1]) {
-		if (RTA_PAYLOAD(tb[TCA_ROUTE4_CLASSID-1]) < 4)
-			goto errout;
-		f->res.classid = *(u32*)RTA_DATA(tb[TCA_ROUTE4_CLASSID-1]);
-	}
-
-	h1 = to_hash(f->handle);
+	h1 = to_hash(nhandle);
 	if ((b = head->table[h1]) == NULL) {
 		err = -ENOBUFS;
 		b = kmalloc(sizeof(struct route4_bucket), GFP_KERNEL);
@@ -463,27 +405,119 @@
 		tcf_tree_lock(tp);
 		head->table[h1] = b;
 		tcf_tree_unlock(tp);
+	} else {
+		unsigned int h2 = from_hash(nhandle >> 16);
+		err = -EEXIST;
+		for (fp = b->ht[h2]; fp; fp = fp->next)
+			if (fp->handle == f->handle)
+				goto errout;
 	}
+
+	tcf_tree_lock(tp);
+	if (tb[TCA_ROUTE4_TO-1])
+		f->id = to;
+
+	if (tb[TCA_ROUTE4_FROM-1])
+		f->id = to | id<<16;
+	else if (tb[TCA_ROUTE4_IIF-1])
+		f->iif = id;
+
+	f->handle = nhandle;
 	f->bkt = b;
+	tcf_tree_unlock(tp);
 
-	err = -EEXIST;
-	h2 = from_hash(f->handle>>16);
-	for (ins_f = &b->ht[h2]; (f1=*ins_f) != NULL; ins_f = &f1->next) {
-		if (f->handle < f1->handle)
-			break;
-		if (f1->handle == f->handle)
+	if (tb[TCA_ROUTE4_CLASSID-1]) {
+		f->res.classid = *(u32*)RTA_DATA(tb[TCA_ROUTE4_CLASSID-1]);
+		tcf_bind_filter(tp, &f->res, base);
+	}
+
+	tcf_exts_change(tp, &f->exts, &e);
+
+	return 0;
+errout:
+	tcf_exts_destroy(tp, &e);
+	return err;
+}
+
+static int route4_change(struct tcf_proto *tp, unsigned long base,
+		       u32 handle,
+		       struct rtattr **tca,
+		       unsigned long *arg)
+{
+	struct route4_head *head = tp->root;
+	struct route4_filter *f, *f1, **fp;
+	struct route4_bucket *b;
+	struct rtattr *opt = tca[TCA_OPTIONS-1];
+	struct rtattr *tb[TCA_ROUTE4_MAX];
+	unsigned int h, th;
+	u32 old_handle = 0;
+	int err;
+
+	if (opt == NULL)
+		return handle ? -EINVAL : 0;
+
+	if (rtattr_parse_nested(tb, TCA_ROUTE4_MAX, opt) < 0)
+		return -EINVAL;
+
+	if ((f = (struct route4_filter*)*arg) != NULL) {
+		if (f->handle != handle && handle)
+			return -EINVAL;
+
+		if (f->bkt)
+			old_handle = f->handle;
+
+		err = route4_set_parms(tp, base, f, handle, head, tb,
+			tca[TCA_RATE-1], 0);
+		if (err < 0)
+			return err;
+
+		goto reinsert;
+	}
+
+	err = -ENOBUFS;
+	if (head == NULL) {
+		head = kmalloc(sizeof(struct route4_head), GFP_KERNEL);
+		if (head == NULL)
 			goto errout;
+		memset(head, 0, sizeof(struct route4_head));
+
+		tcf_tree_lock(tp);
+		tp->root = head;
+		tcf_tree_unlock(tp);
 	}
 
-	tcf_bind_filter(tp, &f->res, base);
-#ifdef CONFIG_NET_CLS_POLICE
-	if (tb[TCA_ROUTE4_POLICE-1])
-		tcf_change_police(tp, &f->police, tb[TCA_ROUTE4_POLICE-1], tca[TCA_RATE-1]);
-#endif
+	f = kmalloc(sizeof(struct route4_filter), GFP_KERNEL);
+	if (f == NULL)
+		goto errout;
+	memset(f, 0, sizeof(*f));
+
+	err = route4_set_parms(tp, base, f, handle, head, tb,
+		tca[TCA_RATE-1], 1);
+	if (err < 0)
+		goto errout;
+
+reinsert:
+	h = from_hash(f->handle >> 16);
+	for (fp = &f->bkt->ht[h]; (f1=*fp) != NULL; fp = &f1->next)
+		if (f->handle < f1->handle)
+			break;
 
 	f->next = f1;
 	tcf_tree_lock(tp);
-	*ins_f = f;
+	*fp = f;
+
+	if (old_handle && f->handle != old_handle) {
+		th = to_hash(old_handle);
+		h = from_hash(old_handle >> 16);
+		if ((b = head->table[th]) != NULL) {
+			for (fp = &b->ht[h]; *fp; fp = &(*fp)->next) {
+				if (*fp == f) {
+					*fp = f->next;
+					break;
+				}
+			}
+		}
+	}
 	tcf_tree_unlock(tp);
 
 	route4_reset_fastmap(tp->q->dev, head, f->id);
@@ -559,17 +593,15 @@
 	}
 	if (f->res.classid)
 		RTA_PUT(skb, TCA_ROUTE4_CLASSID, 4, &f->res.classid);
-#ifdef CONFIG_NET_CLS_POLICE
-	if (tcf_dump_police(skb, f->police, TCA_ROUTE4_POLICE) < 0)
+
+	if (tcf_exts_dump(skb, &f->exts, &route_ext_map) < 0)
 		goto rtattr_failure;
-#endif
 
 	rta->rta_len = skb->tail - b;
-#ifdef CONFIG_NET_CLS_POLICE
-	if (f->police)
-		if (tcf_police_dump_stats(skb, f->police) < 0)
-			goto rtattr_failure;
-#endif
+
+	if (tcf_exts_dump_stats(skb, &f->exts, &route_ext_map) < 0)
+		goto rtattr_failure;
+
 	return skb->len;
 
 rtattr_failure:

[PATCH 6/9] PKT_SCHED: tcindex: allow changing parameters for existing filters and use tcf_exts API

[Thomas Graf]

Transforms tcindex to use tcf_exts API and thus adds support for
actions. Replaces the existing change implementation with a new one
supporting changes for existing filters which allows to change a
classifier without letting a single packet pass by unclassified.

Fixes various cases where a error is returned but the filter was
changed already.

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

--- linux-2.6.10-bk1.orig/include/linux/pkt_cls.h	2004-12-27 22:28:09.000000000 +0100
+++ linux-2.6.10-bk1/include/linux/pkt_cls.h	2004-12-27 22:28:30.000000000 +0100
@@ -312,6 +312,7 @@
 	TCA_TCINDEX_FALL_THROUGH,
 	TCA_TCINDEX_CLASSID,
 	TCA_TCINDEX_POLICE,
+	TCA_TCINDEX_ACT,
 	__TCA_TCINDEX_MAX
 };
 
--- linux-2.6.10-bk2.orig/net/sched/cls_tcindex.c	2004-12-29 20:41:32.000000000 +0100
+++ linux-2.6.10-bk2/net/sched/cls_tcindex.c	2004-12-29 22:52:46.000000000 +0100
@@ -49,12 +49,12 @@
 
 
 struct tcindex_filter_result {
-	struct tcf_police *police;
-	struct tcf_result res;
+	struct tcf_exts		exts;
+	struct tcf_result	res;
 };
 
 struct tcindex_filter {
-	__u16 key;
+	u16 key;
 	struct tcindex_filter_result result;
 	struct tcindex_filter *next;
 };
@@ -64,60 +64,64 @@
 	struct tcindex_filter_result *perfect; /* perfect hash; NULL if none */
 	struct tcindex_filter **h; /* imperfect hash; only used if !perfect;
 				      NULL if unused */
-	__u16 mask;		/* AND key with mask */
+	u16 mask;		/* AND key with mask */
 	int shift;		/* shift ANDed key to the right */
 	int hash;		/* hash table size; 0 if undefined */
 	int alloc_hash;		/* allocated size */
 	int fall_through;	/* 0: only classify if explicit match */
 };
 
+static struct tcf_ext_map tcindex_ext_map = {
+	.police = TCA_TCINDEX_POLICE,
+	.action = TCA_TCINDEX_ACT
+};
+
+static inline int
+tcindex_filter_is_set(struct tcindex_filter_result *r)
+{
+	return tcf_exts_is_predicative(&r->exts) || r->res.classid;
+}
 
-static struct tcindex_filter_result *lookup(struct tcindex_data *p,__u16 key)
+static struct tcindex_filter_result *
+tcindex_lookup(struct tcindex_data *p, u16 key)
 {
 	struct tcindex_filter *f;
 
 	if (p->perfect)
-		return p->perfect[key].res.class ? p->perfect+key : NULL;
-	if (!p->h)
-		return NULL;
-	for (f = p->h[key % p->hash]; f; f = f->next) {
-		if (f->key == key)
-			return &f->result;
+		return tcindex_filter_is_set(p->perfect + key) ?
+			p->perfect + key : NULL;
+	else if (p->h) {
+		for (f = p->h[key % p->hash]; f; f = f->next)
+			if (f->key == key)
+				return &f->result;
 	}
+
 	return NULL;
 }
 
 
 static int tcindex_classify(struct sk_buff *skb, struct tcf_proto *tp,
-			  struct tcf_result *res)
+			    struct tcf_result *res)
 {
 	struct tcindex_data *p = PRIV(tp);
 	struct tcindex_filter_result *f;
+	int key = (skb->tc_index & p->mask) >> p->shift;
 
 	D2PRINTK("tcindex_classify(skb %p,tp %p,res %p),p %p\n",skb,tp,res,p);
 
-	f = lookup(p,(skb->tc_index & p->mask) >> p->shift);
+	f = tcindex_lookup(p, key);
 	if (!f) {
 		if (!p->fall_through)
 			return -1;
-		res->classid = TC_H_MAKE(TC_H_MAJ(tp->q->handle),
-		    (skb->tc_index& p->mask) >> p->shift);
+		res->classid = TC_H_MAKE(TC_H_MAJ(tp->q->handle), key);
 		res->class = 0;
 		D2PRINTK("alg 0x%x\n",res->classid);
 		return 0;
 	}
 	*res = f->res;
 	D2PRINTK("map 0x%x\n",res->classid);
-#ifdef CONFIG_NET_CLS_POLICE
-	if (f->police) {
-		int result;
-
-		result = tcf_police(skb,f->police);
-		D2PRINTK("police %d\n",res);
-		return result;
-	}
-#endif
-	return 0;
+
+	return tcf_exts_exec(skb, &f->exts, res);
 }
 
 
@@ -129,8 +133,8 @@
 	DPRINTK("tcindex_get(tp %p,handle 0x%08x)\n",tp,handle);
 	if (p->perfect && handle >= p->alloc_hash)
 		return 0;
-	r = lookup(PRIV(tp),handle);
-	return r && r->res.class ? (unsigned long) r : 0;
+	r = tcindex_lookup(p, handle);
+	return r && tcindex_filter_is_set(r) ? (unsigned long) r : 0UL;
 }
 
 
@@ -149,13 +153,12 @@
 	if (!p)
 		return -ENOMEM;
 
-	tp->root = p;
-	p->perfect = NULL;
-	p->h = NULL;
-	p->hash = 0;
+	memset(p, 0, sizeof(*p));
 	p->mask = 0xffff;
-	p->shift = 0;
+	p->hash = DEFAULT_HASH_SIZE;
 	p->fall_through = 1;
+
+	tp->root = p;
 	return 0;
 }
 
@@ -190,9 +193,7 @@
 			tcf_tree_unlock(tp);
 	}
 	tcf_unbind_filter(tp, &r->res);
-#ifdef CONFIG_NET_CLS_POLICE
-	tcf_police_release(r->police, TCA_ACT_UNBIND);
-#endif
+	tcf_exts_destroy(tp, &r->exts);
 	if (f)
 		kfree(f);
 	return 0;
@@ -203,148 +204,184 @@
 	return __tcindex_delete(tp, arg, 1);
 }
 
-/*
- * There are no parameters for tcindex_init, so we overload tcindex_change
- */
+static inline int
+valid_perfect_hash(struct tcindex_data *p)
+{
+	return  p->hash > (p->mask >> p->shift);
+}
+
+static int
+tcindex_set_parms(struct tcf_proto *tp, unsigned long base, u32 handle,
+		  struct tcindex_data *p, struct tcindex_filter_result *r,
+		  struct rtattr **tb, struct rtattr *est)
+{
+	int err, balloc = 0;
+	struct tcindex_filter_result new_filter_result, *old_r = r;
+	struct tcindex_filter_result cr;
+	struct tcindex_data cp;
+	struct tcindex_filter *f = NULL; /* make gcc behave */
+	struct tcf_exts e;
+
+	err = tcf_exts_validate(tp, tb, est, &e, &tcindex_ext_map);
+	if (err < 0)
+		return err;
+	
+	memcpy(&cp, p, sizeof(cp));
+	memset(&new_filter_result, 0, sizeof(new_filter_result));
+
+	if (old_r)
+		memcpy(&cr, r, sizeof(cr));
+	else
+		memset(&cr, 0, sizeof(cr));
+
+	err = -EINVAL;
+	if (tb[TCA_TCINDEX_HASH-1]) {
+		if (RTA_PAYLOAD(tb[TCA_TCINDEX_HASH-1]) < sizeof(u32))
+			goto errout;
+		cp.hash = *(u32 *) RTA_DATA(tb[TCA_TCINDEX_HASH-1]);
+	}
+
+	if (tb[TCA_TCINDEX_MASK-1]) {
+		if (RTA_PAYLOAD(tb[TCA_TCINDEX_MASK-1]) < sizeof(u16))
+			goto errout;
+		cp.mask = *(u16 *) RTA_DATA(tb[TCA_TCINDEX_MASK-1]);
+	}
+
+	if (tb[TCA_TCINDEX_SHIFT-1]) {
+		if (RTA_PAYLOAD(tb[TCA_TCINDEX_SHIFT-1]) < sizeof(u16))
+			goto errout;
+		cp.shift = *(u16 *) RTA_DATA(tb[TCA_TCINDEX_SHIFT-1]);
+	}
+
+	err = -EBUSY;
+	/* Hash already allocated, make sure that we still meet the
+	 * requirements for the allocated hash.
+	 */
+	if (cp.perfect) {
+		if (!valid_perfect_hash(&cp) ||
+		    cp.hash > cp.alloc_hash)
+			goto errout;
+	} else if (cp.h && cp.hash != cp.alloc_hash)
+		goto errout;
+
+	err = -EINVAL;
+	if (tb[TCA_TCINDEX_FALL_THROUGH-1]) {
+		if (RTA_PAYLOAD(tb[TCA_TCINDEX_FALL_THROUGH-1]) < sizeof(u32))
+			goto errout;
+		cp.fall_through =
+			*(u32 *) RTA_DATA(tb[TCA_TCINDEX_FALL_THROUGH-1]);
+	}
+
+	if (!cp.hash) {
+		/* Hash not specified, use perfect hash if the upper limit
+		 * of the hashing index is below the threshold.
+		 */
+		if ((cp.mask >> cp.shift) < PERFECT_HASH_THRESHOLD)
+			cp.hash = (cp.mask >> cp.shift)+1;
+		else
+			cp.hash = DEFAULT_HASH_SIZE;
+	}
+
+	if (!cp.perfect && !cp.h)
+		cp.alloc_hash = cp.hash;
+
+	/* Note: this could be as restrictive as if (handle & ~(mask >> shift))
+	 * but then, we'd fail handles that may become valid after some future
+	 * mask change. While this is extremely unlikely to ever matter,
+	 * the check below is safer (and also more backwards-compatible).
+	 */
+	if (cp.perfect || valid_perfect_hash(&cp))
+		if (handle >= cp.alloc_hash)
+			goto errout;
+
+
+	err = -ENOMEM;
+	if (!cp.perfect && !cp.h) {
+		if (valid_perfect_hash(&cp)) {
+			cp.perfect = kmalloc(cp.hash * sizeof(*r), GFP_KERNEL);
+			if (!cp.perfect)
+				goto errout;
+			memset(cp.perfect, 0, cp.hash * sizeof(*r));
+			balloc = 1;
+		} else {
+			cp.h = kmalloc(cp.hash * sizeof(f), GFP_KERNEL);
+			if (!cp.h)
+				goto errout;
+			memset(cp.h, 0, cp.hash * sizeof(f));
+			balloc = 2;
+		}
+	}
+
+	if (cp.perfect)
+		r = cp.perfect + handle;
+	else
+		r = tcindex_lookup(&cp, handle) ? : &new_filter_result;
+
+	if (r == &new_filter_result) {
+		f = kmalloc(sizeof(*f), GFP_KERNEL);
+		if (!f)
+			goto errout_alloc;
+		memset(f, 0, sizeof(*f));
+ 	}
+
+	if (tb[TCA_TCINDEX_CLASSID-1]) {
+		cr.res.classid = *(u32 *) RTA_DATA(tb[TCA_TCINDEX_CLASSID-1]);
+		tcf_bind_filter(tp, &cr.res, base);
+ 	}
+
+	tcf_exts_change(tp, &cr.exts, &e);
+
+	tcf_tree_lock(tp);
+	if (old_r && old_r != r)
+		memset(old_r, 0, sizeof(*old_r));
+
+	memcpy(p, &cp, sizeof(cp));
+	memcpy(r, &cr, sizeof(cr));
+
+	if (r == &new_filter_result) {
+		struct tcindex_filter **fp;
+
+		f->key = handle;
+		f->result = new_filter_result;
+		f->next = NULL;
+		for (fp = p->h+(handle % p->hash); *fp; fp = &(*fp)->next)
+			/* nothing */;
+		*fp = f;
+ 	}
+	tcf_tree_unlock(tp);
+
+	return 0;
 
+errout_alloc:
+	if (balloc == 1)
+		kfree(cp.perfect);
+	else if (balloc == 2)
+		kfree(cp.h);
+errout:
+	tcf_exts_destroy(tp, &e);
+	return err;
+}
 
-static int tcindex_change(struct tcf_proto *tp,unsigned long base,u32 handle,
-    struct rtattr **tca,unsigned long *arg)
-{
-	struct tcindex_filter_result new_filter_result = {
-		NULL,		/* no policing */
-		{ 0,0 },	/* no classification */
-	};
+static int
+tcindex_change(struct tcf_proto *tp, unsigned long base, u32 handle,
+	       struct rtattr **tca, unsigned long *arg)
+{
 	struct rtattr *opt = tca[TCA_OPTIONS-1];
 	struct rtattr *tb[TCA_TCINDEX_MAX];
 	struct tcindex_data *p = PRIV(tp);
-	struct tcindex_filter *f;
 	struct tcindex_filter_result *r = (struct tcindex_filter_result *) *arg;
-	struct tcindex_filter **walk;
-	int hash,shift;
-	__u16 mask;
 
 	DPRINTK("tcindex_change(tp %p,handle 0x%08x,tca %p,arg %p),opt %p,"
-	    "p %p,r %p\n",tp,handle,tca,arg,opt,p,r);
-	if (arg)
-		DPRINTK("*arg = 0x%lx\n",*arg);
+	    "p %p,r %p,*arg 0x%lx\n",
+	    tp, handle, tca, arg, opt, p, r, arg ? *arg : 0L);
+
 	if (!opt)
 		return 0;
-	if (rtattr_parse(tb,TCA_TCINDEX_MAX,RTA_DATA(opt),RTA_PAYLOAD(opt)) < 0)
-		return -EINVAL;
-	if (!tb[TCA_TCINDEX_HASH-1]) {
-		hash = p->hash;
-	} else {
-		if (RTA_PAYLOAD(tb[TCA_TCINDEX_HASH-1]) < sizeof(int))
-			return -EINVAL;
-		hash = *(int *) RTA_DATA(tb[TCA_TCINDEX_HASH-1]);
-	}
-	if (!tb[TCA_TCINDEX_MASK-1]) {
-		mask = p->mask;
-	} else {
-		if (RTA_PAYLOAD(tb[TCA_TCINDEX_MASK-1]) < sizeof(__u16))
-			return -EINVAL;
-		mask = *(__u16 *) RTA_DATA(tb[TCA_TCINDEX_MASK-1]);
-	}
-	if (!tb[TCA_TCINDEX_SHIFT-1])
-		shift = p->shift;
-	else {
-		if (RTA_PAYLOAD(tb[TCA_TCINDEX_SHIFT-1]) < sizeof(__u16))
-			return -EINVAL;
-		shift = *(int *) RTA_DATA(tb[TCA_TCINDEX_SHIFT-1]);
-	}
-	if (p->perfect && hash <= (mask >> shift))
-		return -EBUSY;
-	if (p->perfect && hash > p->alloc_hash)
-		return -EBUSY;
-	if (p->h && hash != p->alloc_hash)
-		return -EBUSY;
-	p->hash = hash;
-	p->mask = mask;
-	p->shift = shift;
-	if (tb[TCA_TCINDEX_FALL_THROUGH-1]) {
-		if (RTA_PAYLOAD(tb[TCA_TCINDEX_FALL_THROUGH-1]) < sizeof(int))
-			return -EINVAL;
-		p->fall_through =
-		    *(int *) RTA_DATA(tb[TCA_TCINDEX_FALL_THROUGH-1]);
-	}
-	DPRINTK("classid/police %p/%p\n",tb[TCA_TCINDEX_CLASSID-1],
-	    tb[TCA_TCINDEX_POLICE-1]);
-	if (!tb[TCA_TCINDEX_CLASSID-1] && !tb[TCA_TCINDEX_POLICE-1])
-		return 0;
-	if (!hash) {
-		if ((mask >> shift) < PERFECT_HASH_THRESHOLD) {
-			p->hash = (mask >> shift)+1;
-		} else {
-			p->hash = DEFAULT_HASH_SIZE;
-		}
-	}
-	if (!p->perfect && !p->h) {
-		p->alloc_hash = p->hash;
-		DPRINTK("hash %d mask %d\n",p->hash,p->mask);
-		if (p->hash > (mask >> shift)) {
-			p->perfect = kmalloc(p->hash*
-			    sizeof(struct tcindex_filter_result),GFP_KERNEL);
-			if (!p->perfect)
-				return -ENOMEM;
-			memset(p->perfect, 0,
-			       p->hash * sizeof(struct tcindex_filter_result));
-		} else {
-			p->h = kmalloc(p->hash*sizeof(struct tcindex_filter *),
-			    GFP_KERNEL);
-			if (!p->h)
-				return -ENOMEM;
-			memset(p->h, 0, p->hash*sizeof(struct tcindex_filter *));
-		}
-	}
-	/*
-	 * Note: this could be as restrictive as
-	 * if (handle & ~(mask >> shift))
-	 * but then, we'd fail handles that may become valid after some
-	 * future mask change. While this is extremely unlikely to ever
-	 * matter, the check below is safer (and also more
-	 * backwards-compatible).
-	 */
-	if (p->perfect && handle >= p->alloc_hash)
+
+	if (rtattr_parse_nested(tb, TCA_TCINDEX_MAX, opt) < 0)
 		return -EINVAL;
-	if (p->perfect) {
-		r = p->perfect+handle;
-	} else {
-		r = lookup(p,handle);
-		DPRINTK("r=%p\n",r);
-		if (!r)
-			r = &new_filter_result;
-	}
-	DPRINTK("r=%p\n",r);
-	if (tb[TCA_TCINDEX_CLASSID-1]) {
-		r->res.classid = *(__u32 *) RTA_DATA(tb[TCA_TCINDEX_CLASSID-1]);
-		tcf_bind_filter(tp, &r->res, base);
 
-		if (!r->res.class) {
-			r->res.classid = 0;
-			return -ENOENT;
-		}
-	}
-#ifdef CONFIG_NET_CLS_POLICE
-	if (tb[TCA_TCINDEX_POLICE-1]) {
-		int err = tcf_change_police(tp, &r->police, tb[TCA_TCINDEX_POLICE-1], NULL);
-		if (err < 0)
-			return err;
-	}
-#endif
-	if (r != &new_filter_result)
-		return 0;
-	f = kmalloc(sizeof(struct tcindex_filter),GFP_KERNEL);
-	if (!f)
-		return -ENOMEM;
-	f->key = handle;
-	f->result = new_filter_result;
-	f->next = NULL;
-	for (walk = p->h+(handle % p->hash); *walk; walk = &(*walk)->next)
-		/* nothing */;
-	wmb();
-	*walk = f;
-	return 0;
+	return tcindex_set_parms(tp, base, handle, p, r, tb, tca[TCA_RATE-1]);
 }
 
 
@@ -434,6 +471,7 @@
 		RTA_PUT(skb,TCA_TCINDEX_SHIFT,sizeof(p->shift),&p->shift);
 		RTA_PUT(skb,TCA_TCINDEX_FALL_THROUGH,sizeof(p->fall_through),
 		    &p->fall_through);
+		rta->rta_len = skb->tail-b;
 	} else {
 		if (p->perfect) {
 			t->tcm_handle = r-p->perfect;
@@ -453,12 +491,15 @@
 		DPRINTK("handle = %d\n",t->tcm_handle);
 		if (r->res.class)
 			RTA_PUT(skb, TCA_TCINDEX_CLASSID, 4, &r->res.classid);
-#ifdef CONFIG_NET_CLS_POLICE
-		if (tcf_dump_police(skb, r->police, TCA_TCINDEX_POLICE) < 0)
+
+		if (tcf_exts_dump(skb, &r->exts, &tcindex_ext_map) < 0)
+			goto rtattr_failure;
+		rta->rta_len = skb->tail-b;
+
+		if (tcf_exts_dump_stats(skb, &r->exts, &tcindex_ext_map) < 0)
 			goto rtattr_failure;
-#endif
 	}
-	rta->rta_len = skb->tail-b;
+	
 	return skb->len;
 
 rtattr_failure:

[PATCH 7/9] PKT_SCHED: rsvp: use tcf_exts API

[Thomas Graf]

Transforms tcindex to use tcf_exts API and thus adds support for
actions. Needs more work to allow changing parameters for
existing filters.

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

--- linux-2.6.10-bk1.orig/include/linux/pkt_cls.h	2004-12-27 22:38:14.000000000 +0100
+++ linux-2.6.10-bk1/include/linux/pkt_cls.h	2004-12-27 22:39:29.000000000 +0100
@@ -249,6 +249,7 @@
 	TCA_RSVP_SRC,
 	TCA_RSVP_PINFO,
 	TCA_RSVP_POLICE,
+	TCA_RSVP_ACT,
 	__TCA_RSVP_MAX
 };
 
--- linux-2.6.10-bk2.orig/net/sched/cls_rsvp.h	2004-12-29 20:16:24.000000000 +0100
+++ linux-2.6.10-bk2/net/sched/cls_rsvp.h	2004-12-29 20:24:09.000000000 +0100
@@ -95,9 +95,7 @@
 	u8			tunnelhdr;
 
 	struct tcf_result	res;
-#ifdef CONFIG_NET_CLS_POLICE
-	struct tcf_police	*police;
-#endif
+	struct tcf_exts		exts;
 
 	u32			handle;
 	struct rsvp_session	*sess;
@@ -120,18 +118,20 @@
 	return h & 0xF;
 }
 
-#ifdef CONFIG_NET_CLS_POLICE
-#define RSVP_POLICE() \
-if (f->police) { \
-	int pol_res = tcf_police(skb, f->police); \
-	if (pol_res < 0) continue; \
-	if (pol_res) return pol_res; \
-}
-#else
-#define RSVP_POLICE()
-#endif
-
+static struct tcf_ext_map rsvp_ext_map = {
+	.police = TCA_RSVP_POLICE,
+	.action = TCA_RSVP_ACT
+};
 
+#define RSVP_APPLY_RESULT()					\
+	do {							\
+		int r = tcf_exts_exec(skb, &f->exts, res);	\
+		if (r < 0)					\
+			continue;				\
+		else if (r > 0)					\
+			return r;				\
+	} while(0)
+	
 static int rsvp_classify(struct sk_buff *skb, struct tcf_proto *tp,
 			 struct tcf_result *res)
 {
@@ -189,8 +189,7 @@
 #endif
 				    ) {
 					*res = f->res;
-
-					RSVP_POLICE();
+					RSVP_APPLY_RESULT();
 
 matched:
 					if (f->tunnelhdr == 0)
@@ -205,7 +204,7 @@
 			/* And wildcard bucket... */
 			for (f = s->ht[16]; f; f = f->next) {
 				*res = f->res;
-				RSVP_POLICE();
+				RSVP_APPLY_RESULT();
 				goto matched;
 			}
 			return -1;
@@ -251,6 +250,14 @@
 	return -ENOBUFS;
 }
 
+static inline void
+rsvp_delete_filter(struct tcf_proto *tp, struct rsvp_filter *f)
+{
+	tcf_unbind_filter(tp, &f->res);
+	tcf_exts_destroy(tp, &f->exts);
+	kfree(f);
+}
+
 static void rsvp_destroy(struct tcf_proto *tp)
 {
 	struct rsvp_head *data = xchg(&tp->root, NULL);
@@ -273,11 +280,7 @@
 
 				while ((f = s->ht[h2]) != NULL) {
 					s->ht[h2] = f->next;
-					tcf_unbind_filter(tp, &f->res);
-#ifdef CONFIG_NET_CLS_POLICE
-					tcf_police_release(f->police,TCA_ACT_UNBIND);
-#endif
-					kfree(f);
+					rsvp_delete_filter(tp, f);
 				}
 			}
 			kfree(s);
@@ -299,12 +302,7 @@
 			tcf_tree_lock(tp);
 			*fp = f->next;
 			tcf_tree_unlock(tp);
-			tcf_unbind_filter(tp, &f->res);
-#ifdef CONFIG_NET_CLS_POLICE
-			tcf_police_release(f->police,TCA_ACT_UNBIND);
-#endif
-
-			kfree(f);
+			rsvp_delete_filter(tp, f);
 
 			/* Strip tree */
 
@@ -412,6 +410,7 @@
 	struct tc_rsvp_pinfo *pinfo = NULL;
 	struct rtattr *opt = tca[TCA_OPTIONS-1];
 	struct rtattr *tb[TCA_RSVP_MAX];
+	struct tcf_exts e;
 	unsigned h1, h2;
 	u32 *dst;
 	int err;
@@ -419,38 +418,38 @@
 	if (opt == NULL)
 		return handle ? -EINVAL : 0;
 
-	if (rtattr_parse(tb, TCA_RSVP_MAX, RTA_DATA(opt), RTA_PAYLOAD(opt)) < 0)
+	if (rtattr_parse_nested(tb, TCA_RSVP_MAX, opt) < 0)
 		return -EINVAL;
 
+	err = tcf_exts_validate(tp, tb, tca[TCA_RATE-1], &e, &rsvp_ext_map);
+	if (err < 0)
+		return err;
+
 	if ((f = (struct rsvp_filter*)*arg) != NULL) {
 		/* Node exists: adjust only classid */
 
 		if (f->handle != handle && handle)
-			return -EINVAL;
+			goto errout2;
 		if (tb[TCA_RSVP_CLASSID-1]) {
 			f->res.classid = *(u32*)RTA_DATA(tb[TCA_RSVP_CLASSID-1]);
 			tcf_bind_filter(tp, &f->res, base);
 		}
-#ifdef CONFIG_NET_CLS_POLICE
-		if (tb[TCA_RSVP_POLICE-1]) {
-			err = tcf_change_police(tp, &f->police,
-				tb[TCA_RSVP_POLICE-1], tca[TCA_RATE-1]);
-			if (err < 0)
-				return err;
-		}
-#endif
+
+		tcf_exts_change(tp, &f->exts, &e);
 		return 0;
 	}
 
 	/* Now more serious part... */
+	err = -EINVAL;
 	if (handle)
-		return -EINVAL;
+		goto errout2;
 	if (tb[TCA_RSVP_DST-1] == NULL)
-		return -EINVAL;
+		goto errout2;
 
+	err = -ENOBUFS;
 	f = kmalloc(sizeof(struct rsvp_filter), GFP_KERNEL);
 	if (f == NULL)
-		return -ENOBUFS;
+		goto errout2;
 
 	memset(f, 0, sizeof(*f));
 	h2 = 16;
@@ -516,10 +515,8 @@
 			f->sess = s;
 			if (f->tunnelhdr == 0)
 				tcf_bind_filter(tp, &f->res, base);
-#ifdef CONFIG_NET_CLS_POLICE
-			if (tb[TCA_RSVP_POLICE-1])
-				tcf_change_police(tp, &f->police, tb[TCA_RSVP_POLICE-1], tca[TCA_RATE-1]);
-#endif
+
+			tcf_exts_change(tp, &f->exts, &e);
 
 			for (fp = &s->ht[h2]; *fp; fp = &(*fp)->next)
 				if (((*fp)->spi.mask&f->spi.mask) != f->spi.mask)
@@ -560,6 +557,8 @@
 errout:
 	if (f)
 		kfree(f);
+errout2:
+	tcf_exts_destroy(tp, &e);
 	return err;
 }
 
@@ -624,17 +623,14 @@
 		RTA_PUT(skb, TCA_RSVP_CLASSID, 4, &f->res.classid);
 	if (((f->handle>>8)&0xFF) != 16)
 		RTA_PUT(skb, TCA_RSVP_SRC, sizeof(f->src), f->src);
-#ifdef CONFIG_NET_CLS_POLICE
-	if (tcf_dump_police(skb, f->police, TCA_RSVP_POLICE) < 0)
+
+	if (tcf_exts_dump(skb, &f->exts, &rsvp_ext_map) < 0)
 		goto rtattr_failure;
-#endif
 
 	rta->rta_len = skb->tail - b;
-#ifdef CONFIG_NET_CLS_POLICE
-	if (f->police)
-		if (tcf_police_dump_stats(skb, f->police) < 0)
-			goto rtattr_failure;
-#endif
+
+	if (tcf_exts_dump_stats(skb, &f->exts, &rsvp_ext_map) < 0)
+		goto rtattr_failure;
 	return skb->len;
 
 rtattr_failure:

[PATCH 8/9] PKT_SCHED: Remove old action/police helpers

[Thomas Graf]

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

--- linux-2.6.10-bk1.orig/include/net/pkt_cls.h	2004-12-27 21:35:31.000000000 +0100
+++ linux-2.6.10-bk1/include/net/pkt_cls.h	2004-12-27 22:46:36.000000000 +0100
@@ -149,88 +149,6 @@
 extern int tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts,
 	                       struct tcf_ext_map *map);
 
-#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 = tcf_action_init_1(act_police_tlv, rate_tlv, "police",
-	                        TCA_ACT_NOREPLACE, TCA_ACT_BIND, &ret);
-	if (act == NULL)
-		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 = tcf_action_init(act_tlv, rate_tlv, NULL,
-	                      TCA_ACT_NOREPLACE, TCA_ACT_BIND, &ret);
-	if (act == NULL)
-		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)
@@ -260,44 +178,4 @@
 }
 #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 9/9] PKT_SCHED: Actions are now available for all classifiers

[Thomas Graf]

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

--- linux-2.6.10-bk1.orig/net/sched/Kconfig	2004-12-27 14:32:33.000000000 +0100
+++ linux-2.6.10-bk1/net/sched/Kconfig	2004-12-27 22:47:22.000000000 +0100
@@ -381,7 +381,6 @@
 	---help---
 	This option requires you have a new iproute2. It enables
 	tc extensions which can be used with tc classifiers.
-	Only the u32 and fw classifiers are supported at the moment.
 	  You MUST NOT turn this on if you dont have an update iproute2.
 
 config NET_ACT_POLICE

Re: [PATCH 2/9] PKT_SCHED: tc filter extension API

[jamal]

Thomas,

Havent looked at the whole set - will later today; quick question:

On Thu, 2004-12-30 at 07:30, Thomas Graf wrote:

> +struct tcf_exts
> +{
> +#ifdef CONFIG_NET_CLS_ACT
> +	struct tc_action *action;
> +#elif defined CONFIG_NET_CLS_POLICE
> +	struct tcf_police *police;
> +#endif
> +};

Things like above:
In current code you can have CONFIG_NET_CLS_ACT and not use new
style policer, rather use old one i.e CONFIG_NET_CLS_POLICE. You seem to
indicate presence of CONFIG_NET_CLS_ACT implies absence of
NET_CLS_POLICE.
A fix for example maybe to s/elif/ifdef [1]

Anyways, back later to peek some more.

cheers,
jamal


[1]Look at Kconfig rules:

----
config NET_CLS_POLICE
        ...
        depends on NET_CLS && NET_QOS && NET_ACT_POLICE!=y &&  
NET_ACT_POLICE!=m
----

Anyways, back later to peek some more.

cheers,
jamal

Re: [PATCH 2/9] PKT_SCHED: tc filter extension API

[Thomas Graf]

* jamal <1104414713.1047.130.camel@jzny.localdomain> 2004-12-30 08:51
> In current code you can have CONFIG_NET_CLS_ACT and not use new
> style policer, rather use old one i.e CONFIG_NET_CLS_POLICE. You seem to
> indicate presence of CONFIG_NET_CLS_ACT implies absence of
> NET_CLS_POLICE.

Is this wrong? Current code: (u32)

2004/06/15 hadi       | #ifdef CONFIG_NET_CLS_ACT
2004/06/15 hadi       | 	struct tc_action	*action;
2004/06/15 hadi       | #else
2002/02/05 torvalds   | #ifdef CONFIG_NET_CLS_POLICE
2002/02/05 torvalds   | 	struct tcf_police	*police;
2002/02/05 torvalds   | #endif
2004/06/15 hadi       | #endif

> config NET_CLS_POLICE
>         ...
>         depends on NET_CLS && NET_QOS && NET_ACT_POLICE!=y &&  
> NET_ACT_POLICE!=m

Hmm... doesn't make too much sense for me. What's the advantage of
allowing this mix?

[RESEND 2/9] PKT_SCHED: tc filter extension API

[Thomas Graf]

Resend: Added unused __attribute__ to the rtattr_failure error labels
to surpress the warnings if no extensions are compiled in. It's not
pretty but moving it into the ifdefs is even worse. Missed this
latest change in the patch. Sorry.

The tcf_exts API abstracts extensions such as actions/policers
into a generic layer and reduces the knowledge inside classifiers
to the minimum required. It isolates the validation code into
its own function to allow classifiers to validate all input
data before making changes and thus avoids the need to undo changes
if a extension configuration request cannot be fullfilled.

As a nice side effect, using this API removes the existing
ifdef clutter.

Usage:
  The classifier holds struct tcf_exts which may be empty if no
  extensions are compiled in. It then calls tcf_exts_validate
  when a new change request was received and provides a temporary
  tcf_exts copy to store the change requests. Given it succeeded
  the classifier may change its own parameters and at the end
  call tcf_exts_change to commit the changes and replace the
  existing extension configuration with the new one. The classifier
  is responsible to destroy his temporary copy if any of its own
  validation checks fail.

  The classifier specific TLV types must be exported to the extensions
  API via tcf_ext_map.

  Destroying the extensions is as easy as calling tcf_exts_destroy.

  The extensions are executed by the classifier by calling tcf_exts_exec
  which must be done as the last thing after making sure the
  filter matches. Note: A classifier might take further actions after
  the execution to tcf_exts_exec such as correcting its own cache to
  avoid caching results which could have been influenced by the extensions.

  tcf_exts_exec returns a negative error code if the filter must be
  considered unmatched, 0 on normal execution or a positive classifier
  return code (TC_ACT_*) which must be returned to the underlying layer
  as-is.

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

--- linux-2.6.10-bk2.orig/include/net/pkt_cls.h	2004-12-30 01:22:01.000000000 +0100
+++ linux-2.6.10-bk2/include/net/pkt_cls.h	2004-12-30 01:22:39.000000000 +0100
@@ -62,6 +62,93 @@
 		tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
 }
 
+struct tcf_exts
+{
+#ifdef CONFIG_NET_CLS_ACT
+	struct tc_action *action;
+#elif defined CONFIG_NET_CLS_POLICE
+	struct tcf_police *police;
+#endif
+};
+
+/* Map to export classifier specific extension TLV types to the
+ * generic extensions API. Unsupported extensions must be set to 0.
+ */
+struct tcf_ext_map
+{
+	int action;
+	int police;
+};
+
+/**
+ * tcf_exts_is_predicative - check if a predicative extension is present
+ * @exts: tc filter extensions handle
+ *
+ * Returns 1 if a predicative extension is present, i.e. an extension which
+ * might cause further actions and thus overrule the regular tcf_result.
+ */
+static inline int
+tcf_exts_is_predicative(struct tcf_exts *exts)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	return !!exts->action;
+#elif defined CONFIG_NET_CLS_POLICE
+	return !!exts->police;
+#else
+	return 0;
+#endif
+}
+
+/**
+ * tcf_exts_is_available - check if at least one extension is present
+ * @exts: tc filter extensions handle
+ *
+ * Returns 1 if at least one extension is present.
+ */
+static inline int
+tcf_exts_is_available(struct tcf_exts *exts)
+{
+	/* All non-predicative extensions must be added here. */
+	return tcf_exts_is_predicative(exts);
+}
+
+/**
+ * tcf_exts_exec - execute tc filter extensions
+ * @skb: socket buffer
+ * @exts: tc filter extensions handle
+ * @res: desired result
+ *
+ * Executes all configured extensions. Returns 0 on a normal execution,
+ * a negative number if the filter must be considered unmatched or
+ * a positive action code (TC_ACT_*) which must be returned to the
+ * underlying layer.
+ */
+static inline int
+tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts,
+	       struct tcf_result *res)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (exts->action)
+		return tcf_action_exec(skb, exts->action, res);
+#elif defined CONFIG_NET_CLS_POLICE
+	if (exts->police)
+		return tcf_police(skb, exts->police);
+#endif
+
+	return 0;
+}
+
+extern int tcf_exts_validate(struct tcf_proto *tp, struct rtattr **tb,
+	                     struct rtattr *rate_tlv, struct tcf_exts *exts,
+	                     struct tcf_ext_map *map);
+extern void tcf_exts_destroy(struct tcf_proto *tp, struct tcf_exts *exts);
+extern void tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
+	                     struct tcf_exts *src);
+extern int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts,
+	                 struct tcf_ext_map *map);
+extern int tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts,
+	                       struct tcf_ext_map *map);
+
 #ifdef CONFIG_NET_CLS_ACT
 static inline int
 tcf_change_act_police(struct tcf_proto *tp, struct tc_action **action,
--- linux-2.6.10-bk2.orig/net/sched/cls_api.c	2004-12-24 22:34:26.000000000 +0100
+++ linux-2.6.10-bk2/net/sched/cls_api.c	2004-12-30 17:03:52.000000000 +0100
@@ -439,6 +439,162 @@
 	return skb->len;
 }
 
+void
+tcf_exts_destroy(struct tcf_proto *tp, struct tcf_exts *exts)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (exts->action) {
+		tcf_action_destroy(exts->action, TCA_ACT_UNBIND);
+		exts->action = NULL;
+	}
+#elif defined CONFIG_NET_CLS_POLICE
+	if (exts->police) {
+		tcf_police_release(exts->police, TCA_ACT_UNBIND);
+		exts->police = NULL;
+	}
+#endif
+}
+
+
+int
+tcf_exts_validate(struct tcf_proto *tp, struct rtattr **tb,
+	          struct rtattr *rate_tlv, struct tcf_exts *exts,
+	          struct tcf_ext_map *map)
+{
+	memset(exts, 0, sizeof(*exts));
+	
+#ifdef CONFIG_NET_CLS_ACT
+	int err;
+	struct tc_action *act;
+
+	if (map->police && tb[map->police-1] && rate_tlv) {
+		act = tcf_action_init_1(tb[map->police-1], rate_tlv, "police",
+			TCA_ACT_NOREPLACE, TCA_ACT_BIND, &err);
+		if (NULL == act)
+			return err;
+
+		act->type = TCA_OLD_COMPAT;
+		exts->action = act;
+	} else if (map->action && tb[map->action-1] && rate_tlv) {
+		act = tcf_action_init(tb[map->action-1], rate_tlv, NULL,
+			TCA_ACT_NOREPLACE, TCA_ACT_BIND, &err);
+		if (NULL == act)
+			return err;
+
+		exts->action = act;
+	}
+#elif defined CONFIG_NET_CLS_POLICE
+	if (map->police && tb[map->police-1] && rate_tlv) {
+		struct tcf_police *p;
+		
+		p = tcf_police_locate(tb[map->police-1], rate_tlv);
+		if (NULL == p)
+			return -EINVAL;
+
+		exts->police = p;
+	} else if (map->action && tb[map->action-1])
+		return -EOPNOTSUPP;
+#else
+	if ((map->action && tb[map->action-1]) ||
+	    (map->police && tb[map->police-1]))
+		return -EOPNOTSUPP;
+#endif
+
+	return 0;
+}
+
+void
+tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
+	        struct tcf_exts *src)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (src->action) {
+		if (dst->action) {
+			struct tc_action *act;
+
+			tcf_tree_lock(tp);
+			act = xchg(&dst->action, src->action);
+			tcf_tree_unlock(tp);
+
+			tcf_action_destroy(act, TCA_ACT_UNBIND);
+		} else
+			dst->action = src->action;
+	}
+#elif defined CONFIG_NET_CLS_POLICE
+	if (src->police) {
+		if (dst->police) {
+			struct tcf_police *p;
+
+			tcf_tree_lock(tp);
+			p = xchg(&dst->police, src->police);
+			tcf_tree_unlock(tp);
+
+			tcf_police_release(p, TCA_ACT_UNBIND);
+		} else
+			dst->police = src->police;
+	}
+#endif
+}
+
+int
+tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts,
+	      struct tcf_ext_map *map)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (map->action && exts->action) {
+		/*
+		 * 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
+		 */
+		struct rtattr * p_rta = (struct rtattr*) skb->tail;
+
+		if (exts->action->type != TCA_OLD_COMPAT) {
+			RTA_PUT(skb, map->action, 0, NULL);
+			if (tcf_action_dump(skb, exts->action, 0, 0) < 0)
+				goto rtattr_failure;
+			p_rta->rta_len = skb->tail - (u8*)p_rta;
+		} else if (map->police) {
+			RTA_PUT(skb, map->police, 0, NULL);
+			if (tcf_action_dump_old(skb, exts->action, 0, 0) < 0)
+				goto rtattr_failure;
+			p_rta->rta_len = skb->tail - (u8*)p_rta;
+		}
+	}
+#elif defined CONFIG_NET_CLS_POLICE
+	if (map->police && exts->police) {
+		struct rtattr * p_rta = (struct rtattr*) skb->tail;
+
+		RTA_PUT(skb, map->police, 0, NULL);
+
+		if (tcf_police_dump(skb, exts->police) < 0)
+			goto rtattr_failure;
+
+		p_rta->rta_len = skb->tail - (u8*)p_rta;
+	}
+#endif
+	return 0;
+rtattr_failure: __attribute__ ((unused))
+	return -1;
+}
+
+int
+tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts,
+	            struct tcf_ext_map *map)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (exts->action)
+		if (tcf_action_copy_stats(skb, exts->action) < 0)
+			goto rtattr_failure;
+#elif defined CONFIG_NET_CLS_POLICE
+	if (exts->police)
+		if (tcf_police_dump_stats(skb, exts->police) < 0)
+			goto rtattr_failure;
+#endif
+	return 0;
+rtattr_failure: __attribute__ ((unused))
+	return -1;
+}
 
 static int __init tc_filter_init(void)
 {
@@ -461,3 +617,8 @@
 
 EXPORT_SYMBOL(register_tcf_proto_ops);
 EXPORT_SYMBOL(unregister_tcf_proto_ops);
+EXPORT_SYMBOL(tcf_exts_validate);
+EXPORT_SYMBOL(tcf_exts_destroy);
+EXPORT_SYMBOL(tcf_exts_change);
+EXPORT_SYMBOL(tcf_exts_dump);
+EXPORT_SYMBOL(tcf_exts_dump_stats);

Re: [PATCH 2/9] PKT_SCHED: tc filter extension API

[Patrick McHardy]

Thomas Graf wrote:

> +int
> +tcf_exts_validate(struct tcf_proto *tp, struct rtattr **tb,
> +	          struct rtattr *rate_tlv, struct tcf_exts *exts,
> +	          struct tcf_ext_map *map)
> +{
> +	memset(exts, 0, sizeof(*exts));
> +	
> +#ifdef CONFIG_NET_CLS_ACT
> +	int err;
> +	struct tc_action *act;
> +
> +	if (map->police && tb[map->police-1] && rate_tlv) {
> +		act = tcf_action_init_1(tb[map->police-1], rate_tlv, "police",
> +			TCA_ACT_NOREPLACE, TCA_ACT_BIND, &err);
> +		if (NULL == act)

Please use act == NULL


> +void
> +tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
> +	        struct tcf_exts *src)
> +{
> +#ifdef CONFIG_NET_CLS_ACT
> +	if (src->action) {
> +		if (dst->action) {
> +			struct tc_action *act;
> +
> +			tcf_tree_lock(tp);
> +			act = xchg(&dst->action, src->action);
> +			tcf_tree_unlock(tp);
> +
> +			tcf_action_destroy(act, TCA_ACT_UNBIND);
> +		} else
> +			dst->action = src->action;

This isn't right (its also wrong in the current code). If the
CPU reorders stores and another CPU looks at dst->action at
the wrong time it might see an inconsistent structure. So at
least smp_wmb is required for the unlocked case, but I think
you should just remove it completely. I also wonder if anyone
actually knows why we need the xchg (here and in all the other
places), it looks totally useless.

Regards
Patrick

Re: [PATCH 2/9] PKT_SCHED: tc filter extension API

[Arnaldo Carvalho de Melo]

Patrick McHardy wrote:
> Thomas Graf wrote:
> 
>> +int
>> +tcf_exts_validate(struct tcf_proto *tp, struct rtattr **tb,
>> +              struct rtattr *rate_tlv, struct tcf_exts *exts,
>> +              struct tcf_ext_map *map)
>> +{
>> +    memset(exts, 0, sizeof(*exts));
>> +   
>> +#ifdef CONFIG_NET_CLS_ACT
>> +    int err;
>> +    struct tc_action *act;
>> +
>> +    if (map->police && tb[map->police-1] && rate_tlv) {
>> +        act = tcf_action_init_1(tb[map->police-1], rate_tlv, "police",
>> +            TCA_ACT_NOREPLACE, TCA_ACT_BIND, &err);
>> +        if (NULL == act)
> 
> 
> Please use act == NULL

Agreed, but I understand the people who like this (ugly) style, it becomes
less likely to become an "if (act = NULL)", but hey, compilers already
helps us with a nice warning.

- Arnaldo

Re: [PATCH 2/9] PKT_SCHED: tc filter extension API

[jamal]

On Thu, 2004-12-30 at 07:30, Thomas Graf wrote:

>   The extensions are executed by the classifier by calling tcf_exts_exec
>   which must be done as the last thing after making sure the
>   filter matches. Note: A classifier might take further actions after
>   the execution to tcf_exts_exec such as correcting its own cache to
>   avoid caching results which could have been influenced by the extensions.

Which cache?

>   tcf_exts_exec returns a negative error code if the filter must be
>   considered unmatched, 0 on normal execution or a positive classifier
>   return code (TC_ACT_*) which must be returned to the underlying layer
>   as-is.

Look at the TC_ACT_*; i think they should be sufficient.

> 
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> 
> --- linux-2.6.10-bk2.orig/include/net/pkt_cls.h	2004-12-30 01:22:01.000000000 +0100
> +++ linux-2.6.10-bk2/include/net/pkt_cls.h	2004-12-30 01:22:39.000000000 +0100


> +/**
> + * tcf_exts_exec - execute tc filter extensions
> + * @skb: socket buffer
> + * @exts: tc filter extensions handle
> + * @res: desired result
> + *
> + * Executes all configured extensions. Returns 0 on a normal execution,
> + * a negative number if the filter must be considered unmatched or
> + * a positive action code (TC_ACT_*) which must be returned to the
> + * underlying layer.

TCA_ACT_OK is 0. 



>  tcf_change_act_police(struct tcf_proto *tp, struct tc_action **action,
> --- linux-2.6.10-bk2.orig/net/sched/cls_api.c	2004-12-30 01:22:01.000000000 +0100
> +++ linux-2.6.10-bk2/net/sched/cls_api.c	2004-12-30 00:47:06.000000000 +0100
> @@ -439,6 +439,162 @@
>  	return skb->len;
>  }

> +
> +int
> +tcf_exts_validate(struct tcf_proto *tp, struct rtattr **tb,
> +	          struct rtattr *rate_tlv, struct tcf_exts *exts,
> +	          struct tcf_ext_map *map)
> +{
> +	memset(exts, 0, sizeof(*exts));
> +	
> +#ifdef CONFIG_NET_CLS_ACT
> +	int err;
> +	struct tc_action *act;
> +
> +	if (map->police && tb[map->police-1] && rate_tlv) {
> +		act = tcf_action_init_1(tb[map->police-1], rate_tlv, "police",
> +			TCA_ACT_NOREPLACE, TCA_ACT_BIND, &err);
> +		if (NULL == act)
> +			return err;
> +
> +		act->type = TCA_OLD_COMPAT;
> +		exts->action = act;
> +	} else if (map->action && tb[map->action-1] && rate_tlv) {
> +		act = tcf_action_init(tb[map->action-1], rate_tlv, NULL,
> +			TCA_ACT_NOREPLACE, TCA_ACT_BIND, &err);
> +		if (NULL == act)
> +			return err;
> +
> +		exts->action = act;
> +	}
> +#elif defined CONFIG_NET_CLS_POLICE
> +	if (map->police && tb[map->police-1] && rate_tlv) {
> +		struct tcf_police *p;
> +		
> +		p = tcf_police_locate(tb[map->police-1], rate_tlv);
> +		if (NULL == p)
> +			return -EINVAL;
> +
> +		exts->police = p;
> +	} else if (map->action && tb[map->action-1])
> +		return -EOPNOTSUPP;
> +#else
> +	if ((map->action && tb[map->action-1]) ||
> +	    (map->police && tb[map->police-1]))
> +		return -EOPNOTSUPP;
> +#endif
> +
> +	return 0;
> +}

Maybe a few DPRINTKs for debugging purposes?

> +void
> +tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
> +	        struct tcf_exts *src)
> +{
> +#ifdef CONFIG_NET_CLS_ACT
> +	if (src->action) {
> +		if (dst->action) {
> +			struct tc_action *act;
> +
> +			tcf_tree_lock(tp);
> +			act = xchg(&dst->action, src->action);
> +			tcf_tree_unlock(tp);
> +
> +			tcf_action_destroy(act, TCA_ACT_UNBIND);
> +		} else
> +			dst->action = src->action;
> +	}
> +#elif defined CONFIG_NET_CLS_POLICE
> +	if (src->police) {
> +		if (dst->police) {
> +			struct tcf_police *p;
> +
> +			tcf_tree_lock(tp);
> +			p = xchg(&dst->police, src->police);
> +			tcf_tree_unlock(tp);
> +
> +			tcf_police_release(p, TCA_ACT_UNBIND);
> +		} else
> +			dst->police = src->police;
> +	}
> +#endif

Patrick pointed this in other email: the xchg is sort of defeated by the
else. Perhaps make  the second one xchg as well.

BTW Thomas: Hopefully these patches match closely what was already in
place? (sorry didnt cross reference)
i.e if any optimizations we should probably bring next phase

cheers,
jamal

Re: [PATCH 2/9] PKT_SCHED: tc filter extension API

[jamal]

On Thu, 2004-12-30 at 09:09, Thomas Graf wrote:
> * jamal <1104414713.1047.130.camel@jzny.localdomain> 2004-12-30 08:51
> > In current code you can have CONFIG_NET_CLS_ACT and not use new
> > style policer, rather use old one i.e CONFIG_NET_CLS_POLICE. You seem to
> > indicate presence of CONFIG_NET_CLS_ACT implies absence of
> > NET_CLS_POLICE.
> 
> Is this wrong? Current code: (u32)
> 
> 2004/06/15 hadi       | #ifdef CONFIG_NET_CLS_ACT
> 2004/06/15 hadi       | 	struct tc_action	*action;
> 2004/06/15 hadi       | #else
> 2002/02/05 torvalds   | #ifdef CONFIG_NET_CLS_POLICE
> 2002/02/05 torvalds   | 	struct tcf_police	*police;
> 2002/02/05 torvalds   | #endif
> 2004/06/15 hadi       | #endif
> 
> > config NET_CLS_POLICE
> >         ...
> >         depends on NET_CLS && NET_QOS && NET_ACT_POLICE!=y &&  
> > NET_ACT_POLICE!=m
> 
> Hmm... doesn't make too much sense for me. What's the advantage of
> allowing this mix?

Ok, send a patch for the Kconfig then;-> Make sure that CLS_POLICE cant
be selected if CONFIG_NET_CLS_ACT is on.
[Agreed that doing it this way would allow killing the policer sooner]

cheers,
jamal

Re: [PATCH 3/9] PKT_SCHED: u32: make use of tcf_exts API

[jamal]

On Thu, 2004-12-30 at 07:31, Thomas Graf wrote:

>  
> +static struct tcf_ext_map u32_ext_map = {
> +	.action = TCA_U32_ACT,
> +	.police = TCA_U32_POLICE
> +};

Repeated on all classifiers - perhaps a little macro magic in order? ;->

Generally rest all looking good - I need to stare some more at the route
and tcindex one but dont see any show stoppers off hand;

cheers,
jamal

Re: [PATCH 2/9] PKT_SCHED: tc filter extension API

[jamal]

On Thu, 2004-12-30 at 20:01, Patrick McHardy wrote:

> > +#ifdef CONFIG_NET_CLS_ACT
> > +	if (src->action) {
> > +		if (dst->action) {
> > +			struct tc_action *act;
> > +
> > +			tcf_tree_lock(tp);
> > +			act = xchg(&dst->action, src->action);
> > +			tcf_tree_unlock(tp);
> > +
> > +			tcf_action_destroy(act, TCA_ACT_UNBIND);
> > +		} else
> > +			dst->action = src->action;
> 
> This isn't right (its also wrong in the current code). If the
> CPU reorders stores and another CPU looks at dst->action at
> the wrong time it might see an inconsistent structure. 

I think an xchg around the else should fix this.

> So at
> least smp_wmb is required for the unlocked case,

or maybe this. 

>  but I think
> you should just remove it completely. I also wonder if anyone
> actually knows why we need the xchg (here and in all the other
> places), it looks totally useless.

All these were put in by Alexey and the LinuxWay(tm) took effect.
an xchg puts almost a lock and ensures an atomic swap. I dont see any
harm in leaving it as is - just needs fixing the else

cheers,
jamal

Re: [PATCH 2/9] PKT_SCHED: tc filter extension API

[jamal]

On Thu, 2004-12-30 at 21:04, Arnaldo Carvalho de Melo wrote:

> > 
> > Please use act == NULL
> 
> Agreed, but I understand the people who like this (ugly) style, it becomes
> less likely to become an "if (act = NULL)", but hey, compilers already
> helps us with a nice warning.

There are certain people who would kill you for doing the above ;->
Or maybe theyll just take away your coffee. I think its a taste issue.
Code looks fine either way. 

cheers,
jamal

Re: [PATCH 2/9] PKT_SCHED: tc filter extension API

[Patrick McHardy]

jamal wrote:
> On Thu, 2004-12-30 at 20:01, Patrick McHardy wrote:
> 
>>This isn't right (its also wrong in the current code). If the
>>CPU reorders stores and another CPU looks at dst->action at
>>the wrong time it might see an inconsistent structure. 
> 
> 
> I think an xchg around the else should fix this.

Yes.

>>I also wonder if anyone
>>actually knows why we need the xchg (here and in all the other
>>places), it looks totally useless.
> 
> All these were put in by Alexey and the LinuxWay(tm) took effect.
> an xchg puts almost a lock and ensures an atomic swap. I dont see any
> harm in leaving it as is - just needs fixing the else

No real harm, but it still should be removed IMO, or used _instead_
of tcf_tree_lock in this place. I've asked myself multiple times what
it is meant for and I've seen others do the same, this alone justifies
removing it. Another reason is what you call LinuxWay(tm), strange
things spread on their own and at some time you have to touch lots of
files to get rid them. So its best to do it as early as possible.

Regards
Patrick

Re: [PATCH 2/9] PKT_SCHED: tc filter extension API

[Thomas Graf]

* Patrick McHardy <41D52176.80703@trash.net> 2004-12-31 10:52
> jamal wrote:
> >On Thu, 2004-12-30 at 20:01, Patrick McHardy wrote:
> >
> >>This isn't right (its also wrong in the current code). If the
> >>CPU reorders stores and another CPU looks at dst->action at
> >>the wrong time it might see an inconsistent structure. 
> >
> >
> >I think an xchg around the else should fix this.

Agreed, we do have a problem when changing a existing filter and adding
a action/police. Thanks Patrick.

> >>I also wonder if anyone
> >>actually knows why we need the xchg (here and in all the other
> >>places), it looks totally useless.
> >
> >All these were put in by Alexey and the LinuxWay(tm) took effect.
> >an xchg puts almost a lock and ensures an atomic swap. I dont see any
> >harm in leaving it as is - just needs fixing the else
> 
> No real harm, but it still should be removed IMO, or used _instead_
> of tcf_tree_lock in this place. I've asked myself multiple times what
> it is meant for and I've seen others do the same, this alone justifies
> removing it. Another reason is what you call LinuxWay(tm), strange
> things spread on their own and at some time you have to touch lots of
> files to get rid them. So its best to do it as early as possible.

There are many of those spread all around. Alexey used them right and
now some have been surrounded by locks. I wanted to fix this since ages
but didn't get around yet. I'll remove the tcf lock in my patch and
change the other occurances later.

Re: [PATCH 3/9] PKT_SCHED: u32: make use of tcf_exts API

[Thomas Graf]

* jamal <1104468201.1047.201.camel@jzny.localdomain> 2004-12-30 23:43
> On Thu, 2004-12-30 at 07:31, Thomas Graf wrote:
> 
> >  
> > +static struct tcf_ext_map u32_ext_map = {
> > +	.action = TCA_U32_ACT,
> > +	.police = TCA_U32_POLICE
> > +};
> 
> Repeated on all classifiers - perhaps a little macro magic in order? ;->

I thought about this but couldn't come up with one that makes it
actually easier. I don't want to hardcode action/police in a macro
because we might have extensions only needed for specific classifiers.

> Generally rest all looking good - I need to stare some more at the route
> and tcindex one but dont see any show stoppers off hand;

Yes please, those are the critical points.

Re: [PATCH 2/9] PKT_SCHED: tc filter extension API

[Thomas Graf]

* jamal <1104467816.1049.181.camel@jzny.localdomain> 2004-12-30 23:36
> On Thu, 2004-12-30 at 07:30, Thomas Graf wrote:
> 
> >   The extensions are executed by the classifier by calling tcf_exts_exec
> >   which must be done as the last thing after making sure the
> >   filter matches. Note: A classifier might take further actions after
> >   the execution to tcf_exts_exec such as correcting its own cache to
> >   avoid caching results which could have been influenced by the extensions.
> 
> Which cache?

route classifier maintains a fastmap to cache results. It may only make
use of the cache if no extension is involed in the matching, instead of
just disabling caching completely it only caches results where no
extensions are involved.

> >   tcf_exts_exec returns a negative error code if the filter must be
> >   considered unmatched, 0 on normal execution or a positive classifier
> >   return code (TC_ACT_*) which must be returned to the underlying layer
> >   as-is.
> 
> Look at the TC_ACT_*; i think they should be sufficient.

I know them, I should have written TC_ACT_OK instead of 0. We mean the
same. I defined it a little bit more generic to allow further extensions
to make use of it.

> Maybe a few DPRINTKs for debugging purposes?

Yes, it might be a good idea to introduce a overall debugging level
config option to put in all the generic debug messages such as the
ing_filter indev correction printk.

> Patrick pointed this in other email: the xchg is sort of defeated by the
> else. Perhaps make  the second one xchg as well.

Agreed, I changed it to:

		struct tc_action *act;
		act = xchg(&dst->action, src->action);
		if (act)
			tcf_action_destroy(act, TCA_ACT_UNBIND);

Am I missing something?

I'll resend patch 2 with the cosmetic and locking fixes and patch 9
to fix Kconfig police dependencies.

> BTW Thomas: Hopefully these patches match closely what was already in
> place? (sorry didnt cross reference)

They match as closely as possible, I even inherited the bugs as you can see ;->
The major change is that the actions/police get initialized before changing
the classifier itself and might eventually be destroyed again if the any
changes of the classifier fails.

> i.e if any optimizations we should probably bring next phase

No optimizations, there are a few cosmetic fixes such as to not use
__u* in kernel only space and I inlined the route hashing functions.

[RESEND 2/9] PKT_SCHED: tc filter extension API

[Thomas Graf]

Resend: Fixes missing locking when exchanging action/police
pointers and removes unneded locking and uses xchg instead
as noted by Patrick McHardy.

The tcf_exts API abstracts extensions such as actions/policers
into a generic layer and reduces the knowledge inside classifiers
to the minimum required. It isolates the validation code into
its own function to allow classifiers to validate all input
data before making changes and thus avoids the need to undo changes
if a extension configuration request cannot be fullfilled.

As a nice side effect, using this API removes the existing
ifdef clutter.

Usage:
  The classifier holds struct tcf_exts which may be empty if no
  extensions are compiled in. It then calls tcf_exts_validate
  when a new change request was received and provides a temporary
  tcf_exts copy to store the change requests. Given it succeeded
  the classifier may change its own parameters and at the end
  call tcf_exts_change to commit the changes and replace the
  existing extension configuration with the new one. The classifier
  is responsible to destroy his temporary copy if any of its own
  validation checks fail.

  The classifier specific TLV types must be exported to the extensions
  API via tcf_ext_map.

  Destroying the extensions is as easy as calling tcf_exts_destroy.

  The extensions are executed by the classifier by calling tcf_exts_exec
  which must be done as the last thing after making sure the
  filter matches. Note: A classifier might take further actions after
  the execution to tcf_exts_exec such as correcting its own cache to
  avoid caching results which could have been influenced by the extensions.

  tcf_exts_exec returns a negative error code if the filter must be
  considered unmatched, 0 on normal execution or a positive classifier
  return code (TC_ACT_*) which must be returned to the underlying layer
  as-is.

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


--- linux-2.6.10-bk2.orig/include/net/pkt_cls.h	2004-12-31 14:10:54.000000000 +0100
+++ linux-2.6.10-bk2/include/net/pkt_cls.h	2004-12-30 17:17:56.000000000 +0100
@@ -62,6 +62,93 @@
 		tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
 }
 
+struct tcf_exts
+{
+#ifdef CONFIG_NET_CLS_ACT
+	struct tc_action *action;
+#elif defined CONFIG_NET_CLS_POLICE
+	struct tcf_police *police;
+#endif
+};
+
+/* Map to export classifier specific extension TLV types to the
+ * generic extensions API. Unsupported extensions must be set to 0.
+ */
+struct tcf_ext_map
+{
+	int action;
+	int police;
+};
+
+/**
+ * tcf_exts_is_predicative - check if a predicative extension is present
+ * @exts: tc filter extensions handle
+ *
+ * Returns 1 if a predicative extension is present, i.e. an extension which
+ * might cause further actions and thus overrule the regular tcf_result.
+ */
+static inline int
+tcf_exts_is_predicative(struct tcf_exts *exts)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	return !!exts->action;
+#elif defined CONFIG_NET_CLS_POLICE
+	return !!exts->police;
+#else
+	return 0;
+#endif
+}
+
+/**
+ * tcf_exts_is_available - check if at least one extension is present
+ * @exts: tc filter extensions handle
+ *
+ * Returns 1 if at least one extension is present.
+ */
+static inline int
+tcf_exts_is_available(struct tcf_exts *exts)
+{
+	/* All non-predicative extensions must be added here. */
+	return tcf_exts_is_predicative(exts);
+}
+
+/**
+ * tcf_exts_exec - execute tc filter extensions
+ * @skb: socket buffer
+ * @exts: tc filter extensions handle
+ * @res: desired result
+ *
+ * Executes all configured extensions. Returns 0 on a normal execution,
+ * a negative number if the filter must be considered unmatched or
+ * a positive action code (TC_ACT_*) which must be returned to the
+ * underlying layer.
+ */
+static inline int
+tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts,
+	       struct tcf_result *res)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (exts->action)
+		return tcf_action_exec(skb, exts->action, res);
+#elif defined CONFIG_NET_CLS_POLICE
+	if (exts->police)
+		return tcf_police(skb, exts->police);
+#endif
+
+	return 0;
+}
+
+extern int tcf_exts_validate(struct tcf_proto *tp, struct rtattr **tb,
+	                     struct rtattr *rate_tlv, struct tcf_exts *exts,
+	                     struct tcf_ext_map *map);
+extern void tcf_exts_destroy(struct tcf_proto *tp, struct tcf_exts *exts);
+extern void tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
+	                     struct tcf_exts *src);
+extern int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts,
+	                 struct tcf_ext_map *map);
+extern int tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts,
+	                       struct tcf_ext_map *map);
+
 #ifdef CONFIG_NET_CLS_ACT
 static inline int
 tcf_change_act_police(struct tcf_proto *tp, struct tc_action **action,
--- linux-2.6.10-bk2.orig/net/sched/cls_api.c	2004-12-24 22:34:26.000000000 +0100
+++ linux-2.6.10-bk2/net/sched/cls_api.c	2004-12-31 13:44:59.000000000 +0100
@@ -439,6 +439,150 @@
 	return skb->len;
 }
 
+void
+tcf_exts_destroy(struct tcf_proto *tp, struct tcf_exts *exts)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (exts->action) {
+		tcf_action_destroy(exts->action, TCA_ACT_UNBIND);
+		exts->action = NULL;
+	}
+#elif defined CONFIG_NET_CLS_POLICE
+	if (exts->police) {
+		tcf_police_release(exts->police, TCA_ACT_UNBIND);
+		exts->police = NULL;
+	}
+#endif
+}
+
+
+int
+tcf_exts_validate(struct tcf_proto *tp, struct rtattr **tb,
+	          struct rtattr *rate_tlv, struct tcf_exts *exts,
+	          struct tcf_ext_map *map)
+{
+	memset(exts, 0, sizeof(*exts));
+	
+#ifdef CONFIG_NET_CLS_ACT
+	int err;
+	struct tc_action *act;
+
+	if (map->police && tb[map->police-1] && rate_tlv) {
+		act = tcf_action_init_1(tb[map->police-1], rate_tlv, "police",
+			TCA_ACT_NOREPLACE, TCA_ACT_BIND, &err);
+		if (act == NULL)
+			return err;
+
+		act->type = TCA_OLD_COMPAT;
+		exts->action = act;
+	} else if (map->action && tb[map->action-1] && rate_tlv) {
+		act = tcf_action_init(tb[map->action-1], rate_tlv, NULL,
+			TCA_ACT_NOREPLACE, TCA_ACT_BIND, &err);
+		if (act == NULL)
+			return err;
+
+		exts->action = act;
+	}
+#elif defined CONFIG_NET_CLS_POLICE
+	if (map->police && tb[map->police-1] && rate_tlv) {
+		struct tcf_police *p;
+		
+		p = tcf_police_locate(tb[map->police-1], rate_tlv);
+		if (p == NULL)
+			return -EINVAL;
+
+		exts->police = p;
+	} else if (map->action && tb[map->action-1])
+		return -EOPNOTSUPP;
+#else
+	if ((map->action && tb[map->action-1]) ||
+	    (map->police && tb[map->police-1]))
+		return -EOPNOTSUPP;
+#endif
+
+	return 0;
+}
+
+void
+tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
+	        struct tcf_exts *src)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (src->action) {
+		struct tc_action *act;
+		act = xchg(&dst->action, src->action);
+		if (act)
+			tcf_action_destroy(act, TCA_ACT_UNBIND);
+	}
+#elif defined CONFIG_NET_CLS_POLICE
+	if (src->police) {
+		struct tcf_police *p;
+		p = xchg(&dst->police, src->police);
+		if (p)
+			tcf_police_release(p, TCA_ACT_UNBIND);
+	}
+#endif
+}
+
+int
+tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts,
+	      struct tcf_ext_map *map)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (map->action && exts->action) {
+		/*
+		 * 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
+		 */
+		struct rtattr * p_rta = (struct rtattr*) skb->tail;
+
+		if (exts->action->type != TCA_OLD_COMPAT) {
+			RTA_PUT(skb, map->action, 0, NULL);
+			if (tcf_action_dump(skb, exts->action, 0, 0) < 0)
+				goto rtattr_failure;
+			p_rta->rta_len = skb->tail - (u8*)p_rta;
+		} else if (map->police) {
+			RTA_PUT(skb, map->police, 0, NULL);
+			if (tcf_action_dump_old(skb, exts->action, 0, 0) < 0)
+				goto rtattr_failure;
+			p_rta->rta_len = skb->tail - (u8*)p_rta;
+		}
+	}
+#elif defined CONFIG_NET_CLS_POLICE
+	if (map->police && exts->police) {
+		struct rtattr * p_rta = (struct rtattr*) skb->tail;
+
+		RTA_PUT(skb, map->police, 0, NULL);
+
+		if (tcf_police_dump(skb, exts->police) < 0)
+			goto rtattr_failure;
+
+		p_rta->rta_len = skb->tail - (u8*)p_rta;
+	}
+#endif
+	return 0;
+rtattr_failure: __attribute__ ((unused))
+	return -1;
+}
+
+int
+tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts,
+	            struct tcf_ext_map *map)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (exts->action)
+		if (tcf_action_copy_stats(skb, exts->action) < 0)
+			goto rtattr_failure;
+#elif defined CONFIG_NET_CLS_POLICE
+	if (exts->police)
+		if (tcf_police_dump_stats(skb, exts->police) < 0)
+			goto rtattr_failure;
+#endif
+	return 0;
+rtattr_failure: __attribute__ ((unused))
+	return -1;
+}
 
 static int __init tc_filter_init(void)
 {
@@ -461,3 +605,8 @@
 
 EXPORT_SYMBOL(register_tcf_proto_ops);
 EXPORT_SYMBOL(unregister_tcf_proto_ops);
+EXPORT_SYMBOL(tcf_exts_validate);
+EXPORT_SYMBOL(tcf_exts_destroy);
+EXPORT_SYMBOL(tcf_exts_change);
+EXPORT_SYMBOL(tcf_exts_dump);
+EXPORT_SYMBOL(tcf_exts_dump_stats);

[RESEND 9/9] PKT_SCHED: Actions are now available for all classifiers & Fix police Kconfig dependencies

[Thomas Graf]

Removes outdated comment and make action and old compat policer
mutually exclusive to reflect the code. Noted by Jamal Hadi Salim.

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

--- linux-2.6.10-bk2.orig/net/sched/Kconfig	2004-12-31 14:10:54.000000000 +0100
+++ linux-2.6.10-bk2/net/sched/Kconfig	2004-12-31 15:04:01.000000000 +0100
@@ -381,7 +381,6 @@
 	---help---
 	This option requires you have a new iproute2. It enables
 	tc extensions which can be used with tc classifiers.
-	Only the u32 and fw classifiers are supported at the moment.
 	  You MUST NOT turn this on if you dont have an update iproute2.
 
 config NET_ACT_POLICE
@@ -392,13 +391,6 @@
 	below to select a policer.
 	  You MUST NOT turn this on if you dont have an update iproute2.
 
-config NET_CLS_POLICE
-	bool "Traffic policing (needed for in/egress)"
-	depends on NET_CLS && NET_QOS && NET_ACT_POLICE!=y && NET_ACT_POLICE!=m
-	help
-	  Say Y to support traffic policing (bandwidth limits).  Needed for
-	  ingress and egress rate limiting.
-
 config NET_ACT_GACT
         tristate "generic Actions"
         depends on NET_CLS_ACT
@@ -432,3 +424,11 @@
         ---help---
         requires new iproute2
         This allows for packets to be generically edited
+
+config NET_CLS_POLICE
+	bool "Traffic policing (needed for in/egress)"
+	depends on NET_CLS && NET_QOS && NET_CLS_ACT!=y
+	help
+	  Say Y to support traffic policing (bandwidth limits).  Needed for
+	  ingress and egress rate limiting.
+

Re: [PATCH 2/9] PKT_SCHED: tc filter extension API

[Patrick McHardy]

Thomas Graf wrote:
> * jamal <1104467816.1049.181.camel@jzny.localdomain> 2004-12-30 23:36
> 
> 
>>Patrick pointed this in other email: the xchg is sort of defeated by the
>>else. Perhaps make  the second one xchg as well.
> 
> Agreed, I changed it to:
> 
> 		struct tc_action *act;
> 		act = xchg(&dst->action, src->action);
> 		if (act)
> 			tcf_action_destroy(act, TCA_ACT_UNBIND);
> 
> Am I missing something?

Yes, the xchg without locking is only right in case we don't have
an existing action that needs to be destroyed. Someone might still
be looking at the old action in a softirq. If you want to keep the
"lockless" variant, you need to call synchronize_net() before
tcf_action_destroy().

Regards
Patrick

Re: [PATCH 2/9] PKT_SCHED: tc filter extension API

[Thomas Graf]

* Patrick McHardy <41D55FC9.6040605@trash.net> 2004-12-31 15:18
> Thomas Graf wrote:
> >		struct tc_action *act;
> >		act = xchg(&dst->action, src->action);
> >		if (act)
> >			tcf_action_destroy(act, TCA_ACT_UNBIND);
> >
> >Am I missing something?
> 
> Yes, the xchg without locking is only right in case we don't have
> an existing action that needs to be destroyed. Someone might still
> be looking at the old action in a softirq. If you want to keep the
> "lockless" variant, you need to call synchronize_net() before
> tcf_action_destroy().

You're absolutely right. I will put locks around it again to avoid
having the pointer exchanged in the middle of a classification.
A synchronize_net will avoid the crash but will no prevent a
possible corruption of the classification result if the action is used
more than once.

[FINAL RESEND 2/9] PKT_SCHED: tc filter extension API

[Thomas Graf]

The tcf_exts API abstracts extensions such as actions/policers
into a generic layer and reduces the knowledge inside classifiers
to the minimum required. It isolates the validation code into
its own function to allow classifiers to validate all input
data before making changes and thus avoids the need to undo changes
if a extension configuration request cannot be fullfilled.

Adds missing locking when adding a action/police extension to an
already existing filter. Acquiring  dev->queue_lock makes sure we
don't change the action/police in the middle of a classification.
Noted by Patrick McHardy.

As a nice side effect, using this API removes the existing
ifdef clutter.

Usage:
  The classifier holds struct tcf_exts which may be empty if no
  extensions are compiled in. It then calls tcf_exts_validate
  when a new change request was received and provides a temporary
  tcf_exts copy to store the change requests. Given it succeeded
  the classifier may change its own parameters and at the end
  call tcf_exts_change to commit the changes and replace the
  existing extension configuration with the new one. The classifier
  is responsible to destroy his temporary copy if any of its own
  validation checks fail.

  The classifier specific TLV types must be exported to the extensions
  API via tcf_ext_map.

  Destroying the extensions is as easy as calling tcf_exts_destroy.

  The extensions are executed by the classifier by calling tcf_exts_exec
  which must be done as the last thing after making sure the
  filter matches. Note: A classifier might take further actions after
  the execution to tcf_exts_exec such as correcting its own cache to
  avoid caching results which could have been influenced by the extensions.

  tcf_exts_exec returns a negative error code if the filter must be
  considered unmatched, 0 on normal execution or a positive classifier
  return code (TC_ACT_*) which must be returned to the underlying layer
  as-is.

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

--- linux-2.6.10-bk2.orig/include/net/pkt_cls.h	2004-12-31 14:10:54.000000000 +0100
+++ linux-2.6.10-bk2/include/net/pkt_cls.h	2004-12-30 17:17:56.000000000 +0100
@@ -62,6 +62,93 @@
 		tp->q->ops->cl_ops->unbind_tcf(tp->q, cl);
 }
 
+struct tcf_exts
+{
+#ifdef CONFIG_NET_CLS_ACT
+	struct tc_action *action;
+#elif defined CONFIG_NET_CLS_POLICE
+	struct tcf_police *police;
+#endif
+};
+
+/* Map to export classifier specific extension TLV types to the
+ * generic extensions API. Unsupported extensions must be set to 0.
+ */
+struct tcf_ext_map
+{
+	int action;
+	int police;
+};
+
+/**
+ * tcf_exts_is_predicative - check if a predicative extension is present
+ * @exts: tc filter extensions handle
+ *
+ * Returns 1 if a predicative extension is present, i.e. an extension which
+ * might cause further actions and thus overrule the regular tcf_result.
+ */
+static inline int
+tcf_exts_is_predicative(struct tcf_exts *exts)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	return !!exts->action;
+#elif defined CONFIG_NET_CLS_POLICE
+	return !!exts->police;
+#else
+	return 0;
+#endif
+}
+
+/**
+ * tcf_exts_is_available - check if at least one extension is present
+ * @exts: tc filter extensions handle
+ *
+ * Returns 1 if at least one extension is present.
+ */
+static inline int
+tcf_exts_is_available(struct tcf_exts *exts)
+{
+	/* All non-predicative extensions must be added here. */
+	return tcf_exts_is_predicative(exts);
+}
+
+/**
+ * tcf_exts_exec - execute tc filter extensions
+ * @skb: socket buffer
+ * @exts: tc filter extensions handle
+ * @res: desired result
+ *
+ * Executes all configured extensions. Returns 0 on a normal execution,
+ * a negative number if the filter must be considered unmatched or
+ * a positive action code (TC_ACT_*) which must be returned to the
+ * underlying layer.
+ */
+static inline int
+tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts,
+	       struct tcf_result *res)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (exts->action)
+		return tcf_action_exec(skb, exts->action, res);
+#elif defined CONFIG_NET_CLS_POLICE
+	if (exts->police)
+		return tcf_police(skb, exts->police);
+#endif
+
+	return 0;
+}
+
+extern int tcf_exts_validate(struct tcf_proto *tp, struct rtattr **tb,
+	                     struct rtattr *rate_tlv, struct tcf_exts *exts,
+	                     struct tcf_ext_map *map);
+extern void tcf_exts_destroy(struct tcf_proto *tp, struct tcf_exts *exts);
+extern void tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
+	                     struct tcf_exts *src);
+extern int tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts,
+	                 struct tcf_ext_map *map);
+extern int tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts,
+	                       struct tcf_ext_map *map);
+
 #ifdef CONFIG_NET_CLS_ACT
 static inline int
 tcf_change_act_police(struct tcf_proto *tp, struct tc_action **action,
--- linux-2.6.10-bk2.orig/net/sched/cls_api.c	2004-12-31 19:31:07.000000000 +0100
+++ linux-2.6.10-bk2/net/sched/cls_api.c	2004-12-31 19:26:59.000000000 +0100
@@ -439,6 +439,154 @@
 	return skb->len;
 }
 
+void
+tcf_exts_destroy(struct tcf_proto *tp, struct tcf_exts *exts)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (exts->action) {
+		tcf_action_destroy(exts->action, TCA_ACT_UNBIND);
+		exts->action = NULL;
+	}
+#elif defined CONFIG_NET_CLS_POLICE
+	if (exts->police) {
+		tcf_police_release(exts->police, TCA_ACT_UNBIND);
+		exts->police = NULL;
+	}
+#endif
+}
+
+
+int
+tcf_exts_validate(struct tcf_proto *tp, struct rtattr **tb,
+	          struct rtattr *rate_tlv, struct tcf_exts *exts,
+	          struct tcf_ext_map *map)
+{
+	memset(exts, 0, sizeof(*exts));
+	
+#ifdef CONFIG_NET_CLS_ACT
+	int err;
+	struct tc_action *act;
+
+	if (map->police && tb[map->police-1] && rate_tlv) {
+		act = tcf_action_init_1(tb[map->police-1], rate_tlv, "police",
+			TCA_ACT_NOREPLACE, TCA_ACT_BIND, &err);
+		if (act == NULL)
+			return err;
+
+		act->type = TCA_OLD_COMPAT;
+		exts->action = act;
+	} else if (map->action && tb[map->action-1] && rate_tlv) {
+		act = tcf_action_init(tb[map->action-1], rate_tlv, NULL,
+			TCA_ACT_NOREPLACE, TCA_ACT_BIND, &err);
+		if (act == NULL)
+			return err;
+
+		exts->action = act;
+	}
+#elif defined CONFIG_NET_CLS_POLICE
+	if (map->police && tb[map->police-1] && rate_tlv) {
+		struct tcf_police *p;
+		
+		p = tcf_police_locate(tb[map->police-1], rate_tlv);
+		if (p == NULL)
+			return -EINVAL;
+
+		exts->police = p;
+	} else if (map->action && tb[map->action-1])
+		return -EOPNOTSUPP;
+#else
+	if ((map->action && tb[map->action-1]) ||
+	    (map->police && tb[map->police-1]))
+		return -EOPNOTSUPP;
+#endif
+
+	return 0;
+}
+
+void
+tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
+	        struct tcf_exts *src)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (src->action) {
+		struct tc_action *act;
+		tcf_tree_lock(tp);
+		act = xchg(&dst->action, src->action);
+		tcf_tree_unlock(tp);
+		if (act)
+			tcf_action_destroy(act, TCA_ACT_UNBIND);
+	}
+#elif defined CONFIG_NET_CLS_POLICE
+	if (src->police) {
+		struct tcf_police *p;
+		tcf_tree_lock(tp);
+		p = xchg(&dst->police, src->police);
+		tcf_tree_unlock(tp);
+		if (p)
+			tcf_police_release(p, TCA_ACT_UNBIND);
+	}
+#endif
+}
+
+int
+tcf_exts_dump(struct sk_buff *skb, struct tcf_exts *exts,
+	      struct tcf_ext_map *map)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (map->action && exts->action) {
+		/*
+		 * 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
+		 */
+		struct rtattr * p_rta = (struct rtattr*) skb->tail;
+
+		if (exts->action->type != TCA_OLD_COMPAT) {
+			RTA_PUT(skb, map->action, 0, NULL);
+			if (tcf_action_dump(skb, exts->action, 0, 0) < 0)
+				goto rtattr_failure;
+			p_rta->rta_len = skb->tail - (u8*)p_rta;
+		} else if (map->police) {
+			RTA_PUT(skb, map->police, 0, NULL);
+			if (tcf_action_dump_old(skb, exts->action, 0, 0) < 0)
+				goto rtattr_failure;
+			p_rta->rta_len = skb->tail - (u8*)p_rta;
+		}
+	}
+#elif defined CONFIG_NET_CLS_POLICE
+	if (map->police && exts->police) {
+		struct rtattr * p_rta = (struct rtattr*) skb->tail;
+
+		RTA_PUT(skb, map->police, 0, NULL);
+
+		if (tcf_police_dump(skb, exts->police) < 0)
+			goto rtattr_failure;
+
+		p_rta->rta_len = skb->tail - (u8*)p_rta;
+	}
+#endif
+	return 0;
+rtattr_failure: __attribute__ ((unused))
+	return -1;
+}
+
+int
+tcf_exts_dump_stats(struct sk_buff *skb, struct tcf_exts *exts,
+	            struct tcf_ext_map *map)
+{
+#ifdef CONFIG_NET_CLS_ACT
+	if (exts->action)
+		if (tcf_action_copy_stats(skb, exts->action) < 0)
+			goto rtattr_failure;
+#elif defined CONFIG_NET_CLS_POLICE
+	if (exts->police)
+		if (tcf_police_dump_stats(skb, exts->police) < 0)
+			goto rtattr_failure;
+#endif
+	return 0;
+rtattr_failure: __attribute__ ((unused))
+	return -1;
+}
 
 static int __init tc_filter_init(void)
 {
@@ -461,3 +609,8 @@
 
 EXPORT_SYMBOL(register_tcf_proto_ops);
 EXPORT_SYMBOL(unregister_tcf_proto_ops);
+EXPORT_SYMBOL(tcf_exts_validate);
+EXPORT_SYMBOL(tcf_exts_destroy);
+EXPORT_SYMBOL(tcf_exts_change);
+EXPORT_SYMBOL(tcf_exts_dump);
+EXPORT_SYMBOL(tcf_exts_dump_stats);

Re: [RESEND 9/9] PKT_SCHED: Actions are now available for all classifiers & Fix police Kconfig dependencies

[David S. Miller]

Ok, all 9 patches are in my tree now Thomas.
Thanks.

Let's now see how merging in Patrick's stuff goes.