800+ byte inlines in include/net/pkt_act.h

DISCUSSION Denis Vlasenko 3 participants 5 messages Aug 20, 2006 – Aug 22, 2006

Subsystems (3)

Networking Network drivers Traffic control (tc)

lore

Share & tools

lore

Denis Vlasenko

author

2006-08-20 17:33 UTC #0 lore raw

Hi,

include/net/pkt_act.h plays a game of inlines which are kind of "templatized", like this:

act_ipt.c:

/* ovewrride the defaults */

#define tcf_st          tcf_ipt
#define tcf_t_lock      ipt_lock
#define tcf_ht          tcf_ipt_ht

#define CONFIG_NET_ACT_INIT
#include <net/pkt_act.h>

This results in code duplication. For example, tcf_generic_walker() is duplicated four times.
On i386 it is about 4*800 bytes in text section. Other inlines are a bit smaller but still are substantial. --
vda

jamal

2006-08-21 12:26 UTC #1 lore raw

Quoted 11 lines · Denis Vlasenko

Hi,
include/net/pkt_act.h plays a game of inlines which are kind of "templatized", like this:
act_ipt.c:
/* ovewrride the defaults */
#define tcf_st          tcf_ipt
#define tcf_t_lock      ipt_lock
#define tcf_ht          tcf_ipt_ht

Quoted 7 lines

#define CONFIG_NET_ACT_INIT
#include <net/pkt_act.h>
This results in code duplication. For example, tcf_generic_walker() is duplicated four times.
On i386 it is about 4*800 bytes in text section. Other inlines are a bit smaller but still are substantial.

As per last discussion, either Patrick McHardy or myself are going to work on it - at some point. Please be patient. The other alternative is:
you fix it and send patches.

Note that even after conversion, functions like tcf_generic_walker() are not going to save much in total .text sizes; also note they run the control path and are typically not as much invoked and therefore not performance impacting on smaller cache systems..

cheers,
jamal

David Miller

maintainer

2006-08-21 23:37 UTC #2 lore raw

From: jamal <[email protected]>
Date: Mon, 21 Aug 2006 08:26:00 -0400

Quoted 3 lines

As per last discussion, either Patrick McHardy or myself are going to work on it - at some point. Please be patient. The other alternative is: you fix it and send patches.

I'm working on it right now. This code is really gross and needs to be fixed immediately.

What I'll do is define a "struct tcf_common" and have the generic interfaces take that as well as a "struct tcf_hashinfo *" parameter to deal with the individual hash tables.

We define all of this templated stuff then don't even use it in act_police.c, we just duplicate everything!

Absolutely unbelievable.

jamal

2006-08-22 13:45 UTC #3 lore raw

Quoted 7 lines · David Miller

From: jamal <[email protected]>
Date: Mon, 21 Aug 2006 08:26:00 -0400
Quoted 3 lines
As per last discussion, either Patrick McHardy or myself are going to work on it - at some point. Please be patient. The other alternative is: you fix it and send patches.

Quoted 7 lines

I'm working on it right now. This code is really gross and needs to be fixed immediately.
What I'll do is define a "struct tcf_common" and have the generic interfaces take that as well as a "struct tcf_hashinfo *" parameter to deal with the individual hash tables.

Sounds reasonable. May actually be close to what Patrick and I had in discussion (I cant find my notes) i.e hashinfo would contain table{size,index,mask,lock, and pointer to table}
After staring at the code for a minute, I think the challenges you may face are in the conversions of: tcf_ {dump_walker(), del_walker() and generic_walker()}

Thanks for taking this up Dave. And if you get it started and get distracted somewhere, I could take it over.

Quoted 2 lines

We define all of this templated stuff then don't even use it in act_police.c, we just duplicate everything!

act_police deviates from the generic layout; the intent is to allow for that. The desire was/is for usability for whoever uses the generic layout (read: joe-netfilter) could write a single page of code quickly to do something powerful (like gact for example). It is turning out code augmentation is not such a practical idea in the kernel.

cheers,
jamal

Quoted 6 lines

Absolutely unbelievable.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Miller

maintainer

2006-08-22 06:56 UTC #4 lore raw

In reply to Denis Vlasenko

I've checked in the following to net-2.6.19 to fix this stuff. Thanks for pointing it out Denis.

commit e4fa535c2177fcc92116c397534429feb0f5c0b1

Author: David S. Miller <[email protected]>
Date:   Mon Aug 21 23:54:55 2006 -0700

[PKT_SCHED]: Kill pkt_act.h inlining.

This was simply making templates of functions and mostly causing a lot of code duplication in the classifier action modules.

We solve this more cleanly by having a common "struct tcf_common" that hash worker functions contained once in act_api.c can work with.

Callers work with real action objects that have the common struct plus their module specific struct members. You go from a common object to the higher level one using a "to_foo()" macro which makes use of container_of() to do the dirty work.

This also kills off act_generic.h which was only used by act_simple.c and keeping it around was more work than the it's value.

Signed-off-by David S. Miller <[email protected]>

include/net/act_api.h +88 ~~-48~~

index 11e9eaf..f56957e 100644
@@ -8,70 +8,110 @@ #define __NET_ACT_API_H
 #include <net/sch_generic.h>
 #include <net/pkt_sched.h>
 
-#define tca_gen(name) \
-struct tcf_##name *next; \
-	u32 index; \
-	int refcnt; \
-	int bindcnt; \
-	u32 capab; \
-	int action; \
-	struct tcf_t tm; \
-	struct gnet_stats_basic bstats; \
-	struct gnet_stats_queue qstats; \
-	struct gnet_stats_rate_est rate_est; \
-	spinlock_t *stats_lock; \
-	spinlock_t lock
-
-struct tcf_police
-{
-	tca_gen(police);
-	int		result;
-	u32		ewma_rate;
-	u32		burst;
-	u32		mtu;
-	u32		toks;
-	u32		ptoks;
-	psched_time_t	t_c;
-	struct qdisc_rate_table *R_tab;
-	struct qdisc_rate_table *P_tab;
+struct tcf_common {
+	struct tcf_common		*tcfc_next;
+	u32				tcfc_index;
+	int				tcfc_refcnt;
+	int				tcfc_bindcnt;
+	u32				tcfc_capab;
+	int				tcfc_action;
+	struct tcf_t			tcfc_tm;
+	struct gnet_stats_basic		tcfc_bstats;
+	struct gnet_stats_queue		tcfc_qstats;
+	struct gnet_stats_rate_est	tcfc_rate_est;
+	spinlock_t			*tcfc_stats_lock;
+	spinlock_t			tcfc_lock;
+};
+#define tcf_next	common.tcfc_next
+#define tcf_index	common.tcfc_index
+#define tcf_refcnt	common.tcfc_refcnt
+#define tcf_bindcnt	common.tcfc_bindcnt
+#define tcf_capab	common.tcfc_capab
+#define tcf_action	common.tcfc_action
+#define tcf_tm		common.tcfc_tm
+#define tcf_bstats	common.tcfc_bstats
+#define tcf_qstats	common.tcfc_qstats
+#define tcf_rate_est	common.tcfc_rate_est
+#define tcf_stats_lock	common.tcfc_stats_lock
+#define tcf_lock	common.tcfc_lock
+
+struct tcf_police {
+	struct tcf_common	common;
+	int			tcfp_result;
+	u32			tcfp_ewma_rate;
+	u32			tcfp_burst;
+	u32			tcfp_mtu;
+	u32			tcfp_toks;
+	u32			tcfp_ptoks;
+	psched_time_t		tcfp_t_c;
+	struct qdisc_rate_table	*tcfp_R_tab;
+	struct qdisc_rate_table	*tcfp_P_tab;
 };
+#define to_police(pc)	\
+	container_of(pc, struct tcf_police, common)
+
+struct tcf_hashinfo {
+	struct tcf_common	**htab;
+	unsigned int		hmask;
+	rwlock_t		*lock;
+};
+
+static inline unsigned int tcf_hash(u32 index, unsigned int hmask)
+{
+	return index & hmask;
+}
 
 #ifdef CONFIG_NET_CLS_ACT
 
 #define ACT_P_CREATED 1
 #define ACT_P_DELETED 1
 
-struct tcf_act_hdr
-{
-	tca_gen(act_hdr);
+struct tcf_act_hdr {
+	struct tcf_common	common;
 };
 
-struct tc_action
-{
-	void *priv;
-	struct tc_action_ops *ops;
-	__u32   type;   /* for backward compat(TCA_OLD_COMPAT) */
-	__u32   order; 
-	struct tc_action *next;
+struct tc_action {
+	void			*priv;
+	struct tc_action_ops	*ops;
+	__u32			type; /* for backward compat(TCA_OLD_COMPAT) */
+	__u32			order; 
+	struct tc_action	*next;
 };
 
 #define TCA_CAP_NONE 0
-struct tc_action_ops
-{
+struct tc_action_ops {
 	struct tc_action_ops *next;
+	struct tcf_hashinfo *hinfo;
 	char    kind[IFNAMSIZ];
 	__u32   type; /* TBD to match kind */
 	__u32 	capab;  /* capabilities includes 4 bit version */
 	struct module		*owner;
 	int     (*act)(struct sk_buff *, struct tc_action *, struct tcf_result *);
 	int     (*get_stats)(struct sk_buff *, struct tc_action *);
-	int     (*dump)(struct sk_buff *, struct tc_action *,int , int);
+	int     (*dump)(struct sk_buff *, struct tc_action *, int, int);
 	int     (*cleanup)(struct tc_action *, int bind);
-	int     (*lookup)(struct tc_action *, u32 );
-	int     (*init)(struct rtattr *,struct rtattr *,struct tc_action *, int , int );
-	int     (*walk)(struct sk_buff *, struct netlink_callback *, int , struct tc_action *);
+	int     (*lookup)(struct tc_action *, u32);
+	int     (*init)(struct rtattr *, struct rtattr *, struct tc_action *, int , int);
+	int     (*walk)(struct sk_buff *, struct netlink_callback *, int, struct tc_action *);
 };
 
+extern struct tcf_common *tcf_hash_lookup(u32 index,
+					  struct tcf_hashinfo *hinfo);
+extern void tcf_hash_destroy(struct tcf_common *p, struct tcf_hashinfo *hinfo);
+extern int tcf_hash_release(struct tcf_common *p, int bind,
+			    struct tcf_hashinfo *hinfo);
+extern int tcf_generic_walker(struct sk_buff *skb, struct netlink_callback *cb,
+			      int type, struct tc_action *a);
+extern u32 tcf_hash_new_index(u32 *idx_gen, struct tcf_hashinfo *hinfo);
+extern int tcf_hash_search(struct tc_action *a, u32 index);
+extern struct tcf_common *tcf_hash_check(u32 index, struct tc_action *a,
+					 int bind, struct tcf_hashinfo *hinfo);
+extern struct tcf_common *tcf_hash_create(u32 index, struct rtattr *est,
+					  struct tc_action *a, int size,
+					  int bind, u32 *idx_gen,
+					  struct tcf_hashinfo *hinfo);
+extern void tcf_hash_insert(struct tcf_common *p, struct tcf_hashinfo *hinfo);
+
 extern int tcf_register_action(struct tc_action_ops *a);
 extern int tcf_unregister_action(struct tc_action_ops *a);
 extern void tcf_action_destroy(struct tc_action *a, int bind);

@@ -96,17 +136,17 @@ tcf_police_release(struct tcf_police *p,
 	int ret = 0;
 #ifdef CONFIG_NET_CLS_ACT
 	if (p) {
-		if (bind) {
-			 p->bindcnt--;
-		}
-		p->refcnt--;
-		if (p->refcnt <= 0 && !p->bindcnt) {
+		if (bind)
+			p->tcf_bindcnt--;
+
+		p->tcf_refcnt--;
+		if (p->tcf_refcnt <= 0 && !p->tcf_bindcnt) {
 			tcf_police_destroy(p);
 			ret = 1;
 		}
 	}
 #else
-	if (p && --p->refcnt == 0)
+	if (p && --p->tcf_refcnt == 0)
 		tcf_police_destroy(p);
 
 #endif /* CONFIG_NET_CLS_ACT */

include/net/act_generic.h deleted +0 ~~-142~~

142 lines not on this page · raw message

include/net/pkt_act.h deleted +0 ~~-273~~

273 lines not on this page · raw message

include/net/tc_act/tc_defact.h +7 -6

index 463aa67..65f024b 100644
@@ -3,11 +3,12 @@ #define __NET_TC_DEF_H
 
 #include <net/act_api.h>
 
-struct tcf_defact
-{
-	tca_gen(defact);
-	u32     datalen;
-	void    *defdata;
+struct tcf_defact {
+	struct tcf_common	common;
+	u32     		tcfd_datalen;
+	void    		*tcfd_defdata;
 };
+#define to_defact(pc) \
+	container_of(pc, struct tcf_defact, common)
 
-#endif
+#endif /* __NET_TC_DEF_H */

include/net/tc_act/tc_gact.h +9 -9

index 59f0d96..9e3f676 100644
@@ -3,15 +3,15 @@ #define __NET_TC_GACT_H
 
 #include <net/act_api.h>
 
-struct tcf_gact
-{
-        tca_gen(gact);
+struct tcf_gact {
+	struct tcf_common	common;
 #ifdef CONFIG_GACT_PROB
-        u16                 ptype;
-        u16                 pval;
-        int                 paction;
+        u16			tcfg_ptype;
+        u16			tcfg_pval;
+        int			tcfg_paction;
 #endif
-                                                                                
 };
-                                                                                
-#endif
+#define to_gact(pc) \
+	container_of(pc, struct tcf_gact, common)
+
+#endif /* __NET_TC_GACT_H */

include/net/tc_act/tc_ipt.h +8 -7

index cb37ad0..f7d25df 100644
@@ -5,12 +5,13 @@ #include <net/act_api.h>
 
 struct xt_entry_target;
 
-struct tcf_ipt
-{
-	tca_gen(ipt);
-	u32 hook;
-	char *tname;
-	struct xt_entry_target *t;
+struct tcf_ipt {
+	struct tcf_common	common;
+	u32			tcfi_hook;
+	char			*tcfi_tname;
+	struct xt_entry_target	*tcfi_t;
 };
+#define to_ipt(pc) \
+	container_of(pc, struct tcf_ipt, common)
 
-#endif
+#endif /* __NET_TC_IPT_H */

include/net/tc_act/tc_mirred.h +9 -8

index b5c32f6..ceac661 100644
@@ -3,13 +3,14 @@ #define __NET_TC_MIR_H
 
 #include <net/act_api.h>
 
-struct tcf_mirred
-{
-	tca_gen(mirred);
-	int eaction;
-	int ifindex;
-	int ok_push;
-	struct net_device *dev;
+struct tcf_mirred {
+	struct tcf_common	common;
+	int			tcfm_eaction;
+	int			tcfm_ifindex;
+	int			tcfm_ok_push;
+	struct net_device	*tcfm_dev;
 };
+#define to_mirred(pc) \
+	container_of(pc, struct tcf_mirred, common)
 
-#endif
+#endif /* __NET_TC_MIR_H */

include/net/tc_act/tc_pedit.h +8 -7

index eb21689..e6f6e15 100644
@@ -3,12 +3,13 @@ #define __NET_TC_PED_H
 
 #include <net/act_api.h>
 
-struct tcf_pedit
-{
-	tca_gen(pedit);
-	unsigned char           nkeys;
-	unsigned char           flags;
-	struct tc_pedit_key     *keys;
+struct tcf_pedit {
+	struct tcf_common	common;
+	unsigned char		tcfp_nkeys;
+	unsigned char		tcfp_flags;
+	struct tc_pedit_key	*tcfp_keys;
 };
+#define to_pedit(pc) \
+	container_of(pc, struct tcf_pedit, common)
 
-#endif
+#endif /* __NET_TC_PED_H */

net/sched/act_api.c +227 ~~-19~~

290 lines not on this page · raw message

net/sched/act_gact.c +68 ~~-74~~

237 lines not on this page · raw message

net/sched/act_ipt.c +79 ~~-96~~

329 lines not on this page · raw message

net/sched/act_mirred.c +74 ~~-85~~

268 lines not on this page · raw message

net/sched/act_pedit.c +72 ~~-94~~

281 lines not on this page · raw message

net/sched/act_police.c +260 ~~-248~~

739 lines not on this page · raw message

net/sched/act_simple.c +152 ~~-31~~

162 lines not on this page · raw message

The rest of this diff is not shown. View the raw message.

Part of the diff is not shown here. View the raw message.