Skip to contentSkip to conversation

Reduce struct page by 8 bytes on 64bit

PATCH Andi Kleen 5 participants 18 messages Apr 15, 2003 – Apr 17, 2003

Subsystems (5)
Memory management MM core Generic include/asm header files Page allocator +1 more subsystems
lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

(all 64bit maintainers in cc)

struct page currently has this:

struct page {
unsigned long flags; /* atomic flags, some possibly
updated asynchronously */
atomic_t count; /* Usage count, see below. */
struct list_head list; /* ->mapping has some page lists. */

On a 64bit host only 32bit of flags are used, the second 32bit are wated. atomic_t is also normally 32bit, which requires 4 bytes of padding for natural alignment. Together this gives 8 byte wasted.

8 byte overhead in struct page is 2MB wasted on a 1GB 64bit system with 4K pages.

This could be avoided if flags was declared to a 32bit type. The problem is that some architectures need the unsigned long for atomic set_bit/clear_bit. Others like x86-64 do not - they can toggle bits atomically in any data type.

I worked around this by declaring a new data type atomic_bitmask32 with matching set_bit32/clear_bit32 etc. interfaces. Currently only on x86-64 aomitc_bitmask32 is defined to unsigned, everybody else still uses unsigned long. The other 64bit architectures can define it to unsigned too if they can confirm that it's ok to do.

Patch for 2.5.67.

-Andi

include/asm-alpha/bitops.h renamed from include/asm-alpha/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-alpha/bitops.h-BM32 linux-2.5.67-work/include/asm-alpha/bitops.h
@@ -499,6 +499,8 @@
 #define minix_test_bit(nr,addr) test_bit(nr,addr)
 #define minix_find_first_zero_bit(addr,size) find_first_zero_bit(addr,size)
 
+#include <asm-generic/bitops32.h>
+
 #endif /* __KERNEL__ */
 
 #endif /* _ALPHA_BITOPS_H */
include/asm-arm/bitops.h renamed from include/asm-arm/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-arm/bitops.h-BM32 linux-2.5.67-work/include/asm-arm/bitops.h
@@ -381,6 +381,8 @@
 #define minix_find_first_zero_bit(p,sz)		\
 		_find_first_zero_bit_le(p,sz)
 
+#include <asm-generic/bitops32.h>
+
 #endif /* __KERNEL__ */
 
 #endif /* _ARM_BITOPS_H */
include/asm-cris/bitops.h renamed from include/asm-cris/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-cris/bitops.h-BM32 linux-2.5.67-work/include/asm-cris/bitops.h
@@ -410,6 +410,8 @@
 #undef MAX_RT_PRIO
 #endif
 
+#include <asm-generic/bitops32.h>
+
 #endif /* __KERNEL__ */
 
 #endif /* _CRIS_BITOPS_H */
include/asm-generic/bitops32.h new file +17 -0
diff -u /dev/null linux-2.5.67-work/include/asm-generic/bitops32.h
@@ -0,0 +1,17 @@
+#ifndef _BITFLAGS32_H
+#define _BITFLAGS32_H 1
+
+/* Fallback. Use unsigned long for 32bit bitflags operations.
+   If your architecture supports this on unsigned change it in bitops.h
+   (makes only sense on 64bit architectures) */
+
+typedef unsigned long atomic_bitmask32; 
+#define set_bit32 set_bit
+#define __set_bit32 __set_bit
+#define clear_bit32 clear_bit
+#define __clear_bit32 __clear_bit
+#define test_bit32 test_bit
+#define test_and_set_bit32 test_and_set_bit
+#define test_and_clear_bit32 test_and_clear_bit
+
+#endif
include/asm-i386/bitops.h renamed from include/asm-i386/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-i386/bitops.h-BM32 linux-2.5.67-work/include/asm-i386/bitops.h
@@ -495,6 +495,8 @@
 #define minix_find_first_zero_bit(addr,size) \
 	find_first_zero_bit((void*)addr,size)
 
+#include <asm-generic/bitops32.h>
+
 #endif /* __KERNEL__ */
 
 #endif /* _I386_BITOPS_H */
include/asm-ia64/bitops.h renamed from include/asm-ia64/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-ia64/bitops.h-BM32 linux-2.5.67-work/include/asm-ia64/bitops.h
@@ -475,6 +475,8 @@
 	return __ffs(b[2]) + 128;
 }
 
+#include <asm-generic/bitops32.h>
+
 #endif /* __KERNEL__ */
 
 #endif /* _ASM_IA64_BITOPS_H */
include/asm-m68k/bitops.h renamed from include/asm-m68k/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-m68k/bitops.h-BM32 linux-2.5.67-work/include/asm-m68k/bitops.h
@@ -416,6 +416,8 @@
 	return (p - addr) * 32 + res;
 }
 
+#include <asm-generic/bitops32.h>
+
 #endif /* __KERNEL__ */
 
 #endif /* _M68K_BITOPS_H */
include/asm-m68knommu/bitops.h renamed from include/asm-m68knommu/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-m68knommu/bitops.h-BM32 linux-2.5.67-work/include/asm-m68knommu/bitops.h
@@ -483,6 +483,8 @@
 #define hweight16(x) generic_hweight16(x)
 #define hweight8(x) generic_hweight8(x)
 
+#include <asm-generic/bitops32.h>
+
 #endif /* __KERNEL__ */
 
 #endif /* _M68KNOMMU_BITOPS_H */
include/asm-mips/bitops.h renamed from include/asm-mips/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-mips/bitops.h-BM32 linux-2.5.67-work/include/asm-mips/bitops.h
@@ -909,4 +909,6 @@
 #define minix_test_bit(nr,addr) test_bit(nr,addr)
 #define minix_find_first_zero_bit(addr,size) find_first_zero_bit(addr,size)
 
+#include <asm-generic/bitops32.h>
+
 #endif /* _ASM_BITOPS_H */
include/asm-mips64/bitops.h renamed from include/asm-mips64/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-mips64/bitops.h-BM32 linux-2.5.67-work/include/asm-mips64/bitops.h
@@ -618,6 +618,8 @@
 #define minix_test_bit(nr,addr) test_bit(nr,addr)
 #define minix_find_first_zero_bit(addr,size) find_first_zero_bit(addr,size)
 
+#include <asm-generic/bitops32.h>
+
 #endif /* __KERNEL__ */
 
 #endif /* _ASM_BITOPS_H */
include/asm-parisc/bitops.h renamed from include/asm-parisc/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-parisc/bitops.h-BM32 linux-2.5.67-work/include/asm-parisc/bitops.h
@@ -455,4 +455,6 @@
 #define minix_test_bit(nr,addr) ext2_test_bit(nr,addr)
 #define minix_find_first_zero_bit(addr,size) ext2_find_first_zero_bit(addr,size)
 
+#include <asm-generic/bitops32.h>
+
 #endif /* _PARISC_BITOPS_H */
include/asm-ppc/bitops.h renamed from include/asm-ppc/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-ppc/bitops.h-BM32 linux-2.5.67-work/include/asm-ppc/bitops.h
@@ -454,5 +454,7 @@
 #define minix_test_bit(nr,addr) ext2_test_bit(nr,addr)
 #define minix_find_first_zero_bit(addr,size) ext2_find_first_zero_bit(addr,size)
 
+#include <asm-generic/bitops32.h>
+
 #endif /* _PPC_BITOPS_H */
 #endif /* __KERNEL__ */
include/asm-ppc64/bitops.h renamed from include/asm-ppc64/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-ppc64/bitops.h-BM32 linux-2.5.67-work/include/asm-ppc64/bitops.h
@@ -350,5 +350,7 @@
 #define minix_test_bit(nr,addr)			test_bit(nr,addr)
 #define minix_find_first_zero_bit(addr,size)	find_first_zero_bit(addr,size)
 
+#include <asm-generic/bitops32.h>
+
 #endif /* __KERNEL__ */
 #endif /* _PPC64_BITOPS_H */
include/asm-s390/bitops.h renamed from include/asm-s390/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-s390/bitops.h-BM32 linux-2.5.67-work/include/asm-s390/bitops.h
@@ -901,6 +901,8 @@
 #define minix_test_bit(nr,addr) test_bit(nr,addr)
 #define minix_find_first_zero_bit(addr,size) find_first_zero_bit(addr,size)
 
+#include <asm-generic/bitops32.h>
+
 #endif /* __KERNEL__ */
 
 #endif /* _S390_BITOPS_H */
include/asm-s390x/bitops.h renamed from include/asm-s390x/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-s390x/bitops.h-BM32 linux-2.5.67-work/include/asm-s390x/bitops.h
@@ -941,6 +941,8 @@
 #define minix_test_bit(nr,addr) test_bit(nr,addr)
 #define minix_find_first_zero_bit(addr,size) find_first_zero_bit(addr,size)
 
+#include <asm-generic/bitops32.h>
+
 #endif /* __KERNEL__ */
 
 #endif /* _S390_BITOPS_H */
include/asm-sh/bitops.h renamed from include/asm-sh/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-sh/bitops.h-BM32 linux-2.5.67-work/include/asm-sh/bitops.h
@@ -351,6 +351,8 @@
 #define minix_test_bit(nr,addr) test_bit(nr,addr)
 #define minix_find_first_zero_bit(addr,size) find_first_zero_bit(addr,size)
 
+#include <asm-generic/bitops32.h>
+
 #endif /* __KERNEL__ */
 
 #endif /* __ASM_SH_BITOPS_H */
include/asm-sparc/bitops.h renamed from include/asm-sparc/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-sparc/bitops.h-BM32 linux-2.5.67-work/include/asm-sparc/bitops.h
@@ -466,6 +466,8 @@
 #define minix_test_bit(nr,addr)			test_bit(nr,addr)
 #define minix_find_first_zero_bit(addr,size)	find_first_zero_bit(addr,size)
 
+#include <asm-generic/bitops32.h>
+
 #endif /* __KERNEL__ */
 
 #endif /* defined(_SPARC_BITOPS_H) */
include/asm-sparc64/bitops.h renamed from include/asm-sparc64/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-sparc64/bitops.h-BM32 linux-2.5.67-work/include/asm-sparc64/bitops.h
@@ -367,6 +367,8 @@
 #define minix_find_first_zero_bit(addr,size) \
 	find_first_zero_bit((unsigned long *)(addr),(size))
 
+#include <asm-generic/bitops32.h>
+
 #endif /* __KERNEL__ */
 
 #endif /* defined(_SPARC64_BITOPS_H) */
include/asm-v850/bitops.h renamed from include/asm-v850/bitops.h-BM32 +2 -0
diff -u linux-2.5.67-work/include/asm-v850/bitops.h-BM32 linux-2.5.67-work/include/asm-v850/bitops.h
@@ -264,6 +264,8 @@
 #define minix_test_bit 			test_bit
 #define minix_find_first_zero_bit 	find_first_zero_bit
 
+#include <asm-generic/bitops32.h>
+
 #endif /* __KERNEL__ */
 
 #endif /* __V850_BITOPS_H__ */
include/asm-x86_64/bitops.h renamed from include/asm-x86_64/bitops.h-BM32 +10 -0
diff -u linux-2.5.67-work/include/asm-x86_64/bitops.h-BM32 linux-2.5.67-work/include/asm-x86_64/bitops.h
@@ -506,6 +506,16 @@
 /* find last set bit */
 #define fls(x) generic_fls(x)
 
+/* bitflag operations on 32bit */
+typedef unsigned int atomic_bitmask32; 
+#define set_bit32 set_bit
+#define __set_bit32 __set_bit
+#define clear_bit32 clear_bit
+#define __clear_bit32 __clear_bit
+#define test_bit32 test_bit
+#define test_and_set_bit32 test_and_set_bit
+#define test_and_clear_bit32 test_and_clear_bit
+
 #endif /* __KERNEL__ */
 
 #endif /* _X86_64_BITOPS_H */
include/linux/mm.h renamed from include/linux/mm.h-BM32 +4 -3
diff -u linux-2.5.67-work/include/linux/mm.h-BM32 linux-2.5.67-work/include/linux/mm.h
@@ -24,6 +24,7 @@
 #include <asm/page.h>
 #include <asm/pgtable.h>
 #include <asm/atomic.h>
+#include <asm/bitops.h>
 
 /*
  * Linux kernel virtual memory manager primitives.

@@ -158,7 +159,7 @@
  * TODO: make this structure smaller, it could be as small as 32 bytes.
  */
 struct page {
-	unsigned long flags;		/* atomic flags, some possibly
+	atomic_bitmask32 flags;	/* atomic flags, some possibly
 					   updated asynchronously */
 	atomic_t count;			/* Usage count, see below. */
 	struct list_head list;		/* ->mapping has some page lists. */

@@ -314,7 +315,7 @@
  * sets it, so none of the operations on it need to be atomic.
  */
 #define NODE_SHIFT 4
-#define ZONE_SHIFT (BITS_PER_LONG - 8)
+#define ZONE_SHIFT (sizeof(atomic_bitmask32)*8 - 8)
 
 struct zone;
 extern struct zone *zone_table[];

@@ -326,7 +327,7 @@
 
 static inline void set_page_zone(struct page *page, unsigned long zone_num)
 {
-	page->flags &= ~(~0UL << ZONE_SHIFT);
+	page->flags &= ~(~(atomic_bitmask32)0U << ZONE_SHIFT);
 	page->flags |= zone_num << ZONE_SHIFT;
 }
include/linux/page-flags.h renamed from include/linux/page-flags.h-BM32 +76 -76

198 lines not on this page · raw message

mm/page_alloc.c renamed from mm/page_alloc.c-BM32 +1 -1
diff -u linux-2.5.67-work/mm/page_alloc.c-BM32 linux-2.5.67-work/mm/page_alloc.c
@@ -70,7 +70,7 @@
 {
 	printk("Bad page state at %s\n", function);
 	printk("flags:0x%08lx mapping:%p mapped:%d count:%d\n",
-		page->flags, page->mapping,
+		(unsigned long)page->flags, page->mapping,
 		page_mapped(page), page_count(page));
 	printk("Backtrace:\n");
 	dump_stack();
#1 lore raw

From: Andi Kleen <[email protected]>
Date: Tue, 15 Apr 2003 13:24:30 +0200

I worked around this by declaring a new data type atomic_bitmask32 with matching set_bit32/clear_bit32 etc. interfaces. Currently only on x86-64 aomitc_bitmask32 is defined to unsigned, everybody else still uses unsigned long. The other 64bit architectures can define it to unsigned too if they can confirm that it's ok to do.

I have no problem with this.

If you are clever, you can define a generic version even for the "unsigned long" 64-bit platforms. It's left as an exercise to the reader :-)

#2 lore raw
Quoted 9 lines · David S. Miller

From: Andi Kleen <[email protected]>
Date: Tue, 15 Apr 2003 13:24:30 +0200

I worked around this by declaring a new data type atomic_bitmask32 with matching set_bit32/clear_bit32 etc. interfaces. Currently only on x86-64 aomitc_bitmask32 is defined to unsigned, everybody else still uses unsigned long. The other 64bit architectures can define it to unsigned too if they can confirm that it's ok to do.

Quoted 5 lines

I have no problem with this.

If you are clever, you can define a generic version even for the "unsigned long" 64-bit platforms. It's left as an exercise to the reader :-)

Why is any new macro needed?

struct page
{
#if BITS_PER_LONG == 64 && defined __BIG_ENDIAN

  union {
    unsigned long flags;
    atomic_t count;
  };
#elif BITS_PER_LONG == 64 && defined __LITTLE_ENDIAN
  union {
    unsigned long flags;
    struct {
      int : 32;
      atomic_t count;
    };

};
#else
unsigned long flags;
atomic_t count;
#endif
...
};

should do the job.

Jakub

author
#3 lore raw
In reply to David S. Miller
Quoted 9 lines · David S. Miller

From: Andi Kleen <[email protected]>
Date: Tue, 15 Apr 2003 13:24:30 +0200

I worked around this by declaring a new data type atomic_bitmask32 with matching set_bit32/clear_bit32 etc. interfaces. Currently only on x86-64 aomitc_bitmask32 is defined to unsigned, everybody else still uses unsigned long. The other 64bit architectures can define it to unsigned too if they can confirm that it's ok to do.

Quoted 5 lines

I have no problem with this.

If you are clever, you can define a generic version even for the "unsigned long" 64-bit platforms. It's left as an exercise to the reader :-)

How so? Of course I could write an generic set_bit32, but the question is if these bit operations would be still atomic on SMP and not conflict with fields occuping the same 8 byte slot. I remember you flaming someone some time ago because he used set_bit in an atomic fashion on a type smaller than unsigned long for example.

-Andi

#4 lore raw

From: Andi Kleen <[email protected]>
Date: Wed, 16 Apr 2003 16:07:15 +0200

How so? Of course I could write an generic set_bit32, but the question is if these bit operations would be still atomic on SMP and not conflict with fields occuping the same 8 byte slot. I remember you flaming someone some time ago because he used set_bit in an atomic fashion on a type smaller than unsigned long for example.

It's OK if you align the pointer to 8 bytes, and subsequently the bit offset as appropriate. :-)

author
#5 lore raw
Quoted 11 lines · David S. Miller

From: Andi Kleen <[email protected]>
Date: Wed, 16 Apr 2003 16:07:15 +0200

How so? Of course I could write an generic set_bit32, but the question is if these bit operations would be still atomic on SMP and not conflict with fields occuping the same 8 byte slot. I remember you flaming someone some time ago because he used set_bit in an atomic fashion on a type smaller than unsigned long for example.

It's OK if you align the pointer to 8 bytes, and subsequently the bit offset as appropriate. :-)

On sparc64. But is that true too for all other 64bit architectures supported?

e.g. How about PA-RISC? (always seems to do things differently)

-Andi

#6 lore raw

From: Andi Kleen <[email protected]>
Date: Wed, 16 Apr 2003 16:43:12 +0200

On sparc64. But is that true too for all other 64bit architectures supported?

e.g. How about PA-RISC? (always seems to do things differently)

It cannot require more than the existing API requires, which is "unsigned long *bitmask", ie. anything equivalent in behavior to an unsigned long pointer is good enough.

author
#7 lore raw
Quoted 10 lines · David S. Miller

From: Andi Kleen <[email protected]>
Date: Wed, 16 Apr 2003 16:43:12 +0200

On sparc64. But is that true too for all other 64bit architectures supported?

e.g. How about PA-RISC? (always seems to do things differently)

It cannot require more than the existing API requires, which is "unsigned long *bitmask", ie. anything equivalent in behavior to an unsigned long pointer is good enough.

Sure, but perhaps it does assume all accesses to this bitmap are going through the set_bit functions. e.g. consider an implementation that uses a spinlock for this - parisc seems to do that for example from a quick look into their bitops.h. And they have an 64bit kernel too.

In this case part of the unsigned long could be accessed directly using the aliasing.

-Andi

#8 lore raw

From: Andi Kleen <[email protected]>
Date: Wed, 16 Apr 2003 16:58:53 +0200

In this case part of the unsigned long could be accessed directly using the aliasing.

They use the same ATOMIC_HASH() for both atomic_t's and bitops. I don't see how an alias can be generated. This thing must work for any ordering of unsigned long *'s and atomic_t's.

#9 lore raw
In reply to Andi Kleen
Quoted 3 lines · Andi Kleen

On sparc64. But is that true too for all other 64bit architectures supported?

e.g. How about PA-RISC? (always seems to do things differently)

As you know our only two atomic ops are load-and-clear 32-bit quantity and load-and-clear 64-bit quantity. so we take one of the hashed spinlocks ..

Signature · 3 lines
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk
author
#10 lore raw
Quoted 7 lines · Matthew Wilcox
Quoted 3 lines · Andi Kleen

On sparc64. But is that true too for all other 64bit architectures supported?

e.g. How about PA-RISC? (always seems to do things differently)

As you know our only two atomic ops are load-and-clear 32-bit quantity and load-and-clear 64-bit quantity. so we take one of the hashed spinlocks ..

Sure, but you use a 64bit read/store in set_bit/clear_bit etc., right?

If yes then you can't use this unless you rewrite them to use 32bit store
- otherwise it will conflict with the atomic_t counter in the 64bit slot which is not protected.

I think my current patch is fine for you - you can still optimize it this way, but it should already work. Jakub's version would break though.

-Andi

#11 lore raw

From: Andi Kleen <[email protected]>
Date: Wed, 16 Apr 2003 17:04:27 +0200

Sure, but you use a 64bit read/store in set_bit/clear_bit etc., right?

If yes then you can't use this unless you rewrite them to use 32bit store
- otherwise it will conflict with the atomic_t counter in the 64bit slot which is not protected.

It will be protected by the same spinlock, look at how the thing works :-)

#12 lore raw
In reply to Andi Kleen
Quoted 10 lines · Andi Kleen
Quoted 7 lines · Matthew Wilcox
Quoted 3 lines · Andi Kleen

On sparc64. But is that true too for all other 64bit architectures supported?

e.g. How about PA-RISC? (always seems to do things differently)

As you know our only two atomic ops are load-and-clear 32-bit quantity and load-and-clear 64-bit quantity. so we take one of the hashed spinlocks ..

Sure, but you use a 64bit read/store in set_bit/clear_bit etc., right?

err.. so what? Once we've got the spinlock, other writers are blocked and readers can either see the state before or the state after (stores are atomic on PA)

Quoted 6 lines

If yes then you can't use this unless you rewrite them to use 32bit store
- otherwise it will conflict with the atomic_t counter in the 64bit slot which is not protected.

I think my current patch is fine for you - you can still optimize it this way, but it should already work. Jakub's version would break though.

Jacob's would break if we hashed to different spinlocks. But we don't, we shift right by 8, so we get the same spinlock for atomic things that are on the same "cacheline" (i think PA cachelines are actually 64 or 128 bytes, depending on model).

Signature · 3 lines
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk
#13 lore raw
Quoted 6 lines · Matthew Wilcox

Jacob's would break if we hashed to different spinlocks. But we don't, we shift right by 8, so we get the same spinlock for atomic things that are on the same "cacheline" (i think PA cachelines are actually 64 or 128 bytes, depending on model).

Are you prepared to cast this in stone?

If so then I think Jacob's approach would be preferable, don't you agree?

It needs some compiler-detection magic around the anon union though.

#14 lore raw
Quoted 9 lines · Andrew Morton
Quoted 6 lines · Matthew Wilcox

Jacob's would break if we hashed to different spinlocks. But we don't, we shift right by 8, so we get the same spinlock for atomic things that are on the same "cacheline" (i think PA cachelines are actually 64 or 128 bytes, depending on model).

Are you prepared to cast this in stone?

I think so. It makes sense to me that we lock an entire cacheline for this kind of thing. Indeed, locking a smaller amount would probably break other stuff. Remember set_bit() et al take a pointer to an unsigned long... but can take a bit number > number of bits in an unsigned long. If anything, we should maybe expand the range covered by a single lock to a larger amount than 256 bytes. How big are ext2 bitmaps, for example?

Signature · 3 lines
"It's not Hollywood.  War is real, war is primarily not about defeat or
victory, it is about death.  I've seen thousands and thousands of dead bodies.
Do you think I want to have an academic debate on this subject?" -- Robert Fisk
#15 lore raw
Quoted 10 lines · Matthew Wilcox
Quoted 8 lines · Andrew Morton
Quoted 6 lines · Matthew Wilcox

Jacob's would break if we hashed to different spinlocks. But we don't, we shift right by 8, so we get the same spinlock for atomic things that are on the same "cacheline" (i think PA cachelines are actually 64 or 128 bytes, depending on model).

Quoted 8 lines
Quoted 1 line

Are you prepared to cast this in stone?

I think so. It makes sense to me that we lock an entire cacheline for this kind of thing. Indeed, locking a smaller amount would probably break other stuff. Remember set_bit() et al take a pointer to an unsigned long... but can take a bit number > number of bits in an unsigned long. If anything, we should maybe expand the range covered by a single lock to a larger amount than 256 bytes.

Well are we sure that the flags' and count' fields will always fall into the same 256-byte range? Wouldn't it subtly break if sizeof(struct page) became not a multiple of eight? Will the compiler pad it out anyway?

Quoted 1 line

How big are ext2 bitmaps, for example?

They can be 8k, rarely. Usually 4k. But ext2_set_bit() and friends are nonatomic - the filesystem does the locking.

We just added the new ext2_set/clear_bit_atomic() functions which _are_ supposed to be atomic. The architecture is passed a spinlock which it can use for that, but only big-endian architectures are likely to need it.

#16 lore raw

From: Andrew Morton <[email protected]>
Date: Wed, 16 Apr 2003 14:43:11 -0700

Well are we sure that the flags' and count' fields will always fall into the same 256-byte range? Wouldn't it subtly break if sizeof(struct page) became not a multiple of eight? Will the compiler pad it out anyway?

As long as there is a long or pointer member, the structure will be required to be 8 byte or better aligned.

author
#17 lore raw
Quoted 9 lines · David S. Miller

From: Andrew Morton <[email protected]>
Date: Wed, 16 Apr 2003 14:43:11 -0700

Well are we sure that the flags' and count' fields will always fall into the same 256-byte range? Wouldn't it subtly break if sizeof(struct page) became not a multiple of eight? Will the compiler pad it out anyway?

As long as there is a long or pointer member, the structure will be required to be 8 byte or better aligned.

Yes that should be a safe assumption. mem_map will be always page aligned and it should be a multiple of a power of 8. That makes it impossible for it to cross a cache line.

-Andi