[PATCH] [NET] reduce sizeof(struct inet_peer), cleanup, change in peer_check_expire()

[PATCH] [NET] reduce sizeof(struct inet_peer), cleanup, change in peer_check_expire()

[Eric Dumazet]

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

Hi David

Please find this patch against include/net/inetpeer.h and net/ipv4/inetpeer.c

1) shrink struct inet_peer on 64 bits platforms.
------------------------------------------------
I noticed sizeof(struct inet_peer) was 64+8 on x86_64

As we dont really need 64 bits timestamps (we only care for garbage 
collection), we can use 32bits ones and reduce sizeof(struct inet_peer) to 64 
bytes : Because of SLAB_HWCACHE_ALIGN constraint, final allocation is 64 bytes 
instead of 128 bytes per inet_peer structure.

2) Cleanup
----------
  inet_putpeer() is not anymore inlined in inetpeer.h, as this is not called 
in fast paths, to reduce text size. Some exports are not anymore needed 
(inet_peer_unused_lock, inet_peer_unused_tailp) and can be declared static.

3) No more hard limit (PEER_MAX_CLEANUP_WORK = 30)
--------------------------------------------------
  peer_check_expire() try to delete entries for at most one timer tick. CPUS 
are going faster, hard limits are becoming useless... Similar thing is done in 
net/ipv4/route.c garbage collector.

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>


[-- Attachment #2: peer.patch --]
[-- Type: text/plain, Size: 3347 bytes --]

--- linux-2.6.18/include/net/inetpeer.h	Wed Sep 20 05:42:06 2006
+++ linux-2.6.18-ed/include/net/inetpeer.h	Thu Oct 12 21:40:28 2006
@@ -19,7 +19,7 @@
 {
 	struct inet_peer	*avl_left, *avl_right;
 	struct inet_peer	*unused_next, **unused_prevp;
-	unsigned long		dtime;		/* the time of last use of not
+	__u32			dtime;		/* the time of last use of not
 						 * referenced entries */
 	atomic_t		refcnt;
 	__u32			v4daddr;	/* peer's address */
@@ -35,21 +35,8 @@
 /* can be called with or without local BH being disabled */
 struct inet_peer	*inet_getpeer(__u32 daddr, int create);
 
-extern spinlock_t inet_peer_unused_lock;
-extern struct inet_peer **inet_peer_unused_tailp;
 /* can be called from BH context or outside */
-static inline void	inet_putpeer(struct inet_peer *p)
-{
-	spin_lock_bh(&inet_peer_unused_lock);
-	if (atomic_dec_and_test(&p->refcnt)) {
-		p->unused_prevp = inet_peer_unused_tailp;
-		p->unused_next = NULL;
-		*inet_peer_unused_tailp = p;
-		inet_peer_unused_tailp = &p->unused_next;
-		p->dtime = jiffies;
-	}
-	spin_unlock_bh(&inet_peer_unused_lock);
-}
+extern void inet_putpeer(struct inet_peer *p);
 
 extern spinlock_t inet_peer_idlock;
 /* can be called with or without local BH being disabled */
--- linux-2.6.18/net/ipv4/inetpeer.c	Wed Sep 20 05:42:06 2006
+++ linux-2.6.18-ed/net/ipv4/inetpeer.c	Thu Oct 12 21:55:23 2006
@@ -94,10 +94,8 @@
 int inet_peer_maxttl = 10 * 60 * HZ;	/* usual time to live: 10 min */
 
 static struct inet_peer *inet_peer_unused_head;
-/* Exported for inet_putpeer inline function.  */
-struct inet_peer **inet_peer_unused_tailp = &inet_peer_unused_head;
-DEFINE_SPINLOCK(inet_peer_unused_lock);
-#define PEER_MAX_CLEANUP_WORK 30
+static struct inet_peer **inet_peer_unused_tailp = &inet_peer_unused_head;
+static DEFINE_SPINLOCK(inet_peer_unused_lock);
 
 static void peer_check_expire(unsigned long dummy);
 static DEFINE_TIMER(peer_periodic_timer, peer_check_expire, 0, 0);
@@ -343,7 +341,8 @@
 	spin_lock_bh(&inet_peer_unused_lock);
 	p = inet_peer_unused_head;
 	if (p != NULL) {
-		if (time_after(p->dtime + ttl, jiffies)) {
+		__u32 delta = (__u32)jiffies - p->dtime;
+		if (delta < ttl) {
 			/* Do not prune fresh entries. */
 			spin_unlock_bh(&inet_peer_unused_lock);
 			return -1;
@@ -435,7 +434,7 @@
 /* Called with local BH disabled. */
 static void peer_check_expire(unsigned long dummy)
 {
-	int i;
+	unsigned long now = jiffies;
 	int ttl;
 
 	if (peer_total >= inet_peer_threshold)
@@ -444,7 +443,10 @@
 		ttl = inet_peer_maxttl
 				- (inet_peer_maxttl - inet_peer_minttl) / HZ *
 					peer_total / inet_peer_threshold * HZ;
-	for (i = 0; i < PEER_MAX_CLEANUP_WORK && !cleanup_once(ttl); i++);
+	while (!cleanup_once(ttl)) {
+		if (jiffies != now)
+			break;
+	}
 
 	/* Trigger the timer after inet_peer_gc_mintime .. inet_peer_gc_maxtime
 	 * interval depending on the total number of entries (more entries,
@@ -458,3 +460,16 @@
 				peer_total / inet_peer_threshold * HZ;
 	add_timer(&peer_periodic_timer);
 }
+
+void inet_putpeer(struct inet_peer *p)
+{
+	spin_lock_bh(&inet_peer_unused_lock);
+	if (atomic_dec_and_test(&p->refcnt)) {
+		p->unused_prevp = inet_peer_unused_tailp;
+		p->unused_next = NULL;
+		*inet_peer_unused_tailp = p;
+		inet_peer_unused_tailp = &p->unused_next;
+		p->dtime = (__u32)jiffies;
+	}
+	spin_unlock_bh(&inet_peer_unused_lock);
+}

Re: [PATCH] [NET] reduce sizeof(struct inet_peer), cleanup, change in peer_check_expire()

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Thu, 12 Oct 2006 22:14:12 +0200

> 1) shrink struct inet_peer on 64 bits platforms.
> ------------------------------------------------
> I noticed sizeof(struct inet_peer) was 64+8 on x86_64
> 
> As we dont really need 64 bits timestamps (we only care for garbage 
> collection), we can use 32bits ones and reduce sizeof(struct inet_peer) to 64 
> bytes : Because of SLAB_HWCACHE_ALIGN constraint, final allocation is 64 bytes 
> instead of 128 bytes per inet_peer structure.

I'm not convinced this is %100 correct.  There are wrapping
cases that I think aren't covered.

Consider an entry that lives long enough for the lower 32-bits
of jiffies to wrap, then we kill it, but we won't purge it
properly if the wrapped jiffie is close to dtime.

I'm sure there are other similar cases as well.

> 2) Cleanup
> ----------
>   inet_putpeer() is not anymore inlined in inetpeer.h, as this is not called 
> in fast paths, to reduce text size. Some exports are not anymore needed 
> (inet_peer_unused_lock, inet_peer_unused_tailp) and can be declared static.
> 
> 3) No more hard limit (PEER_MAX_CLEANUP_WORK = 30)
> --------------------------------------------------
>   peer_check_expire() try to delete entries for at most one timer tick. CPUS 
> are going faster, hard limits are becoming useless... Similar thing is done in 
> net/ipv4/route.c garbage collector.

These parts are fine.

Re: [PATCH] [NET] reduce sizeof(struct inet_peer), cleanup, change in peer_check_expire()

[Eric Dumazet]

David Miller a écrit :
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Thu, 12 Oct 2006 22:14:12 +0200
> 
>> 1) shrink struct inet_peer on 64 bits platforms.
>> ------------------------------------------------
>> I noticed sizeof(struct inet_peer) was 64+8 on x86_64
>>
>> As we dont really need 64 bits timestamps (we only care for garbage 
>> collection), we can use 32bits ones and reduce sizeof(struct inet_peer) to 64 
>> bytes : Because of SLAB_HWCACHE_ALIGN constraint, final allocation is 64 bytes 
>> instead of 128 bytes per inet_peer structure.
> 
> I'm not convinced this is %100 correct.  There are wrapping
> cases that I think aren't covered.
> 
> Consider an entry that lives long enough for the lower 32-bits
> of jiffies to wrap, then we kill it, but we won't purge it
> properly if the wrapped jiffie is close to dtime.
> 
> I'm sure there are other similar cases as well.

Hum, if it was incorrect, I urge you to grep for tcp_time_stamp, and correct 
this as soon as possible :)

2^31 is 2147483648

Thats a *lot* of timer ticks, an inet_peer entry should not stay in 
unused_list for more than 10 minutes.

Even if the system is under stress for more than 30 days, and some entries 
stay that long in unused list, they wont leak : either they are re-used, 
either they are purged by cleanup_once(0); done in inet_getpeer().

Eric

Re: [PATCH] [NET] reduce sizeof(struct inet_peer), cleanup, change in peer_check_expire()

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Fri, 13 Oct 2006 05:56:43 +0200

> 2^31 is 2147483648
> 
> Thats a *lot* of timer ticks, an inet_peer entry should not stay in 
> unused_list for more than 10 minutes.

My bad, I thought the time was compared to the creation time
not the time at which it was added to the unused list.

I like your patch and I'll apply it.

Thanks Eric.

Re: [PATCH] [NET] reduce sizeof(struct inet_peer), cleanup, change in peer_check_expire()

[Eric Dumazet]

David Miller a écrit :
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Fri, 13 Oct 2006 05:56:43 +0200
> 
>> 2^31 is 2147483648
>>
>> Thats a *lot* of timer ticks, an inet_peer entry should not stay in 
>> unused_list for more than 10 minutes.
> 
> My bad, I thought the time was compared to the creation time
> not the time at which it was added to the unused list.
> 
> I like your patch and I'll apply it.
> 
> Thanks Eric.

Thank you David

(Re-reading my previous mail, I forgot to say that on ia32, unsigned long is 
already 32 bits, so if a 32bits timestamp is OK (for delta computing) for such 
platforms, it must be  OK for other platforms as well)

Eric

[PATCH] [NET] reduce sizeof(struct flow)

[Eric Dumazet]

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

Hi David

Each route entry includes a 'struct flow'. This structure has a current size 
of 80 bytes. This patch makes a size reduction depending on 
CONFIG_IPV6/CONFIG_IPV6_MODULE/CONFIG_DECNET/CONFIG_IP_ROUTE_FWMARK

For a platform doing IPV4 only, the new size is 36 bytes (instead of 80)
As many routers are base on PIII (L1_CACHE_SIZE=32), this saves one cache line 
per rtable entry.

Thank you

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>



[-- Attachment #2: shrink_flow.patch --]
[-- Type: text/plain, Size: 4098 bytes --]

--- linux-2.6.19-rc2/include/net/flow.h	2006-10-18 06:03:08.000000000 +0200
+++ linux-2.6.19-rc2-ed/include/net/flow.h	2006-10-18 06:56:37.000000000 +0200
@@ -18,17 +18,21 @@
 		struct {
 			__be32			daddr;
 			__be32			saddr;
+#if defined(CONFIG_IP_ROUTE_FWMARK)
 			__u32			fwmark;
+#endif
 			__u8			tos;
 			__u8			scope;
 		} ip4_u;
 		
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 		struct {
 			struct in6_addr		daddr;
 			struct in6_addr		saddr;
 			__u32			fwmark;
 			__u32			flowlabel;
 		} ip6_u;
+#endif
 
 		struct {
 			__le16			daddr;
@@ -65,6 +69,7 @@
 			__u8	code;
 		} icmpt;
 
+#if defined(CONFIG_DECNET)
 		struct {
 			__le16	sport;
 			__le16	dport;
@@ -72,6 +77,7 @@
 			__u8	objnamel; /* Not 16 bits since max val is 16 */
 			__u8	objname[16]; /* Not zero terminated */
 		} dnports;
+#endif
 
 		__be32		spi;
 
--- linux-2.6.19-rc2/include/net/xfrm.h	2006-10-18 06:21:19.000000000 +0200
+++ linux-2.6.19-rc2-ed/include/net/xfrm.h	2006-10-18 06:53:41.000000000 +0200
@@ -517,6 +517,7 @@
 		(fl->oif == sel->ifindex || !sel->ifindex);
 }
 
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 static inline int
 __xfrm6_selector_match(struct xfrm_selector *sel, struct flowi *fl)
 {
@@ -527,6 +528,7 @@
 		(fl->proto == sel->proto || !sel->proto) &&
 		(fl->oif == sel->ifindex || !sel->ifindex);
 }
+#endif
 
 static inline int
 xfrm_selector_match(struct xfrm_selector *sel, struct flowi *fl,
@@ -535,8 +537,10 @@
 	switch (family) {
 	case AF_INET:
 		return __xfrm4_selector_match(sel, fl);
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6:
 		return __xfrm6_selector_match(sel, fl);
+#endif
 	}
 	return 0;
 }
@@ -577,7 +581,9 @@
 		struct xfrm_dst		*next;
 		struct dst_entry	dst;
 		struct rtable		rt;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 		struct rt6_info		rt6;
+#endif
 	} u;
 	struct dst_entry *route;
 	u32 genid;
@@ -650,12 +656,14 @@
 		 tmpl->saddr.a4 != x->props.saddr.a4);
 }
 
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 static inline int
 __xfrm6_state_addr_cmp(struct xfrm_tmpl *tmpl, struct xfrm_state *x)
 {
 	return	(!ipv6_addr_any((struct in6_addr*)&tmpl->saddr) &&
 		 ipv6_addr_cmp((struct in6_addr *)&tmpl->saddr, (struct in6_addr*)&x->props.saddr));
 }
+#endif
 
 static inline int
 xfrm_state_addr_cmp(struct xfrm_tmpl *tmpl, struct xfrm_state *x, unsigned short family)
@@ -663,8 +671,10 @@
 	switch (family) {
 	case AF_INET:
 		return __xfrm4_state_addr_cmp(tmpl, x);
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6:
 		return __xfrm6_state_addr_cmp(tmpl, x);
+#endif
 	}
 	return !0;
 }
@@ -762,8 +772,10 @@
 	switch (family){
 	case AF_INET:
 		return (xfrm_address_t *)&fl->fl4_dst;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6:
 		return (xfrm_address_t *)&fl->fl6_dst;
+#endif
 	}
 	return NULL;
 }
@@ -774,8 +786,10 @@
 	switch (family){
 	case AF_INET:
 		return (xfrm_address_t *)&fl->fl4_src;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6:
 		return (xfrm_address_t *)&fl->fl6_src;
+#endif
 	}
 	return NULL;
 }
@@ -790,6 +804,7 @@
 	return 0;
 }
 
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 static __inline__ int
 __xfrm6_state_addr_check(struct xfrm_state *x,
 			 xfrm_address_t *daddr, xfrm_address_t *saddr)
@@ -801,6 +816,7 @@
 		return 1;
 	return 0;
 }
+#endif
 
 static __inline__ int
 xfrm_state_addr_check(struct xfrm_state *x,
@@ -810,8 +826,10 @@
 	switch (family) {
 	case AF_INET:
 		return __xfrm4_state_addr_check(x, daddr, saddr);
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6:
 		return __xfrm6_state_addr_check(x, daddr, saddr);
+#endif
 	}
 	return 0;
 }
@@ -825,10 +843,12 @@
 		return __xfrm4_state_addr_check(x,
 						(xfrm_address_t *)&fl->fl4_dst,
 						(xfrm_address_t *)&fl->fl4_src);
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6:
 		return __xfrm6_state_addr_check(x,
 						(xfrm_address_t *)&fl->fl6_dst,
 						(xfrm_address_t *)&fl->fl6_src);
+#endif
 	}
 	return 0;
 }

Re: [PATCH] [NET] reduce sizeof(struct flow)

[YOSHIFUJI Hideaki / 吉藤英明]

In article <4535B6B7.4070107@cosmosbay.com> (at Wed, 18 Oct 2006 07:08:07 +0200), Eric Dumazet <dada1@cosmosbay.com> says:

>  		
> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>  		struct {
>  			struct in6_addr		daddr;
>  			struct in6_addr		saddr;

#ifdef CONFIG_IPV6_ROUTE_FWMARK

>  			__u32			fwmark;

#endif

>  			__u32			flowlabel;
>  		} ip6_u;
> +#endif
>  
>  		struct {
>  			__le16			daddr;

--yoshfuji

Re: [PATCH] [NET] reduce sizeof(struct flow)

[Eric Dumazet]

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

YOSHIFUJI Hideaki / ???? a écrit :
> In article <4535B6B7.4070107@cosmosbay.com> (at Wed, 18 Oct 2006 07:08:07 +0200), Eric Dumazet <dada1@cosmosbay.com> says:
> 
>>  		
>> +#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>>  		struct {
>>  			struct in6_addr		daddr;
>>  			struct in6_addr		saddr;
> 
> #ifdef CONFIG_IPV6_ROUTE_FWMARK
> 
>>  			__u32			fwmark;
> 
> #endif
> 
>>  			__u32			flowlabel;
>>  		} ip6_u;
>> +#endif
>>  
>>  		struct {
>>  			__le16			daddr;
> 
> --yoshfuji

Thank you Yoshfuji

New patch version follows :

Each route entry includes a 'struct flow'. This structure has a current size 
of 80 bytes. This patch makes a size reduction depending on 
CONFIG_IPV6/CONFIG_IPV6_MODULE/CONFIG_DECNET/CONFIG_IP_ROUTE_FWMARK/CONFIG_IPV6_ROUTE_FWMARK

For a platform doing IPV4 only, the new size is 36 bytes (instead of 80)
As many routers are base on PIII (L1_CACHE_SIZE=32), this saves one cache line 
per rtable entry.

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>


[-- Attachment #2: shrink_flow.patch --]
[-- Type: text/plain, Size: 4145 bytes --]

--- linux-2.6.19-rc2/include/net/flow.h	2006-10-18 06:03:08.000000000 +0200
+++ linux-2.6.19-rc2-ed/include/net/flow.h	2006-10-18 07:26:59.000000000 +0200
@@ -18,17 +18,23 @@
 		struct {
 			__be32			daddr;
 			__be32			saddr;
+#if defined(CONFIG_IP_ROUTE_FWMARK)
 			__u32			fwmark;
+#endif
 			__u8			tos;
 			__u8			scope;
 		} ip4_u;
 		
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 		struct {
 			struct in6_addr		daddr;
 			struct in6_addr		saddr;
+#if defined(CONFIG_IPV6_ROUTE_FWMARK)
 			__u32			fwmark;
+#endif
 			__u32			flowlabel;
 		} ip6_u;
+#endif
 
 		struct {
 			__le16			daddr;
@@ -65,6 +71,7 @@
 			__u8	code;
 		} icmpt;
 
+#if defined(CONFIG_DECNET)
 		struct {
 			__le16	sport;
 			__le16	dport;
@@ -72,6 +79,7 @@
 			__u8	objnamel; /* Not 16 bits since max val is 16 */
 			__u8	objname[16]; /* Not zero terminated */
 		} dnports;
+#endif
 
 		__be32		spi;
 
--- linux-2.6.19-rc2/include/net/xfrm.h	2006-10-18 06:21:19.000000000 +0200
+++ linux-2.6.19-rc2-ed/include/net/xfrm.h	2006-10-18 06:53:41.000000000 +0200
@@ -517,6 +517,7 @@
 		(fl->oif == sel->ifindex || !sel->ifindex);
 }
 
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 static inline int
 __xfrm6_selector_match(struct xfrm_selector *sel, struct flowi *fl)
 {
@@ -527,6 +528,7 @@
 		(fl->proto == sel->proto || !sel->proto) &&
 		(fl->oif == sel->ifindex || !sel->ifindex);
 }
+#endif
 
 static inline int
 xfrm_selector_match(struct xfrm_selector *sel, struct flowi *fl,
@@ -535,8 +537,10 @@
 	switch (family) {
 	case AF_INET:
 		return __xfrm4_selector_match(sel, fl);
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6:
 		return __xfrm6_selector_match(sel, fl);
+#endif
 	}
 	return 0;
 }
@@ -577,7 +581,9 @@
 		struct xfrm_dst		*next;
 		struct dst_entry	dst;
 		struct rtable		rt;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 		struct rt6_info		rt6;
+#endif
 	} u;
 	struct dst_entry *route;
 	u32 genid;
@@ -650,12 +656,14 @@
 		 tmpl->saddr.a4 != x->props.saddr.a4);
 }
 
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 static inline int
 __xfrm6_state_addr_cmp(struct xfrm_tmpl *tmpl, struct xfrm_state *x)
 {
 	return	(!ipv6_addr_any((struct in6_addr*)&tmpl->saddr) &&
 		 ipv6_addr_cmp((struct in6_addr *)&tmpl->saddr, (struct in6_addr*)&x->props.saddr));
 }
+#endif
 
 static inline int
 xfrm_state_addr_cmp(struct xfrm_tmpl *tmpl, struct xfrm_state *x, unsigned short family)
@@ -663,8 +671,10 @@
 	switch (family) {
 	case AF_INET:
 		return __xfrm4_state_addr_cmp(tmpl, x);
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6:
 		return __xfrm6_state_addr_cmp(tmpl, x);
+#endif
 	}
 	return !0;
 }
@@ -762,8 +772,10 @@
 	switch (family){
 	case AF_INET:
 		return (xfrm_address_t *)&fl->fl4_dst;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6:
 		return (xfrm_address_t *)&fl->fl6_dst;
+#endif
 	}
 	return NULL;
 }
@@ -774,8 +786,10 @@
 	switch (family){
 	case AF_INET:
 		return (xfrm_address_t *)&fl->fl4_src;
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6:
 		return (xfrm_address_t *)&fl->fl6_src;
+#endif
 	}
 	return NULL;
 }
@@ -790,6 +804,7 @@
 	return 0;
 }
 
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 static __inline__ int
 __xfrm6_state_addr_check(struct xfrm_state *x,
 			 xfrm_address_t *daddr, xfrm_address_t *saddr)
@@ -801,6 +816,7 @@
 		return 1;
 	return 0;
 }
+#endif
 
 static __inline__ int
 xfrm_state_addr_check(struct xfrm_state *x,
@@ -810,8 +826,10 @@
 	switch (family) {
 	case AF_INET:
 		return __xfrm4_state_addr_check(x, daddr, saddr);
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6:
 		return __xfrm6_state_addr_check(x, daddr, saddr);
+#endif
 	}
 	return 0;
 }
@@ -825,10 +843,12 @@
 		return __xfrm4_state_addr_check(x,
 						(xfrm_address_t *)&fl->fl4_dst,
 						(xfrm_address_t *)&fl->fl4_src);
+#if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
 	case AF_INET6:
 		return __xfrm6_state_addr_check(x,
 						(xfrm_address_t *)&fl->fl6_dst,
 						(xfrm_address_t *)&fl->fl6_src);
+#endif
 	}
 	return 0;
 }

Re: [PATCH] [NET] reduce sizeof(struct flow)

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Wed, 18 Oct 2006 07:08:07 +0200

> Each route entry includes a 'struct flow'. This structure has a
> current size of 80 bytes. This patch makes a size reduction
> depending on
> CONFIG_IPV6/CONFIG_IPV6_MODULE/CONFIG_DECNET/CONFIG_IP_ROUTE_FWMARK
> For a platform doing IPV4 only, the new size is 36 bytes (instead of
> 80) As many routers are base on PIII (L1_CACHE_SIZE=32), this saves
> one cache line per rtable entry.

I don't like these kinds of patches because %99 of people will never
ever realize the "savings" because distribution vendors will always,
unlaterally, enable everything.

For example, I'm still pending to cut the patch that kills the "struct
flow" from inet_sock's cork area since it really isn't needed.

I'd rather do things that get rid of the size in a way that benefits
everyone, regardless of kernel config.  I also would totally support
a patch that got rid of all the config ifdefs used in struct sk_buff.

Re: [PATCH] [NET] reduce sizeof(struct flow)

[Eric Dumazet]

David Miller a écrit :
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Wed, 18 Oct 2006 07:08:07 +0200
> 
>> Each route entry includes a 'struct flow'. This structure has a
>> current size of 80 bytes. This patch makes a size reduction
>> depending on
>> CONFIG_IPV6/CONFIG_IPV6_MODULE/CONFIG_DECNET/CONFIG_IP_ROUTE_FWMARK
>> For a platform doing IPV4 only, the new size is 36 bytes (instead of
>> 80) As many routers are base on PIII (L1_CACHE_SIZE=32), this saves
>> one cache line per rtable entry.
> 
> I don't like these kinds of patches because %99 of people will never
> ever realize the "savings" because distribution vendors will always,
> unlaterally, enable everything.
> 
> For example, I'm still pending to cut the patch that kills the "struct
> flow" from inet_sock's cork area since it really isn't needed.
> 
> I'd rather do things that get rid of the size in a way that benefits
> everyone, regardless of kernel config.  I also would totally support
> a patch that got rid of all the config ifdefs used in struct sk_buff.

I agree with you David (I dont like these patches as well), but I know a lot 
of sysadmins who recompile themselves their kernels because they have old 
machines and/or want optimized kernels, not 'big bloated kernels' from distro 
guys running the very latest intel/amd bomb.

As long the #ifdefs are in .h files, I do think the pain is small.

Fact is linux has many CONFIG_XXXX things, and as many possible binary 
versions for a single linux-2.6.XX version as atoms in the universe... A 
distro cannot provides all version and just provide the largest possible one, 
containing many obsolete/funny features.

How many people are using DECNET and want to pay the price of this 20 bytes 
dnports structure ?

Eric

Re: [PATCH] [NET] reduce sizeof(struct flow)

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Wed, 18 Oct 2006 07:42:17 +0200

> How many people are using DECNET and want to pay the price of this
> 20 bytes dnports structure ?

I bet you could make that cost get hidden by careful rearrangement
of the struct flow, or adjustment of the implementation.

BTW, I see assignments to these fields, and as a specific example
uli_u.dnports.objnamel but no use of it.  Perhaps much of dnports can
even be deleted outright. :-)

Re: [PATCH] [NET] reduce sizeof(struct flow)

[Steven Whitehouse]

Hi,

On Tue, Oct 17, 2006 at 11:53:36PM -0700, David Miller wrote:
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Wed, 18 Oct 2006 07:42:17 +0200
> 
> > How many people are using DECNET and want to pay the price of this
> > 20 bytes dnports structure ?
> 
Point taken :-) Eric, you also need to add a || defined(CONFIG_DECNET_MODULE)
I think in your patch, if you want to make this optional.

> I bet you could make that cost get hidden by careful rearrangement
> of the struct flow, or adjustment of the implementation.
> 
> BTW, I see assignments to these fields, and as a specific example
> uli_u.dnports.objnamel but no use of it.  Perhaps much of dnports can
> even be deleted outright. :-)
> -

Its not used at the moment[*], but would be required for any kind of flow
tracking. The objnum field, could be folded into the objname field I
guess on the basis that objnamel == 0 means objname[0] represents the objnum,
but that doesn't really buy much.

Looking at the rearrangement option, and the relative lengths of
ipv6 and DECnet node addresses, dn_u is a lot smaller than ip6_u and thus
the obj[num|name|namel] fields could be moved into that structure.
Even after doing this, dn_u would still be shorter than ip6_u, although
12 bytes longer than ip4_u (if my counting is correct). Is that an
acceptable solution?

[*] By which I mean, as you've already alluded to, that its set up correctly
but not currently read/tested by anything yet.

Steve.

Re: [PATCH] [NET] reduce sizeof(struct flow)

[Eric Dumazet]

On Wednesday 18 October 2006 10:20, Steven Whitehouse wrote:
> Hi,
>
> On Tue, Oct 17, 2006 at 11:53:36PM -0700, David Miller wrote:
> > From: Eric Dumazet <dada1@cosmosbay.com>
> > Date: Wed, 18 Oct 2006 07:42:17 +0200
> >
> > > How many people are using DECNET and want to pay the price of this
> > > 20 bytes dnports structure ?
>
> Point taken :-) Eric, you also need to add a ||
> defined(CONFIG_DECNET_MODULE) I think in your patch, if you want to make
> this optional.
>

Yes, thank you :)

> > I bet you could make that cost get hidden by careful rearrangement
> > of the struct flow, or adjustment of the implementation.
> >
> > BTW, I see assignments to these fields, and as a specific example
> > uli_u.dnports.objnamel but no use of it.  Perhaps much of dnports can
> > even be deleted outright. :-)
> > -
>
> Its not used at the moment[*], but would be required for any kind of flow
> tracking. The objnum field, could be folded into the objname field I
> guess on the basis that objnamel == 0 means objname[0] represents the
> objnum, but that doesn't really buy much.

Well, as I privately said to David, objnamel is used, at least on 2.6.18 tree
( net/decnet/dn_route.c function compare_keys() 
memcmp() will read this field and check before comparing objname[]

So it is set by dn_sk_ports_copy(), and used by compare_keys()

>
> Looking at the rearrangement option, and the relative lengths of
> ipv6 and DECnet node addresses, dn_u is a lot smaller than ip6_u and thus
> the obj[num|name|namel] fields could be moved into that structure.
> Even after doing this, dn_u would still be shorter than ip6_u, although
> 12 bytes longer than ip4_u (if my counting is correct). Is that an
> acceptable solution?

Well, most of my machines dont use IPV6 nor DECNET :)

>
> [*] By which I mean, as you've already alluded to, that its set up
> correctly but not currently read/tested by anything yet.
>

Re: [PATCH] [NET] reduce sizeof(struct flow)

[Steven Whitehouse]

Hi,

> >
> > Its not used at the moment[*], but would be required for any kind of flow
> > tracking. The objnum field, could be folded into the objname field I
> > guess on the basis that objnamel == 0 means objname[0] represents the
> > objnum, but that doesn't really buy much.
> 
> Well, as I privately said to David, objnamel is used, at least on 2.6.18 tree
> ( net/decnet/dn_route.c function compare_keys() 
> memcmp() will read this field and check before comparing objname[]
> 
> So it is set by dn_sk_ports_copy(), and used by compare_keys()
>
It is set by dn_sk_ports_copy() as you say, but the obj[num|name|namel]
fields are not used as a key in compare_keys() at the moment, although
all the other fields are used there.

Since the recent bug fix to this area of the code (memcmp was
comparing uninitialised padding) its easier to see what is
being compared.

In fact I suspect the reason that the obj fields were not put
in the dn_u where they'd take up a lot less room was
because the memcmp covered only dn_u and not the rest of the
structure. It was a while ago that I wrote this and my memory
is failing me as to the exact reason I did it like that.
 
> >
> > Looking at the rearrangement option, and the relative lengths of
> > ipv6 and DECnet node addresses, dn_u is a lot smaller than ip6_u and thus
> > the obj[num|name|namel] fields could be moved into that structure.
> > Even after doing this, dn_u would still be shorter than ip6_u, although
> > 12 bytes longer than ip4_u (if my counting is correct). Is that an
> > acceptable solution?
> 
> Well, most of my machines dont use IPV6 nor DECNET :)
> 
Its still an improvement over the current situation, even though
it might be (probably is) possible to improve it further,

Steve.

Re: [PATCH] [NET] reduce sizeof(struct flow)

[Eric Dumazet]

On Wednesday 18 October 2006 14:42, Steven Whitehouse wrote:
> Hi,
>
> > > Its not used at the moment[*], but would be required for any kind of
> > > flow tracking. The objnum field, could be folded into the objname field
> > > I guess on the basis that objnamel == 0 means objname[0] represents the
> > > objnum, but that doesn't really buy much.
> >
> > Well, as I privately said to David, objnamel is used, at least on 2.6.18
> > tree ( net/decnet/dn_route.c function compare_keys()
> > memcmp() will read this field and check before comparing objname[]
> >
> > So it is set by dn_sk_ports_copy(), and used by compare_keys()
>
> It is set by dn_sk_ports_copy() as you say, but the obj[num|name|namel]
> fields are not used as a key in compare_keys() at the moment, although
> all the other fields are used there.
>
> Since the recent bug fix to this area of the code (memcmp was
> comparing uninitialised padding) its easier to see what is
> being compared.
>

Oh, I see, you are referring to 2.6.19-rc2, I was referring to 2.6.18 (what I 
call current linux version)

> In fact I suspect the reason that the obj fields were not put
> in the dn_u where they'd take up a lot less room was
> because the memcmp covered only dn_u and not the rest of the
> structure. It was a while ago that I wrote this and my memory
> is failing me as to the exact reason I did it like that.
>
> > > Looking at the rearrangement option, and the relative lengths of
> > > ipv6 and DECnet node addresses, dn_u is a lot smaller than ip6_u and
> > > thus the obj[num|name|namel] fields could be moved into that structure.
> > > Even after doing this, dn_u would still be shorter than ip6_u, although
> > > 12 bytes longer than ip4_u (if my counting is correct). Is that an
> > > acceptable solution?
> >
> > Well, most of my machines dont use IPV6 nor DECNET :)
>
> Its still an improvement over the current situation, even though
> it might be (probably is) possible to improve it further,

OK then. Even on a distro kernel (allyesconfig), size(flow) would shrink by 20 
bytes.

Thank you.

Re: [PATCH] [NET] reduce sizeof(struct flow)

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Wed, 18 Oct 2006 15:32:22 +0200

> OK then. Even on a distro kernel (allyesconfig), size(flow) would
> shrink by 20 bytes.

For the time being, why don't we just kill off the unused dnports
bits entirely.

Once the support code to use those bits is written, we can
investigate intelligent ways to pack them back into struct
flow, ok?

Re: [PATCH] [NET] reduce sizeof(struct flow)

[Eric Dumazet]

David Miller a écrit :
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Wed, 18 Oct 2006 15:32:22 +0200
> 
>> OK then. Even on a distro kernel (allyesconfig), size(flow) would
>> shrink by 20 bytes.
> 
> For the time being, why don't we just kill off the unused dnports
> bits entirely.
> 
> Once the support code to use those bits is written, we can
> investigate intelligent ways to pack them back into struct
> flow, ok?
> 
> 

OK I will prepare a patch if Steve doesnt beat me :)

Eric

[PATCH] [NET] reduce sizeof(struct flowi) by 20 bytes

[Eric Dumazet]

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

Hi all

As suggested by David, just kill off some unused fields in dnports to reduce 
sizef(struct flowi). If they come back, they should be moved to nl_u.dn_u in 
order not to enlarge again struct flowi

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

[-- Attachment #2: shrink_flowi.patch --]
[-- Type: text/plain, Size: 1068 bytes --]

diff --git a/include/net/dn.h b/include/net/dn.h
index 465b783..c39385b 100644
--- a/include/net/dn.h
+++ b/include/net/dn.h
@@ -199,11 +199,13 @@ static inline void dn_sk_ports_copy(stru
 {
 	fl->uli_u.dnports.sport = scp->addrloc;
 	fl->uli_u.dnports.dport = scp->addrrem;
+#if 0 /* not yet used */
 	fl->uli_u.dnports.objnum = scp->addr.sdn_objnum;
 	if (fl->uli_u.dnports.objnum == 0) {
 		fl->uli_u.dnports.objnamel = (__u8)dn_ntohs(scp->addr.sdn_objnamel);
 		memcpy(fl->uli_u.dnports.objname, scp->addr.sdn_objname, 16);
 	}
+#endif
 }
 
 extern unsigned dn_mss_from_pmtu(struct net_device *dev, int mtu);
diff --git a/include/net/flow.h b/include/net/flow.h
index 3b44d72..bdc0e49 100644
--- a/include/net/flow.h
+++ b/include/net/flow.h
@@ -68,9 +68,11 @@ #define FLOWI_FLAG_MULTIPATHOLDROUTE 0x0
 		struct {
 			__le16	sport;
 			__le16	dport;
+#if 0 /* not yet used, should move to nl_u.dn_u */
 			__u8	objnum;
 			__u8	objnamel; /* Not 16 bits since max val is 16 */
 			__u8	objname[16]; /* Not zero terminated */
+#endif
 		} dnports;
 
 		__be32		spi;

Re: [PATCH] [NET] reduce sizeof(struct flowi) by 20 bytes

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Fri, 20 Oct 2006 15:55:33 +0200

> As suggested by David, just kill off some unused fields in dnports to reduce 
> sizef(struct flowi). If they come back, they should be moved to nl_u.dn_u in 
> order not to enlarge again struct flowi
> 
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

Applied, except I changed the patch to really delete this
stuff instead of using "#if 0".  Anyone can look into old
trees of the code history to see the old stuff. :-)

Thanks Eric.

Re: [PATCH] [NET] reduce sizeof(struct flow)

[Ingo Oeser]

Hi David,

David Miller schrieb:
> I don't like these kinds of patches because %99 of people will never
> ever realize the "savings" because distribution vendors will always,
> unlaterally, enable everything.

People producing Linux Appliances DO compile their own kernels.

And some distribution vendors are still at 2.6.8-$WHACKY_PATCHES
or similiar, which is not usable for many things 
(e.g. IPsec, SIP behind NAT etc.).

But I agree that workings towards smaller size in "make allyesconfig"
is much better :-)


Regards

Ingo Oeser

[PATCH] [NET] can use __get_cpu_var() instead of per_cpu() in loopback driver

[Eric Dumazet]

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

As BHs are off in loopback_xmit(), preemption cannot occurs, so we can use 
__get_cpu_var() instead of per_cpu() (and avoid a 
preempt_enable()/preempt_disable() pair)

Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>


[-- Attachment #2: loopback.patch --]
[-- Type: text/plain, Size: 522 bytes --]

--- net-2.6/drivers/net/loopback.c	2006-10-19 16:46:27.000000000 +0200
+++ net-2.6-ed/drivers/net/loopback.c      2006-10-20 09:11:04.000000000 +0200
@@ -153,14 +153,14 @@
 #endif
 	dev->last_rx = jiffies;
 
-	lb_stats = &per_cpu(pcpu_lstats, get_cpu());
+	/* it's OK to use __get_cpu_var() because BHs are off */
+	lb_stats = &__get_cpu_var(pcpu_lstats);
 	lb_stats->bytes += skb->len;
 	lb_stats->packets++;
-	put_cpu();
 
 	netif_rx(skb);
 
-	return(0);
+	return 0;
 }
 
 static struct net_device_stats loopback_stats;

Re: [PATCH] [NET] can use __get_cpu_var() instead of per_cpu() in loopback driver

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Fri, 20 Oct 2006 09:18:35 +0200

> As BHs are off in loopback_xmit(), preemption cannot occurs, so we can use 
> __get_cpu_var() instead of per_cpu() (and avoid a 
> preempt_enable()/preempt_disable() pair)
> 
> Signed-off-by: Eric Dumazet <dada1@cosmosbay.com>

Applied, thanks Eric.