[PATCH] reduce netfilte sk_buff enlargement

[PATCH] reduce netfilte sk_buff enlargement

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 19280 bytes --]

Hi!

As discussed at netconf'05, we're trying to save every bit in sk_buff.
The patch below makes sk_buff 8 bytes smaller.  I did some basic testing
on my notebook and it seems to work.

The only real in-tree user of nfcache was IPVS, who only needs a single
bit.  Unfortunately I couldn't find some other free bit in sk_buff to
stuff that bit into, so I introduced a separate field for them.  Maybe
the IPVS guys can resolve that to further save space.

Initially I wanted to shrink pkt_type to three bits (PACKET_HOST and
alike are only 6 values defined), but unfortunately the bluetooth code
overloads pkt_type :(

The conntrack-event-api (out-of-tree) uses nfcache, but Rusty just came
up with a way how to do it without any skb fields, so it's safe to
remove it.


- remove all never-implemented 'nfcache' code
- don't have ipvs code abuse 'nfcache' field. currently get's their own
  compile-conditional skb->ipvs_property field.  IPVS maintainers can
  decide to move this bit elswhere, but nfcache needs to die.
- remove skb->nfcache field to save 4 bytes
- move skb->nfctinfo into three unused bits to save further 4 bytes

Signed-off-by: Harald Welte <laforge@netfilter.org>

---
commit 11f0f1a381dd525aa9b48b64e8fb4dee22ef3520
tree c058108477e708ff81c63318168b6355356e9e6d
parent 1f78366f895151f38c15a2b22b9d4b988dd351e9
author laforge <laforge@netfilter.org> Sun, 17 Jul 2005 23:32:58 +0200
committer laforge <laforge@netfilter.org> Sun, 17 Jul 2005 23:32:58 +0200

 include/linux/netfilter.h              |    5 -----
 include/linux/netfilter_decnet.h       |   10 ----------
 include/linux/netfilter_ipv4.h         |   28 ----------------------------
 include/linux/netfilter_ipv6.h         |   30 ------------------------------
 include/linux/skbuff.h                 |   10 +++++-----
 net/bridge/netfilter/ebt_mark.c        |    5 ++---
 net/core/skbuff.c                      |    2 --
 net/ipv4/ip_output.c                   |    1 -
 net/ipv4/ipvs/ip_vs_core.c             |    9 +++++----
 net/ipv4/ipvs/ip_vs_xmit.c             |    2 +-
 net/ipv4/netfilter/ip_conntrack_core.c |    7 +------
 net/ipv4/netfilter/ip_nat_core.c       |    1 -
 net/ipv4/netfilter/ip_nat_standalone.c |    2 --
 net/ipv4/netfilter/ip_queue.c          |    1 -
 net/ipv4/netfilter/ip_tables.c         |    1 -
 net/ipv4/netfilter/ipt_CLASSIFY.c      |    4 +---
 net/ipv4/netfilter/ipt_CONNMARK.c      |    4 +---
 net/ipv4/netfilter/ipt_DSCP.c          |    1 -
 net/ipv4/netfilter/ipt_ECN.c           |    2 --
 net/ipv4/netfilter/ipt_MARK.c          |   10 ++++------
 net/ipv4/netfilter/ipt_REJECT.c        |    1 -
 net/ipv4/netfilter/ipt_TCPMSS.c        |    1 -
 net/ipv4/netfilter/ipt_TOS.c           |    1 -
 net/ipv6/ip6_output.c                  |   16 ++--------------
 net/ipv6/netfilter/ip6_queue.c         |    1 -
 net/ipv6/netfilter/ip6_tables.c        |    1 -
 net/ipv6/netfilter/ip6t_MARK.c         |    5 ++---
 27 files changed, 24 insertions(+), 137 deletions(-)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -21,11 +21,6 @@
 #define NF_STOP 5
 #define NF_MAX_VERDICT NF_STOP
 
-/* Generic cache responses from hook functions.
-   <= 0x2000 is used for protocol-flags. */
-#define NFC_UNKNOWN 0x4000
-#define NFC_ALTERED 0x8000
-
 #ifdef __KERNEL__
 #include <linux/config.h>
 #ifdef CONFIG_NETFILTER
diff --git a/include/linux/netfilter_decnet.h b/include/linux/netfilter_decnet.h
--- a/include/linux/netfilter_decnet.h
+++ b/include/linux/netfilter_decnet.h
@@ -9,16 +9,6 @@
 
 #include <linux/netfilter.h>
 
-/* IP Cache bits. */
-/* Src IP address. */
-#define NFC_DN_SRC		0x0001
-/* Dest IP address. */
-#define NFC_DN_DST		0x0002
-/* Input device. */
-#define NFC_DN_IF_IN		0x0004
-/* Output device. */
-#define NFC_DN_IF_OUT		0x0008
-
 /* DECnet Hooks */
 /* After promisc drops, checksum checks. */
 #define NF_DN_PRE_ROUTING	0
diff --git a/include/linux/netfilter_ipv4.h b/include/linux/netfilter_ipv4.h
--- a/include/linux/netfilter_ipv4.h
+++ b/include/linux/netfilter_ipv4.h
@@ -8,34 +8,6 @@
 #include <linux/config.h>
 #include <linux/netfilter.h>
 
-/* IP Cache bits. */
-/* Src IP address. */
-#define NFC_IP_SRC		0x0001
-/* Dest IP address. */
-#define NFC_IP_DST		0x0002
-/* Input device. */
-#define NFC_IP_IF_IN		0x0004
-/* Output device. */
-#define NFC_IP_IF_OUT		0x0008
-/* TOS. */
-#define NFC_IP_TOS		0x0010
-/* Protocol. */
-#define NFC_IP_PROTO		0x0020
-/* IP options. */
-#define NFC_IP_OPTIONS		0x0040
-/* Frag & flags. */
-#define NFC_IP_FRAG		0x0080
-
-/* Per-protocol information: only matters if proto match. */
-/* TCP flags. */
-#define NFC_IP_TCPFLAGS		0x0100
-/* Source port. */
-#define NFC_IP_SRC_PT		0x0200
-/* Dest port. */
-#define NFC_IP_DST_PT		0x0400
-/* Something else about the proto */
-#define NFC_IP_PROTO_UNKNOWN	0x2000
-
 /* IP Hooks */
 /* After promisc drops, checksum checks. */
 #define NF_IP_PRE_ROUTING	0
diff --git a/include/linux/netfilter_ipv6.h b/include/linux/netfilter_ipv6.h
--- a/include/linux/netfilter_ipv6.h
+++ b/include/linux/netfilter_ipv6.h
@@ -10,36 +10,6 @@
 
 #include <linux/netfilter.h>
 
-/* IP Cache bits. */
-/* Src IP address. */
-#define NFC_IP6_SRC              0x0001
-/* Dest IP address. */
-#define NFC_IP6_DST              0x0002
-/* Input device. */
-#define NFC_IP6_IF_IN            0x0004
-/* Output device. */
-#define NFC_IP6_IF_OUT           0x0008
-/* TOS. */
-#define NFC_IP6_TOS              0x0010
-/* Protocol. */
-#define NFC_IP6_PROTO            0x0020
-/* IP options. */
-#define NFC_IP6_OPTIONS          0x0040
-/* Frag & flags. */
-#define NFC_IP6_FRAG             0x0080
-
-
-/* Per-protocol information: only matters if proto match. */
-/* TCP flags. */
-#define NFC_IP6_TCPFLAGS         0x0100
-/* Source port. */
-#define NFC_IP6_SRC_PT           0x0200
-/* Dest port. */
-#define NFC_IP6_DST_PT           0x0400
-/* Something else about the proto */
-#define NFC_IP6_PROTO_UNKNOWN    0x2000
-
-
 /* IP6 Hooks */
 /* After promisc drops, checksum checks. */
 #define NF_IP6_PRE_ROUTING	0
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -190,7 +190,6 @@ struct skb_shared_info {
  *	@end: End pointer
  *	@destructor: Destruct function
  *	@nfmark: Can be used for communication between hooks
- *	@nfcache: Cache info
  *	@nfct: Associated connection, if any
  *	@nfctinfo: Relationship of this skb to the connection
  *	@nf_bridge: Saved data about a bridged frame - see br_netfilter.c
@@ -252,17 +251,18 @@ struct sk_buff {
 	__u8			local_df:1,
 				cloned:1,
 				ip_summed:2,
-				nohdr:1;
-				/* 3 bits spare */
+				nohdr:1,
+				nfctinfo:3;
 	__u8			pkt_type;
 	__u16			protocol;
 
 	void			(*destructor)(struct sk_buff *skb);
 #ifdef CONFIG_NETFILTER
 	__u32			nfmark;
-	__u32			nfcache;
-	__u32			nfctinfo;
 	struct nf_conntrack	*nfct;
+#if defined(CONFIG_IP_VS) || defined(CONFIG_IP_VS_MODULE)
+	__u8			ipvs_property:1;
+#endif
 #ifdef CONFIG_BRIDGE_NETFILTER
 	struct nf_bridge_info	*nf_bridge;
 #endif
diff --git a/net/bridge/netfilter/ebt_mark.c b/net/bridge/netfilter/ebt_mark.c
--- a/net/bridge/netfilter/ebt_mark.c
+++ b/net/bridge/netfilter/ebt_mark.c
@@ -23,10 +23,9 @@ static int ebt_target_mark(struct sk_buf
 {
 	struct ebt_mark_t_info *info = (struct ebt_mark_t_info *)data;
 
-	if ((*pskb)->nfmark != info->mark) {
+	if ((*pskb)->nfmark != info->mark)
 		(*pskb)->nfmark = info->mark;
-		(*pskb)->nfcache |= NFC_ALTERED;
-	}
+
 	return info->target;
 }
 
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -361,7 +361,6 @@ struct sk_buff *skb_clone(struct sk_buff
 	n->destructor = NULL;
 #ifdef CONFIG_NETFILTER
 	C(nfmark);
-	C(nfcache);
 	C(nfct);
 	nf_conntrack_get(skb->nfct);
 	C(nfctinfo);
@@ -424,7 +423,6 @@ static void copy_skb_header(struct sk_bu
 	new->destructor = NULL;
 #ifdef CONFIG_NETFILTER
 	new->nfmark	= old->nfmark;
-	new->nfcache	= old->nfcache;
 	new->nfct	= old->nfct;
 	nf_conntrack_get(old->nfct);
 	new->nfctinfo	= old->nfctinfo;
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -392,7 +392,6 @@ static void ip_copy_metadata(struct sk_b
 #endif
 #ifdef CONFIG_NETFILTER
 	to->nfmark = from->nfmark;
-	to->nfcache = from->nfcache;
 	/* Connection association is same as pre-frag packet */
 	nf_conntrack_put(to->nfct);
 	to->nfct = from->nfct;
diff --git a/net/ipv4/ipvs/ip_vs_core.c b/net/ipv4/ipvs/ip_vs_core.c
--- a/net/ipv4/ipvs/ip_vs_core.c
+++ b/net/ipv4/ipvs/ip_vs_core.c
@@ -22,6 +22,7 @@
  *
  * Changes:
  *	Paul `Rusty' Russell		properly handle non-linear skbs
+ *	Harald Welte			don't use nfcache
  *
  */
 
@@ -529,7 +530,7 @@ static unsigned int ip_vs_post_routing(u
 				       const struct net_device *out,
 				       int (*okfn)(struct sk_buff *))
 {
-	if (!((*pskb)->nfcache & NFC_IPVS_PROPERTY))
+	if (!((*pskb)->ipvs_property))
 		return NF_ACCEPT;
 
 	/* The packet was sent from IPVS, exit this chain */
@@ -701,7 +702,7 @@ static int ip_vs_out_icmp(struct sk_buff
 	/* do the statistics and put it back */
 	ip_vs_out_stats(cp, skb);
 
-	skb->nfcache |= NFC_IPVS_PROPERTY;
+	skb->ipvs_property = 1;
 	verdict = NF_ACCEPT;
 
   out:
@@ -739,7 +740,7 @@ ip_vs_out(unsigned int hooknum, struct s
 
 	EnterFunction(11);
 
-	if (skb->nfcache & NFC_IPVS_PROPERTY)
+	if (skb->ipvs_property)
 		return NF_ACCEPT;
 
 	iph = skb->nh.iph;
@@ -821,7 +822,7 @@ ip_vs_out(unsigned int hooknum, struct s
 	ip_vs_set_state(cp, IP_VS_DIR_OUTPUT, skb, pp);
 	ip_vs_conn_put(cp);
 
-	skb->nfcache |= NFC_IPVS_PROPERTY;
+	skb->ipvs_property = 1;
 
 	LeaveFunction(11);
 	return NF_ACCEPT;
diff --git a/net/ipv4/ipvs/ip_vs_xmit.c b/net/ipv4/ipvs/ip_vs_xmit.c
--- a/net/ipv4/ipvs/ip_vs_xmit.c
+++ b/net/ipv4/ipvs/ip_vs_xmit.c
@@ -127,7 +127,7 @@ ip_vs_dst_reset(struct ip_vs_dest *dest)
 
 #define IP_VS_XMIT(skb, rt)				\
 do {							\
-	(skb)->nfcache |= NFC_IPVS_PROPERTY;		\
+	(skb)->ipvs_property = 1;			\
 	(skb)->ip_summed = CHECKSUM_NONE;		\
 	NF_HOOK(PF_INET, NF_IP_LOCAL_OUT, (skb), NULL,	\
 		(rt)->u.dst.dev, dst_output);		\
diff --git a/net/ipv4/netfilter/ip_conntrack_core.c b/net/ipv4/netfilter/ip_conntrack_core.c
--- a/net/ipv4/netfilter/ip_conntrack_core.c
+++ b/net/ipv4/netfilter/ip_conntrack_core.c
@@ -627,9 +627,6 @@ unsigned int ip_conntrack_in(unsigned in
 		return NF_DROP;
 	}
 
-	/* FIXME: Do this right please. --RR */
-	(*pskb)->nfcache |= NFC_UNKNOWN;
-
 /* Doesn't cover locally-generated broadcast, so not worth it. */
 #if 0
 	/* Ignore broadcast: no `connection'. */
@@ -942,10 +939,8 @@ ip_ct_gather_frags(struct sk_buff *skb, 
 	skb = ip_defrag(skb, user);
 	local_bh_enable();
 
-	if (skb) {
+	if (skb)
 		ip_send_check(skb->nh.iph);
-		skb->nfcache |= NFC_ALTERED;
-	}
 	return skb;
 }
 
diff --git a/net/ipv4/netfilter/ip_nat_core.c b/net/ipv4/netfilter/ip_nat_core.c
--- a/net/ipv4/netfilter/ip_nat_core.c
+++ b/net/ipv4/netfilter/ip_nat_core.c
@@ -321,7 +321,6 @@ manip_pkt(u_int16_t proto,
 {
 	struct iphdr *iph;
 
-	(*pskb)->nfcache |= NFC_ALTERED;
 	if (!skb_ip_make_writable(pskb, iphdroff + sizeof(*iph)))
 		return 0;
 
diff --git a/net/ipv4/netfilter/ip_nat_standalone.c b/net/ipv4/netfilter/ip_nat_standalone.c
--- a/net/ipv4/netfilter/ip_nat_standalone.c
+++ b/net/ipv4/netfilter/ip_nat_standalone.c
@@ -73,8 +73,6 @@ ip_nat_fn(unsigned int hooknum,
 	IP_NF_ASSERT(!((*pskb)->nh.iph->frag_off
 		       & htons(IP_MF|IP_OFFSET)));
 
-	(*pskb)->nfcache |= NFC_UNKNOWN;
-
 	/* If we had a hardware checksum before, it's now invalid */
 	if ((*pskb)->ip_summed == CHECKSUM_HW)
 		if (skb_checksum_help(*pskb, (out == NULL)))
diff --git a/net/ipv4/netfilter/ip_queue.c b/net/ipv4/netfilter/ip_queue.c
--- a/net/ipv4/netfilter/ip_queue.c
+++ b/net/ipv4/netfilter/ip_queue.c
@@ -385,7 +385,6 @@ ipq_mangle_ipv4(ipq_verdict_msg_t *v, st
 	if (!skb_ip_make_writable(&e->skb, v->data_len))
 		return -ENOMEM;
 	memcpy(e->skb->data, v->payload, v->data_len);
-	e->skb->nfcache |= NFC_ALTERED;
 
 	/*
 	 * Extra routing may needed on local out, as the QUEUE target never
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -312,7 +312,6 @@ ipt_do_table(struct sk_buff **pskb,
 	do {
 		IP_NF_ASSERT(e);
 		IP_NF_ASSERT(back);
-		(*pskb)->nfcache |= e->nfcache;
 		if (ip_packet_match(ip, indev, outdev, &e->ip, offset)) {
 			struct ipt_entry_target *t;
 
diff --git a/net/ipv4/netfilter/ipt_CLASSIFY.c b/net/ipv4/netfilter/ipt_CLASSIFY.c
--- a/net/ipv4/netfilter/ipt_CLASSIFY.c
+++ b/net/ipv4/netfilter/ipt_CLASSIFY.c
@@ -32,10 +32,8 @@ target(struct sk_buff **pskb,
 {
 	const struct ipt_classify_target_info *clinfo = targinfo;
 
-	if((*pskb)->priority != clinfo->priority) {
+	if((*pskb)->priority != clinfo->priority) 
 		(*pskb)->priority = clinfo->priority;
-		(*pskb)->nfcache |= NFC_ALTERED;
-	}
 
 	return IPT_CONTINUE;
 }
diff --git a/net/ipv4/netfilter/ipt_CONNMARK.c b/net/ipv4/netfilter/ipt_CONNMARK.c
--- a/net/ipv4/netfilter/ipt_CONNMARK.c
+++ b/net/ipv4/netfilter/ipt_CONNMARK.c
@@ -61,10 +61,8 @@ target(struct sk_buff **pskb,
 	    case IPT_CONNMARK_RESTORE:
 		nfmark = (*pskb)->nfmark;
 		diff = (ct->mark ^ nfmark) & markinfo->mask;
-		if (diff != 0) {
+		if (diff != 0)
 		    (*pskb)->nfmark = nfmark ^ diff;
-		    (*pskb)->nfcache |= NFC_ALTERED;
-		}
 		break;
 	    }
 	}
diff --git a/net/ipv4/netfilter/ipt_DSCP.c b/net/ipv4/netfilter/ipt_DSCP.c
--- a/net/ipv4/netfilter/ipt_DSCP.c
+++ b/net/ipv4/netfilter/ipt_DSCP.c
@@ -51,7 +51,6 @@ target(struct sk_buff **pskb,
 						 sizeof(diffs),
 						 (*pskb)->nh.iph->check
 						 ^ 0xFFFF));
-		(*pskb)->nfcache |= NFC_ALTERED;
 	}
 	return IPT_CONTINUE;
 }
diff --git a/net/ipv4/netfilter/ipt_ECN.c b/net/ipv4/netfilter/ipt_ECN.c
--- a/net/ipv4/netfilter/ipt_ECN.c
+++ b/net/ipv4/netfilter/ipt_ECN.c
@@ -43,7 +43,6 @@ set_ect_ip(struct sk_buff **pskb, const 
 						 sizeof(diffs),
 						 (*pskb)->nh.iph->check
 						 ^0xFFFF));
-		(*pskb)->nfcache |= NFC_ALTERED;
 	} 
 	return 1;
 }
@@ -86,7 +85,6 @@ set_ect_tcp(struct sk_buff **pskb, const
 	else
 		if (skb_checksum_help(*pskb, inward))
 			return 0;
-	(*pskb)->nfcache |= NFC_ALTERED;
 	return 1;
 }
 
diff --git a/net/ipv4/netfilter/ipt_MARK.c b/net/ipv4/netfilter/ipt_MARK.c
--- a/net/ipv4/netfilter/ipt_MARK.c
+++ b/net/ipv4/netfilter/ipt_MARK.c
@@ -29,10 +29,9 @@ target_v0(struct sk_buff **pskb,
 {
 	const struct ipt_mark_target_info *markinfo = targinfo;
 
-	if((*pskb)->nfmark != markinfo->mark) {
+	if((*pskb)->nfmark != markinfo->mark)
 		(*pskb)->nfmark = markinfo->mark;
-		(*pskb)->nfcache |= NFC_ALTERED;
-	}
+
 	return IPT_CONTINUE;
 }
 
@@ -61,10 +60,9 @@ target_v1(struct sk_buff **pskb,
 		break;
 	}
 
-	if((*pskb)->nfmark != mark) {
+	if((*pskb)->nfmark != mark)
 		(*pskb)->nfmark = mark;
-		(*pskb)->nfcache |= NFC_ALTERED;
-	}
+
 	return IPT_CONTINUE;
 }
 
diff --git a/net/ipv4/netfilter/ipt_REJECT.c b/net/ipv4/netfilter/ipt_REJECT.c
--- a/net/ipv4/netfilter/ipt_REJECT.c
+++ b/net/ipv4/netfilter/ipt_REJECT.c
@@ -156,7 +156,6 @@ static void send_reset(struct sk_buff *o
 
 	/* This packet will not be the same as the other: clear nf fields */
 	nf_reset(nskb);
-	nskb->nfcache = 0;
 	nskb->nfmark = 0;
 #ifdef CONFIG_BRIDGE_NETFILTER
 	nf_bridge_put(nskb->nf_bridge);
diff --git a/net/ipv4/netfilter/ipt_TCPMSS.c b/net/ipv4/netfilter/ipt_TCPMSS.c
--- a/net/ipv4/netfilter/ipt_TCPMSS.c
+++ b/net/ipv4/netfilter/ipt_TCPMSS.c
@@ -189,7 +189,6 @@ ipt_tcpmss_target(struct sk_buff **pskb,
 	/* We never hw checksum SYN packets.  */
 	BUG_ON((*pskb)->ip_summed == CHECKSUM_HW);
 
-	(*pskb)->nfcache |= NFC_UNKNOWN | NFC_ALTERED;
 	return IPT_CONTINUE;
 }
 
diff --git a/net/ipv4/netfilter/ipt_TOS.c b/net/ipv4/netfilter/ipt_TOS.c
--- a/net/ipv4/netfilter/ipt_TOS.c
+++ b/net/ipv4/netfilter/ipt_TOS.c
@@ -46,7 +46,6 @@ target(struct sk_buff **pskb,
 						 sizeof(diffs),
 						 (*pskb)->nh.iph->check
 						 ^0xFFFF));
-		(*pskb)->nfcache |= NFC_ALTERED;
 	}
 	return IPT_CONTINUE;
 }
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -185,19 +185,6 @@ int ip6_route_me_harder(struct sk_buff *
 }
 #endif
 
-static inline int ip6_maybe_reroute(struct sk_buff *skb)
-{
-#ifdef CONFIG_NETFILTER
-	if (skb->nfcache & NFC_ALTERED){
-		if (ip6_route_me_harder(skb) != 0){
-			kfree_skb(skb);
-			return -EINVAL;
-		}
-	}
-#endif /* CONFIG_NETFILTER */
-	return dst_output(skb);
-}
-
 /*
  *	xmit an sk_buff (used by TCP)
  */
@@ -266,7 +253,8 @@ int ip6_xmit(struct sock *sk, struct sk_
 	mtu = dst_mtu(dst);
 	if ((skb->len <= mtu) || ipfragok) {
 		IP6_INC_STATS(IPSTATS_MIB_OUTREQUESTS);
-		return NF_HOOK(PF_INET6, NF_IP6_LOCAL_OUT, skb, NULL, dst->dev, ip6_maybe_reroute);
+		return NF_HOOK(PF_INET6, NF_IP6_LOCAL_OUT, skb, NULL, dst->dev,
+				dst_output);
 	}
 
 	if (net_ratelimit())
diff --git a/net/ipv6/netfilter/ip6_queue.c b/net/ipv6/netfilter/ip6_queue.c
--- a/net/ipv6/netfilter/ip6_queue.c
+++ b/net/ipv6/netfilter/ip6_queue.c
@@ -379,7 +379,6 @@ ipq_mangle_ipv6(ipq_verdict_msg_t *v, st
 	if (!skb_ip_make_writable(&e->skb, v->data_len))
 		return -ENOMEM;
 	memcpy(e->skb->data, v->payload, v->data_len);
-	e->skb->nfcache |= NFC_ALTERED;
 
 	/*
 	 * Extra routing may needed on local out, as the QUEUE target never
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -401,7 +401,6 @@ ip6t_do_table(struct sk_buff **pskb,
 	do {
 		IP_NF_ASSERT(e);
 		IP_NF_ASSERT(back);
-		(*pskb)->nfcache |= e->nfcache;
 		if (ip6_packet_match(*pskb, indev, outdev, &e->ipv6,
 			&protoff, &offset)) {
 			struct ip6t_entry_target *t;
diff --git a/net/ipv6/netfilter/ip6t_MARK.c b/net/ipv6/netfilter/ip6t_MARK.c
--- a/net/ipv6/netfilter/ip6t_MARK.c
+++ b/net/ipv6/netfilter/ip6t_MARK.c
@@ -28,10 +28,9 @@ target(struct sk_buff **pskb,
 {
 	const struct ip6t_mark_target_info *markinfo = targinfo;
 
-	if((*pskb)->nfmark != markinfo->mark) {
+	if((*pskb)->nfmark != markinfo->mark)
 		(*pskb)->nfmark = markinfo->mark;
-		(*pskb)->nfcache |= NFC_ALTERED;
-	}
+
 	return IP6T_CONTINUE;
 }
 
-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] reduce netfilte sk_buff enlargement

[David S. Miller]

From: Harald Welte <laforge@netfilter.org>
Date: Mon, 18 Jul 2005 00:04:51 +0200

> The only real in-tree user of nfcache was IPVS, who only needs a single
> bit.  Unfortunately I couldn't find some other free bit in sk_buff to
> stuff that bit into, so I introduced a separate field for them.  Maybe
> the IPVS guys can resolve that to further save space.

I think we must resolve this one before 2.6.14 goes out, which
gives us a lot of time, but for now I'll eat that one-bit member.

> Initially I wanted to shrink pkt_type to three bits (PACKET_HOST and
> alike are only 6 values defined), but unfortunately the bluetooth code
> overloads pkt_type :(

This also must be cured somehow, that really isn't a clean nor nice
usage of this field.

> - remove all never-implemented 'nfcache' code
> - don't have ipvs code abuse 'nfcache' field. currently get's their own
>   compile-conditional skb->ipvs_property field.  IPVS maintainers can
>   decide to move this bit elswhere, but nfcache needs to die.
> - remove skb->nfcache field to save 4 bytes
> - move skb->nfctinfo into three unused bits to save further 4 bytes
> 
> Signed-off-by: Harald Welte <laforge@netfilter.org>

Applied, thanks Harald.

Re: [PATCH] reduce netfilte sk_buff enlargement

[Jan Engelhardt]

>but for now I'll eat that one-bit member.

What is more important? Being as small as possible using bitfields, or being 
as fast as possible? (Usage of bitfields is some CPU overhead for their 
extraction)

Re: [PATCH] reduce netfilte sk_buff enlargement

[David S. Miller]

From: Jan Engelhardt <jengelh@linux01.gwdg.de>
Date: Tue, 19 Jul 2005 09:18:38 +0200 (MEST)

> >but for now I'll eat that one-bit member.
> 
> What is more important? Being as small as possible using bitfields, or being 
> as fast as possible? (Usage of bitfields is some CPU overhead for their 
> extraction)

I'm conjuring that we can store the state elsewhere, for example
in the SKB ->cb[] control block.  But that requires some verifications.

Memory access overhead dwarfs whatever the cpu has to do to extract
the bits.

Re: [PATCH] reduce netfilte sk_buff enlargement

[Harald Welte]

[-- Attachment #1: Type: text/plain, Size: 1637 bytes --]

On Mon, Jul 18, 2005 at 08:31:45PM -0700, David S. Miller wrote:
> From: Harald Welte <laforge@netfilter.org>
> Date: Mon, 18 Jul 2005 00:04:51 +0200
> 
> > The only real in-tree user of nfcache was IPVS, who only needs a single
> > bit.  Unfortunately I couldn't find some other free bit in sk_buff to
> > stuff that bit into, so I introduced a separate field for them.  Maybe
> > the IPVS guys can resolve that to further save space.
> 
> I think we must resolve this one before 2.6.14 goes out, which
> gives us a lot of time, but for now I'll eat that one-bit member.

Well, I hope IPVS people will take care of this.  I don't really know
that code too well...

> > Initially I wanted to shrink pkt_type to three bits (PACKET_HOST and
> > alike are only 6 values defined), but unfortunately the bluetooth code
> > overloads pkt_type :(
> 
> This also must be cured somehow, that really isn't a clean nor nice
> usage of this field.

I just ran into Marcel Holtmann earlier today.  He thinks moving that
data into the cb is fine, though he has to double-check that.

He also said that he really only needs 5 bits, so even if the current
pkt_type overloading would persist, we could probably shrink it to make
space for the IPVS bit.

-- 
- Harald Welte <laforge@netfilter.org>                 http://netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] reduce netfilte sk_buff enlargement

[Wensong Zhang]

Hi,

Sorry for the delay.

On Wed, 20 Jul 2005, Harald Welte wrote:

> On Mon, Jul 18, 2005 at 08:31:45PM -0700, David S. Miller wrote:
>> From: Harald Welte <laforge@netfilter.org>
>> Date: Mon, 18 Jul 2005 00:04:51 +0200
>>
>>> The only real in-tree user of nfcache was IPVS, who only needs a single
>>> bit.  Unfortunately I couldn't find some other free bit in sk_buff to
>>> stuff that bit into, so I introduced a separate field for them.  Maybe
>>> the IPVS guys can resolve that to further save space.
>>
>> I think we must resolve this one before 2.6.14 goes out, which
>> gives us a lot of time, but for now I'll eat that one-bit member.
>
> Well, I hope IPVS people will take care of this.  I don't really know
> that code too well...
>

This bit is only to indicate that the sk_buff is already mangled by 
IPVS/NAT, so that when both iptables/NAT and IPVS/NAT are enabled, 
iptables/NAT will not mangle sk_buff again. I am not sure if there is more 
elegant way to work around this issue, will investigate it.

>>> Initially I wanted to shrink pkt_type to three bits (PACKET_HOST and
>>> alike are only 6 values defined), but unfortunately the bluetooth code
>>> overloads pkt_type :(
>>
>> This also must be cured somehow, that really isn't a clean nor nice
>> usage of this field.
>
> I just ran into Marcel Holtmann earlier today.  He thinks moving that
> data into the cb is fine, though he has to double-check that.
>
> He also said that he really only needs 5 bits, so even if the current
> pkt_type overloading would persist, we could probably shrink it to make
> space for the IPVS bit.
>

That's cool.

Thanks,

Wensong

Re: [PATCH] reduce netfilte sk_buff enlargement

[Patrick McHardy]

Wensong Zhang wrote:
>> Well, I hope IPVS people will take care of this.  I don't really know
>> that code too well...
>>
> This bit is only to indicate that the sk_buff is already mangled by 
> IPVS/NAT, so that when both iptables/NAT and IPVS/NAT are enabled, 
> iptables/NAT will not mangle sk_buff again. I am not sure if there is 
> more elegant way to work around this issue, will investigate it.

For new connections you could set the IPS_SRC_NAT_DONE and
IPS_DST_NAT_DONE bits in conntrack->status to avoid NAT setting up
new mappings. But this doesn't work if IPVS is loaded when NAT
has already set up the mappings. In this case you could refuse
to NAT in IPVS.

Regards
Patrick

Re: [PATCH] reduce netfilte sk_buff enlargement

[David S. Miller]

From: Harald Welte <laforge@netfilter.org>
Date: Wed, 20 Jul 2005 09:23:05 -0400

> On Mon, Jul 18, 2005 at 08:31:45PM -0700, David S. Miller wrote:
> > From: Harald Welte <laforge@netfilter.org>
> > Date: Mon, 18 Jul 2005 00:04:51 +0200
> > 
> > > The only real in-tree user of nfcache was IPVS, who only needs a single
> > > bit.  Unfortunately I couldn't find some other free bit in sk_buff to
> > > stuff that bit into, so I introduced a separate field for them.  Maybe
> > > the IPVS guys can resolve that to further save space.
> > 
> > I think we must resolve this one before 2.6.14 goes out, which
> > gives us a lot of time, but for now I'll eat that one-bit member.
> 
> Well, I hope IPVS people will take care of this.  I don't really know
> that code too well...

Ok, I might take a look at this myself.

> > > Initially I wanted to shrink pkt_type to three bits (PACKET_HOST and
> > > alike are only 6 values defined), but unfortunately the bluetooth code
> > > overloads pkt_type :(
> > 
> > This also must be cured somehow, that really isn't a clean nor nice
> > usage of this field.
> 
> I just ran into Marcel Holtmann earlier today.  He thinks moving that
> data into the cb is fine, though he has to double-check that.
> 
> He also said that he really only needs 5 bits, so even if the current
> pkt_type overloading would persist, we could probably shrink it to make
> space for the IPVS bit.

Ok, sounds great.

Re: [PATCH] reduce netfilte sk_buff enlargement

[Marcel Holtmann]

Hi Harald,

> I just ran into Marcel Holtmann earlier today.  He thinks moving that
> data into the cb is fine, though he has to double-check that.
> 
> He also said that he really only needs 5 bits, so even if the current
> pkt_type overloading would persist, we could probably shrink it to make
> space for the IPVS bit.

please forget about the 5 bits thingy, I told you. Splitting it up into
bitlevel will screw up the HCI filter completly. However after a look
trough the Bluetooth core it should be quite easy too move the pkt_type
into the control buffer. We already use it for a direction bit. The
nasty thing is that I have to modify all the drivers. So when you
finally decided to shrink the pkt_type, I think that I can come up with
a patch for it quiet quickly.

Regards

Marcel

Re: [PATCH] reduce netfilte sk_buff enlargement

[David S. Miller]

From: Marcel Holtmann <marcel@holtmann.org>
Date: Thu, 21 Jul 2005 20:20:35 +0200

> However after a look trough the Bluetooth core it should be quite
> easy too move the pkt_type into the control buffer. We already use
> it for a direction bit. The nasty thing is that I have to modify all
> the drivers. So when you finally decided to shrink the pkt_type, I
> think that I can come up with a patch for it quiet quickly.

We are trimming SKB madly right now, so if you could work on
the bluetooth patch so we can trim the pkt_type size ASAP
that would be much appreciated.  You can send diffs against
my net-2.6.14 tree at:

   rsync://rsync.kernel.org/pub/scm/linux/kernel/git/davem/net-2.6.14.git

Thanks.

Re: [PATCH] reduce netfilte sk_buff enlargement

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

Hi Dave,

> > However after a look trough the Bluetooth core it should be quite
> > easy too move the pkt_type into the control buffer. We already use
> > it for a direction bit. The nasty thing is that I have to modify all
> > the drivers. So when you finally decided to shrink the pkt_type, I
> > think that I can come up with a patch for it quiet quickly.
> 
> We are trimming SKB madly right now, so if you could work on
> the bluetooth patch so we can trim the pkt_type size ASAP
> that would be much appreciated.

unfortunatly it is not that straight forward as I thought. The attached
patch which modifies the Bluetooth core and the hci_usb driver is not
working on my machine.

Regards

Marcel


[-- Attachment #2: patch --]
[-- Type: text/x-patch, Size: 7481 bytes --]

diff --git a/drivers/bluetooth/hci_usb.c b/drivers/bluetooth/hci_usb.c
--- a/drivers/bluetooth/hci_usb.c
+++ b/drivers/bluetooth/hci_usb.c
@@ -443,7 +443,7 @@ static int __tx_submit(struct hci_usb *h
 
 static inline int hci_usb_send_ctrl(struct hci_usb *husb, struct sk_buff *skb)
 {
-	struct _urb *_urb = __get_completed(husb, skb->pkt_type);
+	struct _urb *_urb = __get_completed(husb, bt_cb(skb)->pkt_type);
 	struct usb_ctrlrequest *dr;
 	struct urb *urb;
 
@@ -451,7 +451,7 @@ static inline int hci_usb_send_ctrl(stru
 		_urb = _urb_alloc(0, GFP_ATOMIC);
 		if (!_urb)
 			return -ENOMEM;
-		_urb->type = skb->pkt_type;
+		_urb->type = bt_cb(skb)->pkt_type;
 
 		dr = kmalloc(sizeof(*dr), GFP_ATOMIC);
 		if (!dr) {
@@ -479,7 +479,7 @@ static inline int hci_usb_send_ctrl(stru
 
 static inline int hci_usb_send_bulk(struct hci_usb *husb, struct sk_buff *skb)
 {
-	struct _urb *_urb = __get_completed(husb, skb->pkt_type);
+	struct _urb *_urb = __get_completed(husb, bt_cb(skb)->pkt_type);
 	struct urb *urb;
 	int pipe;
 
@@ -487,7 +487,7 @@ static inline int hci_usb_send_bulk(stru
 		_urb = _urb_alloc(0, GFP_ATOMIC);
 		if (!_urb)
 			return -ENOMEM;
-		_urb->type = skb->pkt_type;
+		_urb->type = bt_cb(skb)->pkt_type;
 	}
 
 	urb  = &_urb->urb;
@@ -505,14 +505,14 @@ static inline int hci_usb_send_bulk(stru
 #ifdef CONFIG_BT_HCIUSB_SCO
 static inline int hci_usb_send_isoc(struct hci_usb *husb, struct sk_buff *skb)
 {
-	struct _urb *_urb = __get_completed(husb, skb->pkt_type);
+	struct _urb *_urb = __get_completed(husb, bt_cb(skb)->pkt_type);
 	struct urb *urb;
 
 	if (!_urb) {
 		_urb = _urb_alloc(HCI_MAX_ISOC_FRAMES, GFP_ATOMIC);
 		if (!_urb)
 			return -ENOMEM;
-		_urb->type = skb->pkt_type;
+		_urb->type = bt_cb(skb)->pkt_type;
 	}
 
 	BT_DBG("%s skb %p len %d", husb->hdev->name, skb, skb->len);
@@ -601,11 +601,11 @@ static int hci_usb_send_frame(struct sk_
 	if (!test_bit(HCI_RUNNING, &hdev->flags))
 		return -EBUSY;
 
-	BT_DBG("%s type %d len %d", hdev->name, skb->pkt_type, skb->len);
+	BT_DBG("%s type %d len %d", hdev->name, bt_cb(skb)->pkt_type, skb->len);
 
 	husb = (struct hci_usb *) hdev->driver_data;
 
-	switch (skb->pkt_type) {
+	switch (bt_cb(skb)->pkt_type) {
 	case HCI_COMMAND_PKT:
 		hdev->stat.cmd_tx++;
 		break;
@@ -627,7 +627,7 @@ static int hci_usb_send_frame(struct sk_
 
 	read_lock(&husb->completion_lock);
 
-	skb_queue_tail(__transmit_q(husb, skb->pkt_type), skb);
+	skb_queue_tail(__transmit_q(husb, bt_cb(skb)->pkt_type), skb);
 	hci_usb_tx_wakeup(husb);
 
 	read_unlock(&husb->completion_lock);
@@ -682,7 +682,7 @@ static inline int __recv_frame(struct hc
 				return -ENOMEM;
 			}
 			skb->dev = (void *) husb->hdev;
-			skb->pkt_type = type;
+			bt_cb(skb)->pkt_type = type;
 	
 			__reassembly(husb, type) = skb;
 
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -131,7 +131,8 @@ struct sock *bt_accept_dequeue(struct so
 
 /* Skb helpers */
 struct bt_skb_cb {
-	int incoming;
+	__u8 pkt_type;
+	__u8 incoming;
 };
 #define bt_cb(skb) ((struct bt_skb_cb *)(skb->cb)) 
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -191,7 +191,7 @@ static void hci_init_req(struct hci_dev 
 
 	/* Special commands */
 	while ((skb = skb_dequeue(&hdev->driver_init))) {
-		skb->pkt_type = HCI_COMMAND_PKT;
+		bt_cb(skb)->pkt_type = HCI_COMMAND_PKT;
 		skb->dev = (void *) hdev;
 		skb_queue_tail(&hdev->cmd_q, skb);
 		hci_sched_cmd(hdev);
@@ -995,7 +995,7 @@ static int hci_send_frame(struct sk_buff
 		return -ENODEV;
 	}
 
-	BT_DBG("%s type %d len %d", hdev->name, skb->pkt_type, skb->len);
+	BT_DBG("%s type %d len %d", hdev->name, bt_cb(skb)->pkt_type, skb->len);
 
 	if (atomic_read(&hdev->promisc)) {
 		/* Time stamp */
@@ -1034,7 +1034,7 @@ int hci_send_cmd(struct hci_dev *hdev, _
 
 	BT_DBG("skb len %d", skb->len);
 
-	skb->pkt_type = HCI_COMMAND_PKT;
+	bt_cb(skb)->pkt_type = HCI_COMMAND_PKT;
 	skb->dev = (void *) hdev;
 	skb_queue_tail(&hdev->cmd_q, skb);
 	hci_sched_cmd(hdev);
@@ -1081,7 +1081,7 @@ int hci_send_acl(struct hci_conn *conn, 
 	BT_DBG("%s conn %p flags 0x%x", hdev->name, conn, flags);
 
 	skb->dev = (void *) hdev;
-	skb->pkt_type = HCI_ACLDATA_PKT;
+	bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
 	hci_add_acl_hdr(skb, conn->handle, flags | ACL_START);
 
 	if (!(list = skb_shinfo(skb)->frag_list)) {
@@ -1103,7 +1103,7 @@ int hci_send_acl(struct hci_conn *conn, 
 			skb = list; list = list->next;
 			
 			skb->dev = (void *) hdev;
-			skb->pkt_type = HCI_ACLDATA_PKT;
+			bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
 			hci_add_acl_hdr(skb, conn->handle, flags | ACL_CONT);
 
 			BT_DBG("%s frag %p len %d", hdev->name, skb, skb->len);
@@ -1139,7 +1139,7 @@ int hci_send_sco(struct hci_conn *conn, 
 	memcpy(skb->h.raw, &hdr, HCI_SCO_HDR_SIZE);
 
 	skb->dev = (void *) hdev;
-	skb->pkt_type = HCI_SCODATA_PKT;
+	bt_cb(skb)->pkt_type = HCI_SCODATA_PKT;
 	skb_queue_tail(&conn->data_q, skb);
 	hci_sched_tx(hdev);
 	return 0;
@@ -1369,7 +1369,7 @@ void hci_rx_task(unsigned long arg)
 
 		if (test_bit(HCI_INIT, &hdev->flags)) {
 			/* Don't process data packets in this states. */
-			switch (skb->pkt_type) {
+			switch (bt_cb(skb)->pkt_type) {
 			case HCI_ACLDATA_PKT:
 			case HCI_SCODATA_PKT:
 				kfree_skb(skb);
@@ -1378,7 +1378,7 @@ void hci_rx_task(unsigned long arg)
 		}
 
 		/* Process frame */
-		switch (skb->pkt_type) {
+		switch (bt_cb(skb)->pkt_type) {
 		case HCI_EVENT_PKT:
 			hci_event_packet(hdev, skb);
 			break;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1086,7 +1086,7 @@ void hci_si_event(struct hci_dev *hdev, 
 	bt_cb(skb)->incoming = 1;
 	do_gettimeofday(&skb->stamp);
 
-	skb->pkt_type = HCI_EVENT_PKT;
+	bt_cb(skb)->pkt_type = HCI_EVENT_PKT;
 	skb->dev = (void *) hdev;
 	hci_send_to_sock(hdev, skb);
 	kfree_skb(skb);
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -110,11 +110,11 @@ void hci_send_to_sock(struct hci_dev *hd
 		/* Apply filter */
 		flt = &hci_pi(sk)->filter;
 
-		if (!test_bit((skb->pkt_type == HCI_VENDOR_PKT) ?
-				0 : (skb->pkt_type & HCI_FLT_TYPE_BITS), &flt->type_mask))
+		if (!test_bit((bt_cb(skb)->pkt_type == HCI_VENDOR_PKT) ?
+				0 : (bt_cb(skb)->pkt_type & HCI_FLT_TYPE_BITS), &flt->type_mask))
 			continue;
 
-		if (skb->pkt_type == HCI_EVENT_PKT) {
+		if (bt_cb(skb)->pkt_type == HCI_EVENT_PKT) {
 			register int evt = (*(__u8 *)skb->data & HCI_FLT_EVENT_BITS);
 
 			if (!hci_test_bit(evt, &flt->event_mask))
@@ -131,7 +131,7 @@ void hci_send_to_sock(struct hci_dev *hd
 			continue;
 
 		/* Put type byte before the data */
-		memcpy(skb_push(nskb, 1), &nskb->pkt_type, 1);
+		memcpy(skb_push(nskb, 1), &bt_cb(nskb)->pkt_type, 1);
 
 		if (sock_queue_rcv_skb(sk, nskb))
 			kfree_skb(nskb);
@@ -405,11 +405,11 @@ static int hci_sock_sendmsg(struct kiocb
 		goto drop;
 	}
 
-	skb->pkt_type = *((unsigned char *) skb->data);
+	bt_cb(skb)->pkt_type = *((unsigned char *) skb->data);
 	skb_pull(skb, 1);
 	skb->dev = (void *) hdev;
 
-	if (skb->pkt_type == HCI_COMMAND_PKT) {
+	if (bt_cb(skb)->pkt_type == HCI_COMMAND_PKT) {
 		u16 opcode = __le16_to_cpu(get_unaligned((u16 *)skb->data));
 		u16 ogf = hci_opcode_ogf(opcode);
 		u16 ocf = hci_opcode_ocf(opcode);

Re: [PATCH] reduce netfilte sk_buff enlargement

[David S. Miller]

From: Marcel Holtmann <marcel@holtmann.org>
Date: Thu, 21 Jul 2005 23:42:11 +0200

> unfortunatly it is not that straight forward as I thought. The attached
> patch which modifies the Bluetooth core and the hci_usb driver is not
> working on my machine.

Hmmm... I'll see if I can spot anything obvious in the patch.

Re: [PATCH] reduce netfilte sk_buff enlargement

[David S. Miller]

From: Marcel Holtmann <marcel@holtmann.org>
Date: Thu, 21 Jul 2005 23:42:11 +0200

> unfortunatly it is not that straight forward as I thought. The attached
> patch which modifies the Bluetooth core and the hci_usb driver is not
> working on my machine.

This probably has nothing to do with why the patch doesn't
work for you, but the transformation of "incoming" to a "u8"
from an "int" is not fully correct, because hci_sock.c
does this:

	put_cmsg(msg, SOL_HCI, HCI_CMSG_DIR, sizeof(int), &bt_cb(skb)->incoming);

I haven't found any other problems though...

Maybe the bluetooth code was somehow depending upon the initial
value of skb->pkt_type or something like that?

Re: [PATCH] reduce netfilte sk_buff enlargement

[Marcel Holtmann]

Hi Dave,

> > unfortunatly it is not that straight forward as I thought. The attached
> > patch which modifies the Bluetooth core and the hci_usb driver is not
> > working on my machine.
> 
> This probably has nothing to do with why the patch doesn't
> work for you, but the transformation of "incoming" to a "u8"
> from an "int" is not fully correct, because hci_sock.c
> does this:
> 
> 	put_cmsg(msg, SOL_HCI, HCI_CMSG_DIR, sizeof(int), &bt_cb(skb)->incoming);
> 
> I haven't found any other problems though...

actually I overlooked it. Thanks for spotting it. I fixed it by copying
it into an int first. However this is only used when you really request
the direction bit and this happens only with hcidump from userspace as
far as I know.

> Maybe the bluetooth code was somehow depending upon the initial
> value of skb->pkt_type or something like that?

The pkt_type zero is not a valid one. We only use 1-4 and 0xff. So this
can't be the problem. I assume that the cb is not copied from the driver
into the core at some point.

Regards

Marcel

Re: [PATCH] reduce netfilte sk_buff enlargement

[David S. Miller]

From: Marcel Holtmann <marcel@holtmann.org>
Date: Fri, 22 Jul 2005 01:49:51 +0200

> The pkt_type zero is not a valid one. We only use 1-4 and 0xff. So this
> can't be the problem. I assume that the cb is not copied from the driver
> into the core at some point.

All clones and copies of SKBs copy of the ->cb[] for you.
So perhaps something is spamming the ->cb[] between these
two places.

Re: [PATCH] reduce netfilte sk_buff enlargement

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]

Hi Dave,

> > The pkt_type zero is not a valid one. We only use 1-4 and 0xff. So this
> > can't be the problem. I assume that the cb is not copied from the driver
> > into the core at some point.
> 
> All clones and copies of SKBs copy of the ->cb[] for you.
> So perhaps something is spamming the ->cb[] between these
> two places.

I found the problem. The hci_usb is using the cb[] by itself and so
overwriting the pkt_type value. The attached patch works for me with the
hci_usb driver. However I haven't converted all other drivers and
checked them. This won't happen until I am back home, because I don't
have any of these devices with me around. However it looks like this
seems to work without any problems.

Regards

Marcel


[-- Attachment #2: patch --]
[-- Type: text/x-patch, Size: 8153 bytes --]

diff --git a/drivers/bluetooth/hci_usb.c b/drivers/bluetooth/hci_usb.c
--- a/drivers/bluetooth/hci_usb.c
+++ b/drivers/bluetooth/hci_usb.c
@@ -443,7 +443,7 @@ static int __tx_submit(struct hci_usb *h
 
 static inline int hci_usb_send_ctrl(struct hci_usb *husb, struct sk_buff *skb)
 {
-	struct _urb *_urb = __get_completed(husb, skb->pkt_type);
+	struct _urb *_urb = __get_completed(husb, bt_cb(skb)->pkt_type);
 	struct usb_ctrlrequest *dr;
 	struct urb *urb;
 
@@ -451,7 +451,7 @@ static inline int hci_usb_send_ctrl(stru
 		_urb = _urb_alloc(0, GFP_ATOMIC);
 		if (!_urb)
 			return -ENOMEM;
-		_urb->type = skb->pkt_type;
+		_urb->type = bt_cb(skb)->pkt_type;
 
 		dr = kmalloc(sizeof(*dr), GFP_ATOMIC);
 		if (!dr) {
@@ -479,7 +479,7 @@ static inline int hci_usb_send_ctrl(stru
 
 static inline int hci_usb_send_bulk(struct hci_usb *husb, struct sk_buff *skb)
 {
-	struct _urb *_urb = __get_completed(husb, skb->pkt_type);
+	struct _urb *_urb = __get_completed(husb, bt_cb(skb)->pkt_type);
 	struct urb *urb;
 	int pipe;
 
@@ -487,7 +487,7 @@ static inline int hci_usb_send_bulk(stru
 		_urb = _urb_alloc(0, GFP_ATOMIC);
 		if (!_urb)
 			return -ENOMEM;
-		_urb->type = skb->pkt_type;
+		_urb->type = bt_cb(skb)->pkt_type;
 	}
 
 	urb  = &_urb->urb;
@@ -505,14 +505,14 @@ static inline int hci_usb_send_bulk(stru
 #ifdef CONFIG_BT_HCIUSB_SCO
 static inline int hci_usb_send_isoc(struct hci_usb *husb, struct sk_buff *skb)
 {
-	struct _urb *_urb = __get_completed(husb, skb->pkt_type);
+	struct _urb *_urb = __get_completed(husb, bt_cb(skb)->pkt_type);
 	struct urb *urb;
 
 	if (!_urb) {
 		_urb = _urb_alloc(HCI_MAX_ISOC_FRAMES, GFP_ATOMIC);
 		if (!_urb)
 			return -ENOMEM;
-		_urb->type = skb->pkt_type;
+		_urb->type = bt_cb(skb)->pkt_type;
 	}
 
 	BT_DBG("%s skb %p len %d", husb->hdev->name, skb, skb->len);
@@ -601,11 +601,11 @@ static int hci_usb_send_frame(struct sk_
 	if (!test_bit(HCI_RUNNING, &hdev->flags))
 		return -EBUSY;
 
-	BT_DBG("%s type %d len %d", hdev->name, skb->pkt_type, skb->len);
+	BT_DBG("%s type %d len %d", hdev->name, bt_cb(skb)->pkt_type, skb->len);
 
 	husb = (struct hci_usb *) hdev->driver_data;
 
-	switch (skb->pkt_type) {
+	switch (bt_cb(skb)->pkt_type) {
 	case HCI_COMMAND_PKT:
 		hdev->stat.cmd_tx++;
 		break;
@@ -627,7 +627,7 @@ static int hci_usb_send_frame(struct sk_
 
 	read_lock(&husb->completion_lock);
 
-	skb_queue_tail(__transmit_q(husb, skb->pkt_type), skb);
+	skb_queue_tail(__transmit_q(husb, bt_cb(skb)->pkt_type), skb);
 	hci_usb_tx_wakeup(husb);
 
 	read_unlock(&husb->completion_lock);
@@ -682,7 +682,7 @@ static inline int __recv_frame(struct hc
 				return -ENOMEM;
 			}
 			skb->dev = (void *) husb->hdev;
-			skb->pkt_type = type;
+			bt_cb(skb)->pkt_type = type;
 	
 			__reassembly(husb, type) = skb;
 
@@ -702,6 +702,7 @@ static inline int __recv_frame(struct hc
 		if (!scb->expect) {
 			/* Complete frame */
 			__reassembly(husb, type) = NULL;
+			bt_cb(skb)->pkt_type = type;
 			hci_recv_frame(skb);
 		}
 
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -131,7 +131,8 @@ struct sock *bt_accept_dequeue(struct so
 
 /* Skb helpers */
 struct bt_skb_cb {
-	int incoming;
+	__u8 pkt_type;
+	__u8 incoming;
 };
 #define bt_cb(skb) ((struct bt_skb_cb *)(skb->cb)) 
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -191,7 +191,7 @@ static void hci_init_req(struct hci_dev 
 
 	/* Special commands */
 	while ((skb = skb_dequeue(&hdev->driver_init))) {
-		skb->pkt_type = HCI_COMMAND_PKT;
+		bt_cb(skb)->pkt_type = HCI_COMMAND_PKT;
 		skb->dev = (void *) hdev;
 		skb_queue_tail(&hdev->cmd_q, skb);
 		hci_sched_cmd(hdev);
@@ -995,7 +995,7 @@ static int hci_send_frame(struct sk_buff
 		return -ENODEV;
 	}
 
-	BT_DBG("%s type %d len %d", hdev->name, skb->pkt_type, skb->len);
+	BT_DBG("%s type %d len %d", hdev->name, bt_cb(skb)->pkt_type, skb->len);
 
 	if (atomic_read(&hdev->promisc)) {
 		/* Time stamp */
@@ -1034,7 +1034,7 @@ int hci_send_cmd(struct hci_dev *hdev, _
 
 	BT_DBG("skb len %d", skb->len);
 
-	skb->pkt_type = HCI_COMMAND_PKT;
+	bt_cb(skb)->pkt_type = HCI_COMMAND_PKT;
 	skb->dev = (void *) hdev;
 	skb_queue_tail(&hdev->cmd_q, skb);
 	hci_sched_cmd(hdev);
@@ -1081,7 +1081,7 @@ int hci_send_acl(struct hci_conn *conn, 
 	BT_DBG("%s conn %p flags 0x%x", hdev->name, conn, flags);
 
 	skb->dev = (void *) hdev;
-	skb->pkt_type = HCI_ACLDATA_PKT;
+	bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
 	hci_add_acl_hdr(skb, conn->handle, flags | ACL_START);
 
 	if (!(list = skb_shinfo(skb)->frag_list)) {
@@ -1103,7 +1103,7 @@ int hci_send_acl(struct hci_conn *conn, 
 			skb = list; list = list->next;
 			
 			skb->dev = (void *) hdev;
-			skb->pkt_type = HCI_ACLDATA_PKT;
+			bt_cb(skb)->pkt_type = HCI_ACLDATA_PKT;
 			hci_add_acl_hdr(skb, conn->handle, flags | ACL_CONT);
 
 			BT_DBG("%s frag %p len %d", hdev->name, skb, skb->len);
@@ -1139,7 +1139,7 @@ int hci_send_sco(struct hci_conn *conn, 
 	memcpy(skb->h.raw, &hdr, HCI_SCO_HDR_SIZE);
 
 	skb->dev = (void *) hdev;
-	skb->pkt_type = HCI_SCODATA_PKT;
+	bt_cb(skb)->pkt_type = HCI_SCODATA_PKT;
 	skb_queue_tail(&conn->data_q, skb);
 	hci_sched_tx(hdev);
 	return 0;
@@ -1369,7 +1369,7 @@ void hci_rx_task(unsigned long arg)
 
 		if (test_bit(HCI_INIT, &hdev->flags)) {
 			/* Don't process data packets in this states. */
-			switch (skb->pkt_type) {
+			switch (bt_cb(skb)->pkt_type) {
 			case HCI_ACLDATA_PKT:
 			case HCI_SCODATA_PKT:
 				kfree_skb(skb);
@@ -1378,7 +1378,7 @@ void hci_rx_task(unsigned long arg)
 		}
 
 		/* Process frame */
-		switch (skb->pkt_type) {
+		switch (bt_cb(skb)->pkt_type) {
 		case HCI_EVENT_PKT:
 			hci_event_packet(hdev, skb);
 			break;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1086,7 +1086,7 @@ void hci_si_event(struct hci_dev *hdev, 
 	bt_cb(skb)->incoming = 1;
 	do_gettimeofday(&skb->stamp);
 
-	skb->pkt_type = HCI_EVENT_PKT;
+	bt_cb(skb)->pkt_type = HCI_EVENT_PKT;
 	skb->dev = (void *) hdev;
 	hci_send_to_sock(hdev, skb);
 	kfree_skb(skb);
diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -110,11 +110,11 @@ void hci_send_to_sock(struct hci_dev *hd
 		/* Apply filter */
 		flt = &hci_pi(sk)->filter;
 
-		if (!test_bit((skb->pkt_type == HCI_VENDOR_PKT) ?
-				0 : (skb->pkt_type & HCI_FLT_TYPE_BITS), &flt->type_mask))
+		if (!test_bit((bt_cb(skb)->pkt_type == HCI_VENDOR_PKT) ?
+				0 : (bt_cb(skb)->pkt_type & HCI_FLT_TYPE_BITS), &flt->type_mask))
 			continue;
 
-		if (skb->pkt_type == HCI_EVENT_PKT) {
+		if (bt_cb(skb)->pkt_type == HCI_EVENT_PKT) {
 			register int evt = (*(__u8 *)skb->data & HCI_FLT_EVENT_BITS);
 
 			if (!hci_test_bit(evt, &flt->event_mask))
@@ -131,7 +131,7 @@ void hci_send_to_sock(struct hci_dev *hd
 			continue;
 
 		/* Put type byte before the data */
-		memcpy(skb_push(nskb, 1), &nskb->pkt_type, 1);
+		memcpy(skb_push(nskb, 1), &bt_cb(nskb)->pkt_type, 1);
 
 		if (sock_queue_rcv_skb(sk, nskb))
 			kfree_skb(nskb);
@@ -327,8 +327,10 @@ static inline void hci_sock_cmsg(struct 
 {
 	__u32 mask = hci_pi(sk)->cmsg_mask;
 
-	if (mask & HCI_CMSG_DIR)
-		put_cmsg(msg, SOL_HCI, HCI_CMSG_DIR, sizeof(int), &bt_cb(skb)->incoming);
+	if (mask & HCI_CMSG_DIR) {
+		int incoming = bt_cb(skb)->incoming;
+		put_cmsg(msg, SOL_HCI, HCI_CMSG_DIR, sizeof(incoming), &incoming);
+	}
 
 	if (mask & HCI_CMSG_TSTAMP)
 		put_cmsg(msg, SOL_HCI, HCI_CMSG_TSTAMP, sizeof(skb->stamp), &skb->stamp);
@@ -405,11 +407,11 @@ static int hci_sock_sendmsg(struct kiocb
 		goto drop;
 	}
 
-	skb->pkt_type = *((unsigned char *) skb->data);
+	bt_cb(skb)->pkt_type = *((unsigned char *) skb->data);
 	skb_pull(skb, 1);
 	skb->dev = (void *) hdev;
 
-	if (skb->pkt_type == HCI_COMMAND_PKT) {
+	if (bt_cb(skb)->pkt_type == HCI_COMMAND_PKT) {
 		u16 opcode = __le16_to_cpu(get_unaligned((u16 *)skb->data));
 		u16 ogf = hci_opcode_ogf(opcode);
 		u16 ocf = hci_opcode_ocf(opcode);

Re: [PATCH] reduce netfilte sk_buff enlargement

[David S. Miller]

From: Marcel Holtmann <marcel@holtmann.org>
Date: Fri, 22 Jul 2005 02:26:34 +0200

> I found the problem. The hci_usb is using the cb[] by itself and so
> overwriting the pkt_type value. The attached patch works for me with the
> hci_usb driver. However I haven't converted all other drivers and
> checked them. This won't happen until I am back home, because I don't
> have any of these devices with me around. However it looks like this
> seems to work without any problems.

Great.  I'll wait until you get back and code up an updated
patch that takes care of all of the drivers.

Thanks.

Re: [PATCH] reduce netfilte sk_buff enlargement

[Marcel Holtmann]

Hi David,

> > I found the problem. The hci_usb is using the cb[] by itself and so
> > overwriting the pkt_type value. The attached patch works for me with the
> > hci_usb driver. However I haven't converted all other drivers and
> > checked them. This won't happen until I am back home, because I don't
> > have any of these devices with me around. However it looks like this
> > seems to work without any problems.
> 
> Great.  I'll wait until you get back and code up an updated
> patch that takes care of all of the drivers.

actually the patch is ready by now and hci_usb and hci_vhci are tested.
The tests for the other drivers have to wait at least until first week
of August. However since it seems to work even with 2.6.13-rc3, I don't
see any reason to wait with the inclusion. If you are serious with the
shrinking of the sk_buff, I can send you the final patch along with my
pending patches for 2.6.13.

Regards

Marcel

Re: [PATCH] reduce netfilte sk_buff enlargement

[David S. Miller]

From: Marcel Holtmann <marcel@holtmann.org>
Date: Sat, 23 Jul 2005 03:36:20 +0200

> If you are serious with the shrinking of the sk_buff, I can send you
> the final patch along with my pending patches for 2.6.13.

Please do.  The pkt_type change would get queued for 2.6.14,
and please only send real bug fixes when targetting for 2.6.13.

Thanks.

Re: [PATCH] reduce netfilte sk_buff enlargement

[Amin Azez]

David S. Miller wrote:
> This probably has nothing to do with why the patch doesn't
> work for you, but the transformation of "incoming" to a "u8"
> from an "int" is not fully correct, because hci_sock.c
> does this:
> 
> 	put_cmsg(msg, SOL_HCI, HCI_CMSG_DIR, sizeof(int), &bt_cb(skb)->incoming);
> 

These things always scare me, why do folk litter code with sly knowledge
of the size of struct members that may and do change?

Why don't people do:

put_cmsg(msg, SOL_HCI, HCI_CMSG_DIR, sizeof(bt_cb(skb)->incoming),
&bt_cb(skb)->incoming);

Or even better, use a macro to capture the semantics

#define put_struct_msg(msg, S, H, ITEM) \
 put_cmsg((msg),(S),(H),sizeof(ITEM),&(ITEM))

then do:
put_struct_cmsg(msg, SOL_HCI, HCI_CMSG_DIR, bt_cb(skb)->incoming);


It is true that macros can be over-used to create a meta-language and
uber-api, but this case saves no end of trouble, surely it is what
macros are for?

Sam