[PATCH] reorder struct pipe_buf_operations

[PATCH] reorder struct pipe_buf_operations

[Eric Dumazet]

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

Fields of struct pipe_buf_operations have not a precise layout (ie not 
optimized to fit cache lines nor reduce cache line ping pongs)

The bufs[] array is *large* and is placed near the beginning of the structure, 
so all following fields have a large offset. This is unfortunate because many 
archs have smaller instructions when using small offsets relative to a base 
register. On x86 for example, 7 bits offsets have smaller instruction lengths.

Moving bufs[] at the end of pipe_buf_operations permits all fields to have 
small offsets, and reduce text size, and icache pressure.

# size vmlinux.pre vmlinux
    text    data     bss     dec     hex filename
3268989  664356  492196 4425541  438745 vmlinux.pre
3268765  664356  492196 4425317  438665 vmlinux

So this patch reduces text size by 224 bytes on my x86_64 machine. Similar 
results on ia32.


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

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

--- linux-2.6.19/include/linux/pipe_fs_i.h	2006-12-11 23:06:57.000000000 +0100
+++ linux-2.6.19-ed/include/linux/pipe_fs_i.h	2006-12-11 22:58:42.000000000 +0100
@@ -41,7 +41,6 @@ struct pipe_buf_operations {
 struct pipe_inode_info {
 	wait_queue_head_t wait;
 	unsigned int nrbufs, curbuf;
-	struct pipe_buffer bufs[PIPE_BUFFERS];
 	struct page *tmp_page;
 	unsigned int readers;
 	unsigned int writers;
@@ -51,6 +50,7 @@ struct pipe_inode_info {
 	struct fasync_struct *fasync_readers;
 	struct fasync_struct *fasync_writers;
 	struct inode *inode;
+	struct pipe_buffer bufs[PIPE_BUFFERS];
 };
 
 /* Differs from PIPE_BUF in that PIPE_SIZE is the length of the actual

[PATCH] Introduce jiffies_32 and related compare functions

[Eric Dumazet]

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

Some subsystems dont need more than 32bits timestamps.

See for example net/ipv4/inetpeer.c and include/net/tcp.h :
#define tcp_time_stamp            ((__u32)(jiffies))


Because most timeouts should work with 'normal jiffies' that are 32bits on 
32bits platforms, it makes sense to be able to use only 32bits to store them 
and not 64 bits, to save ram.

This patch introduces jiffies_32, and related comparison functions 
time_after32(), time_before32(), time_after_eq32() and time_before_eq32().

I plan to use this infrastructure in network code for example (struct 
dst_entry comes to mind).

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

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

--- linux-2.6.19/include/linux/jiffies.h	2006-12-12 00:32:00.000000000 +0100
+++ linux-2.6.19-ed/include/linux/jiffies.h	2006-12-12 00:41:40.000000000 +0100
@@ -80,6 +80,11 @@
  */
 extern u64 __jiffy_data jiffies_64;
 extern unsigned long volatile __jiffy_data jiffies;
+/*
+ * Some subsystems need small deltas and can store 32 bits timestamps
+ * instead of 'long', to save space on 64bits platforms.
+ */
+#define jiffies_32 ((u32)jiffies)
 
 #if (BITS_PER_LONG < 64)
 u64 get_jiffies_64(void);
@@ -131,6 +136,22 @@ static inline u64 get_jiffies_64(void)
 #define time_before_eq64(a,b)	time_after_eq64(b,a)
 
 /*
+ * Same as above, but does so with 32bits types.
+ * These must be used when using jiffies_32
+ */
+#define time_after32(a,b)	\
+	(typecheck(__u32, a) &&	\
+	 typecheck(__u32, b) && \
+	 ((__s32)(b) - (__s32)(a) < 0))
+#define time_before32(a,b)	time_after32(b,a)
+
+#define time_after_eq32(a,b)	\
+	(typecheck(__u32, a) && \
+	 typecheck(__u32, b) && \
+	 ((__s32)(a) - (__s32)(b) >= 0))
+#define time_before_eq32(a,b)	time_after_eq32(b,a)
+
+/*
  * Have the 32 bit jiffies value wrap 5 minutes after boot
  * so jiffies wrap bugs show up earlier.
  */

Re: [PATCH] Introduce jiffies_32 and related compare functions

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Mon, 11 Dec 2006 23:58:06 +0100

> Some subsystems dont need more than 32bits timestamps.
> 
> See for example net/ipv4/inetpeer.c and include/net/tcp.h :
> #define tcp_time_stamp            ((__u32)(jiffies))
> 
> 
> Because most timeouts should work with 'normal jiffies' that are 32bits on 
> 32bits platforms, it makes sense to be able to use only 32bits to store them 
> and not 64 bits, to save ram.
> 
> This patch introduces jiffies_32, and related comparison functions 
> time_after32(), time_before32(), time_after_eq32() and time_before_eq32().
> 
> I plan to use this infrastructure in network code for example (struct 
> dst_entry comes to mind).

The TCP case is because the protocol limits the size of
the timestamp we can store in the TCP Timestamp option.

Otherwise we would use the full 64-bit jiffies timestamp,
in order to have a larger window of values which would not
overflow.

Since there is no protocol limitation involved in cases
such as dst_entry, I think we should keep it at 64-bits
on 64-bit platforms to make the wrap-around window as
large as possible.

I really don't see any reason to make these changes.  Yes,
you'd save some space, but one of the chief advantages of
64-bit is that we get larger jiffies value windows.  If
that has zero value, as your intended changes imply, then
we shouldn't need the default 64-bit jiffies either, by
implication.

Re: [PATCH] Introduce jiffies_32 and related compare functions

[Eric Dumazet]

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

David Miller a écrit :
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Mon, 11 Dec 2006 23:58:06 +0100
> 
>> Some subsystems dont need more than 32bits timestamps.
>>
>> See for example net/ipv4/inetpeer.c and include/net/tcp.h :
>> #define tcp_time_stamp            ((__u32)(jiffies))
>>
>>
>> Because most timeouts should work with 'normal jiffies' that are 32bits on 
>> 32bits platforms, it makes sense to be able to use only 32bits to store them 
>> and not 64 bits, to save ram.
>>
>> This patch introduces jiffies_32, and related comparison functions 
>> time_after32(), time_before32(), time_after_eq32() and time_before_eq32().
>>
>> I plan to use this infrastructure in network code for example (struct 
>> dst_entry comes to mind).
> 
> The TCP case is because the protocol limits the size of
> the timestamp we can store in the TCP Timestamp option.
> 
> Otherwise we would use the full 64-bit jiffies timestamp,
> in order to have a larger window of values which would not
> overflow.
> 
> Since there is no protocol limitation involved in cases
> such as dst_entry, I think we should keep it at 64-bits
> on 64-bit platforms to make the wrap-around window as
> large as possible.
> 
> I really don't see any reason to make these changes.  Yes,
> you'd save some space, but one of the chief advantages of
> 64-bit is that we get larger jiffies value windows.  If
> that has zero value, as your intended changes imply, then
> we shouldn't need the default 64-bit jiffies either, by
> implication.

Well, just to have similar functions to manipulate jiffies.

We already know that using a 32bits dtime in struct inet_peer permited an 
object size of 64 bytes instead of 128 bytes (on 64bits platforms)

On one machine (running linux-2.6.16) I have :

# grep peer /proc/slabinfo
inet_peer_cache    65972  86220    128   30    1 : tunables  120   60    8 : 
slabdata   2874   2874    262

(So the 128 bytes -> 64 bytes is going to save 1437 pages of memory)

# grep dst /proc/slabinfo
ip_dst_cache      1765818 2077380    320   12    1 : tunables   54   27    8 : 
slabdata 173115 173115     39

(So saving 4*4 bytes per dst might save 32 MB of ram).
I doubt being able to extend the expiration of a dst above 2^32 ticks (49 days 
if HZ=1000, 198 days if HZ=250) is worth the ram wastage.

Dont you prefer to be able to apply this patch for example ?

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

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

--- linux/net/ipv4/inetpeer.c.orig	2006-12-12 05:25:42.000000000 +0100
+++ linux-ed/net/ipv4/inetpeer.c	2006-12-12 05:29:22.000000000 +0100
@@ -338,8 +338,7 @@ static int cleanup_once(unsigned long tt
 	spin_lock_bh(&inet_peer_unused_lock);
 	p = inet_peer_unused_head;
 	if (p != NULL) {
-		__u32 delta = (__u32)jiffies - p->dtime;
-		if (delta < ttl) {
+		if (time_after32(p->dtime + ttl, jiffies_32)) {
 			/* Do not prune fresh entries. */
 			spin_unlock_bh(&inet_peer_unused_lock);
 			return -1;
@@ -466,7 +465,7 @@ void inet_putpeer(struct inet_peer *p)
 		p->unused_next = NULL;
 		*inet_peer_unused_tailp = p;
 		inet_peer_unused_tailp = &p->unused_next;
-		p->dtime = (__u32)jiffies;
+		p->dtime = jiffies_32;
 	}
 	spin_unlock_bh(&inet_peer_unused_lock);
 }

Re: [PATCH] Introduce jiffies_32 and related compare functions

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Tue, 12 Dec 2006 04:47:14 +0100

> I doubt being able to extend the expiration of a dst above 2^32
> ticks (49 days if HZ=1000, 198 days if HZ=250) is worth the ram
> wastage.

And this doesn't apply for all jiffies uses because? :-)

That's the point I'm trying to make and get a discussion on.

Re: [PATCH] Introduce jiffies_32 and related compare functions

[Eric Dumazet]

David Miller a écrit :
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Tue, 12 Dec 2006 04:47:14 +0100
> 
>> I doubt being able to extend the expiration of a dst above 2^32
>> ticks (49 days if HZ=1000, 198 days if HZ=250) is worth the ram
>> wastage.
> 
> And this doesn't apply for all jiffies uses because? :-)
> 
> That's the point I'm trying to make and get a discussion on.
> 
> 

Ah ok :)

Maybe my intentions were not clear :

I am not suggesting replacing all jiffies to jiffies_32. Just *selected* ones :)

BTW, the real limit is 2^31 ticks, so its 24 days.

We definitly *like* being able to use bigger timeouts on 64bits platforms.

Not that they are mandatory since the same application should run fine on 
32bits kernel. But as the standard type for 'tick timestamps' is 'unsigned 
long', a change would be invasive.

Maybe some applications are now relying on being able to 
sleep()/select()/poll() for periods > 30 days and only run on 64 bits kernels.

Eric

Re: [PATCH] Introduce jiffies_32 and related compare functions

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Tue, 12 Dec 2006 05:09:23 +0100

> We definitly *like* being able to use bigger timeouts on 64bits platforms.
> 
> Not that they are mandatory since the same application should run fine on 
> 32bits kernel. But as the standard type for 'tick timestamps' is 'unsigned 
> long', a change would be invasive.
>
> Maybe some applications are now relying on being able to
> sleep()/select()/poll() for periods > 30 days and only run on 64
> bits kernels.

I think one possible target would be struct timer, at least
in theory.

There is also a line of reasoning that says that on 64-bit
platforms we have some flexibility to set HZ very large, if
we wanted to at some point, and going to 32-bit jiffies
storage for some things may eliminate that kind of flexibility.

Just some food for thought...

Re: [PATCH] Introduce jiffies_32 and related compare functions

[Eric Dumazet]

David Miller a écrit :
> From: Eric Dumazet <dada1@cosmosbay.com>
> Date: Tue, 12 Dec 2006 05:09:23 +0100
> 
>> We definitly *like* being able to use bigger timeouts on 64bits platforms.
>>
>> Not that they are mandatory since the same application should run fine on 
>> 32bits kernel. But as the standard type for 'tick timestamps' is 'unsigned 
>> long', a change would be invasive.
>>
>> Maybe some applications are now relying on being able to
>> sleep()/select()/poll() for periods > 30 days and only run on 64
>> bits kernels.
> 
> I think one possible target would be struct timer, at least
> in theory.
> 
> There is also a line of reasoning that says that on 64-bit
> platforms we have some flexibility to set HZ very large, if
> we wanted to at some point, and going to 32-bit jiffies
> storage for some things may eliminate that kind of flexibility.
>

Yes good point, and my understanding is that we go for a tickless kernel in 
2.6.21, or so.
I wonder if virtual HZ wont be sticked to a low value.

I suspect in the case HZ raises, we switch some/most uses of jiffies_32 to 
another  variable (xtime_32 or whatever), but keep the storage on 32bits...

But keeping 64bits values 'just because hardware allows us this kind of 
expenditure' seems not reasonable to me, but lazy...

Eric

Re: [PATCH] Introduce jiffies_32 and related compare functions

[David Miller]

From: Eric Dumazet <dada1@cosmosbay.com>
Date: Tue, 12 Dec 2006 07:56:35 +0100

> But keeping 64bits values 'just because hardware allows us this kind of 
> expenditure' seems not reasonable to me, but lazy...

I agree with you for the most part.

Flexibility is pretty easy to maintain, and you seem to
alude to this, by allowing the jiffies_32 thing to have
a configurable type.

With this in mind maybe it's better to give the type some other more
descriptive name, something that states what the type represents,
rather than it's likely implementation :-)