Skip to contentSkip to conversation

PKT_SCHED: tcf_exts API & make classifier changes consistent upon failure

PATCH In review Thomas Graf 5 participants 30 messages Dec 30, 2004 – Jan 10, 2005

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

Thomas Graf

author
#0 lore raw

cover PKT_SCHED: tcf_exts API & make classifier changes consistent upon failure

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

author
#1 lore raw

PATCH 1/9 PKT_SCHED: rtattr_parse shortcut for nested TLVs

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

PATCH 2/9 PKT_SCHED: tc filter extension API

In reply to 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 <[email protected]>
include/net/pkt_cls.h +87 -0
@@ -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,
net/sched/cls_api.c +161 -0

170 lines not on this page · raw message

#3 lore raw

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

Thomas,

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

include/net/pkt_cls.h line 72

Quoted 8 lines · Thomas Graf
include/net/pkt_cls.h +8 -0
+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

author
#4 lore raw

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

* jamal <[email protected]> 2004-12-30 08:51

Quoted 4 lines

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
Quoted 4 lines

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?

#5 lore raw

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

Quoted 16 lines · Thomas Graf

* jamal <[email protected]> 2004-12-30 08:51

Quoted 4 lines

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
Quoted 7 lines
Quoted 4 lines

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

author
#6 lore raw

PATCH RESEND 2/9 PKT_SCHED: tc filter extension API

In reply to 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 <[email protected]>
include/net/pkt_cls.h +87 -0
@@ -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,
net/sched/cls_api.c +161 -0

170 lines not on this page · raw message

author
#7 lore raw

PATCH RESEND 2/9 PKT_SCHED: tc filter extension API

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 <[email protected]>
include/net/pkt_cls.h +87 -0
@@ -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,
net/sched/cls_api.c +149 -0

158 lines not on this page · raw message

author
#8 lore raw

PATCH RESEND 2/9 PKT_SCHED: tc filter extension API

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 <[email protected]>
include/net/pkt_cls.h +87 -0
@@ -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,
net/sched/cls_api.c +153 -0

162 lines not on this page · raw message

#9 lore raw

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

In reply to Thomas Graf
Quoted 5 lines · Thomas Graf
net/sched/cls_api.c +5 -0
+int
+tcf_exts_validate(struct tcf_proto *tp, struct rtattr **tb,
+	          struct rtattr *rate_tlv, struct tcf_exts *exts,
+	          struct tcf_ext_map *map)
+{

net/sched/cls_api.c line 473

Quoted 10 lines
net/sched/cls_api.c +10 -0
+	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

Quoted 6 lines
net/sched/cls_api.c +6 -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) {

net/sched/cls_api.c line 521

Quoted 10 lines
net/sched/cls_api.c +10 -0
+		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

#10 lore raw

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

Quoted 12 lines · Patrick McHardy
Quoted 10 lines · Thomas Graf
net/sched/cls_api.c +10 -0
+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;
Quoted 8 lines

net/sched/cls_api.c line 473

Quoted 5 lines
net/sched/cls_api.c +5 -0
+
+    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

#11 lore raw

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

Quoted 6 lines · Arnaldo Carvalho de Melo
Quoted 2 lines

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

#12 lore raw

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

In reply to Patrick McHardy
Quoted 8 lines · Patrick McHardy
Quoted 8 lines
net/sched/cls_api.c +8 -0
+#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);
Quoted 8 lines

net/sched/cls_api.c line 521

Quoted 4 lines
net/sched/cls_api.c +4 -0
+
+			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.

Quoted 2 lines

So at
least smp_wmb is required for the unlocked case,

or maybe this.

Quoted 4 lines

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

#13 lore raw

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

Quoted 8 lines · jamal
Quoted 3 lines · Patrick McHardy

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.

Quoted 7 lines
Quoted 3 lines

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

author
#14 lore raw

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

* Patrick McHardy <[email protected]> 2004-12-31 10:52

Quoted 9 lines
Quoted 8 lines · jamal
Quoted 3 lines · Patrick McHardy

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.

Quoted 8 lines
Quoted 7 lines
Quoted 3 lines

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

Quoted 6 lines

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.

#15 lore raw

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

In reply to Thomas Graf
Quoted 5 lines · Thomas Graf

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?

Quoted 4 lines

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.

Quoted 7 lines
Signed-off-by Thomas Graf <[email protected]>
include/net/pkt_cls.h +0 -0

include/net/pkt_cls.h line 124

Quoted 10 lines
include/net/pkt_cls.h +10 -0
+/**
+ * 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.

Quoted 44 lines

tcf_change_act_police(struct tcf_proto *tp, struct tc_action **action,

net/sched/cls_api.c +37 -0
@@ -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;

net/sched/cls_api.c line 488

Quoted 10 lines
net/sched/cls_api.c +10 -0
+	} 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?

Quoted 21 lines
net/sched/cls_api.c +21 -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;

net/sched/cls_api.c line 536

Quoted 10 lines
net/sched/cls_api.c +10 -0
+
+			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

author
#16 lore raw

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

* jamal <[email protected]> 2004-12-30 23:36

Quoted 9 lines · Thomas Graf
Quoted 6 lines

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.

Quoted 6 lines
Quoted 4 lines

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.

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 1 line

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.

#17 lore raw

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

Quoted 6 lines · Thomas Graf

* jamal <[email protected]> 2004-12-30 23:36

Quoted 2 lines

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

Quoted 8 lines

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

author
#18 lore raw

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

* Patrick McHardy <[email protected]> 2004-12-31 15:18

Quoted 6 lines
Quoted 5 lines · Thomas Graf
		struct tc_action *act;
		act = xchg(&dst->action, src->action);
		if (act)
			tcf_action_destroy(act, TCA_ACT_UNBIND);
Quoted 7 lines
Quoted 1 line

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.

author
#19 lore raw

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

In reply to 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 <[email protected]>
net/sched/cls_u32.c +40 -84
@@ -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:
#20 lore raw

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

net/sched/cls_u32.c line 112

Quoted 5 lines · Thomas Graf
net/sched/cls_u32.c +4 -0
+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

author
#21 lore raw

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

* jamal <[email protected]> 2004-12-30 23:43

Quoted 9 lines · Thomas Graf

net/sched/cls_u32.c line 112

Quoted 6 lines
net/sched/cls_u32.c +4 -0
+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.

Quoted 2 lines

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.

author
#22 lore raw

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

In reply to 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 <[email protected]>
net/sched/cls_fw.c +39 -98
@@ -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:
author
#23 lore raw

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

In reply to 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 <[email protected]>
include/linux/pkt_cls.h +1 -0
@@ -280,6 +280,7 @@
 	TCA_ROUTE4_FROM,
 	TCA_ROUTE4_IIF,
 	TCA_ROUTE4_POLICE,
+	TCA_ROUTE4_ACT,
 	__TCA_ROUTE4_MAX
 };
net/sched/cls_route.c +189 -157

479 lines not on this page · raw message

author
#24 lore raw

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

In reply to 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 <[email protected]>
include/linux/pkt_cls.h +1 -0
@@ -312,6 +312,7 @@
 	TCA_TCINDEX_FALL_THROUGH,
 	TCA_TCINDEX_CLASSID,
 	TCA_TCINDEX_POLICE,
+	TCA_TCINDEX_ACT,
 	__TCA_TCINDEX_MAX
 };
net/sched/cls_tcindex.c +210 -169

478 lines not on this page · raw message

author
#25 lore raw

PATCH 7/9 PKT_SCHED: rsvp: use tcf_exts API

In reply to 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 <[email protected]>
include/linux/pkt_cls.h +1 -0
@@ -249,6 +249,7 @@
 	TCA_RSVP_SRC,
 	TCA_RSVP_PINFO,
 	TCA_RSVP_POLICE,
+	TCA_RSVP_ACT,
 	__TCA_RSVP_MAX
 };
net/sched/cls_rsvp.h +49 -53

197 lines not on this page · raw message

author
#26 lore raw

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

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

PATCH 9/9 PKT_SCHED: Actions are now available for all classifiers

In reply to Thomas Graf
Signed-off-by Thomas Graf <[email protected]>
net/sched/Kconfig +0 -1
@@ -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
author
#28 lore raw

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

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 <[email protected]>
net/sched/Kconfig +8 -8
@@ -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.
+
maintainer
#29 lore raw

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

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

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