Re: PAGE_SIZE Availability Inconsistency

Re: PAGE_SIZE Availability Inconsistency

[Christoph Hellwig]

On Thu, Mar 08, 2007 at 07:53:49AM -0800, Arjan van de Ven wrote:
> > > > PAGE_SIZE should not be available at all.  Please use getpagesize()
> > > > instead.
> > > 
> > > While I agree, NBPG is a bit of a problem, although it's only needed for aout 
> > > coredumps AFAICT, but still needed to compile e.g. gdb.
> > 
> > So we should export this one with an arbitrary value (on multiple page
> > size architectures) and a warning, maybe even an __deprecated attached to
> > it.
> 
> if we think the kernel should export this one, we could do
> 
> #ifndef __KERNEL__
> #define PAGE_SIZE getpagesize()
> #endif

No, no no.  We should never export PAGE_SIZE.  We might export NBPG
as deprecated symbol for gdb if it really needs it, but that should
happen only on a.out systems, and it it should be a true constant,
not depending on PAGE_SIZE.

I've Cc'ed the gdb list on whether they have any comments on this
issue.

Re: PAGE_SIZE Availability Inconsistency

[Daniel Jacobowitz]

On Thu, Mar 08, 2007 at 04:08:52PM +0000, Christoph Hellwig wrote:
> No, no no.  We should never export PAGE_SIZE.  We might export NBPG
> as deprecated symbol for gdb if it really needs it, but that should
> happen only on a.out systems, and it it should be a true constant,
> not depending on PAGE_SIZE.
> 
> I've Cc'ed the gdb list on whether they have any comments on this
> issue.

Sounds reasonable.  I do not believe that GDB has any dependence on
PAGE_SIZE; bfd (i.e. both gdb and binutils) use NBPG on a large number
of systems.  Looks like i386, alpha, m68k, s390, vax - but don't quote
me on that, I had to guess from the configure script.

-- 
Daniel Jacobowitz
CodeSourcery

Re: PAGE_SIZE Availability Inconsistency

[H. Peter Anvin]

Christoph Hellwig wrote:
> No, no no.  We should never export PAGE_SIZE.  We might export NBPG
> as deprecated symbol for gdb if it really needs it, but that should
> happen only on a.out systems, and it it should be a true constant,
> not depending on PAGE_SIZE.
> 
> I've Cc'ed the gdb list on whether they have any comments on this
> issue.

By the way, it's a massive snafu that the swap area magic number is 
dependent on PAGE_SIZE.  There is absolutely no good reason for that.

	-hp

Re: PAGE_SIZE Availability Inconsistency

[Christoph Hellwig]

On Thu, Mar 08, 2007 at 09:05:48AM -0800, H. Peter Anvin wrote:
> Christoph Hellwig wrote:
> >No, no no.  We should never export PAGE_SIZE.  We might export NBPG
> >as deprecated symbol for gdb if it really needs it, but that should
> >happen only on a.out systems, and it it should be a true constant,
> >not depending on PAGE_SIZE.
> >
> >I've Cc'ed the gdb list on whether they have any comments on this
> >issue.
> 
> By the way, it's a massive snafu that the swap area magic number is 
> dependent on PAGE_SIZE.  There is absolutely no good reason for that.

Yeah, now that you mention it I remember having problems with that
in the past.  We should probably create a new swap format that avoids
this problem.  I'll put it on my ever growing todo list.

Re: PAGE_SIZE Availability Inconsistency

[Anton Blanchard]

> By the way, it's a massive snafu that the swap area magic number is 
> dependent on PAGE_SIZE.  There is absolutely no good reason for that.

Agreed, its been a big problem booting between 4kB and 64kB kernels on
ppc64.

Anton

Re: PAGE_SIZE Availability Inconsistency

[H. Peter Anvin]

Anton Blanchard wrote:
>> By the way, it's a massive snafu that the swap area magic number is 
>> dependent on PAGE_SIZE.  There is absolutely no good reason for that.
> 
> Agreed, its been a big problem booting between 4kB and 64kB kernels on
> ppc64.

The easiest way to fix this would be to always park the swap magic at 
the offset of the smallest page size in use, which is 4K.  This is 
analogous how the offset for the ext2/3 superblock got fixed at 1K -- 
for 1K blocks, it's the second block, but for larger blocks, it's part 
of the first block.  If we fix the offset of the swap magic at 4096 
minus the offset that's already there, it will always fall in the first 
page regardless of page size.

	-hpa

Re: PAGE_SIZE Availability Inconsistency

[Anton Blanchard]

 
Hi Peter,

> The easiest way to fix this would be to always park the swap magic at 
> the offset of the smallest page size in use, which is 4K.  This is 
> analogous how the offset for the ext2/3 superblock got fixed at 1K -- 
> for 1K blocks, it's the second block, but for larger blocks, it's part 
> of the first block.  If we fix the offset of the swap magic at 4096 
> minus the offset that's already there, it will always fall in the first 
> page regardless of page size.

Yeah that makes sense. I gave it a go by creating a MIN_PAGE_SIZE
define, and allowing an architecture to override it if required.

A couple of issues:

1. Parts of the swap header are in PAGE_SIZE chunks so I made them
MIN_PAGE_SIZE chunks too.

2. The badblocks stuff is PAGE_SIZEd too. Do we ever use it on modern
disks? Maybe we can just remove this support.

3. This will unfortunately break machines currently running a 64kB
kernel with swap space. We may just have to lump it and fix on upgade.

Anton
--

Our current swap layout has issues with variable page size kernels.
Instead of using the page size at runtime, base it on the minimum page
size the architecture supports.

Signed-off-by: Anton Blanchard <anton@samba.org>
---

Index: linux-2.6/include/linux/swap.h
===================================================================
--- linux-2.6.orig/include/linux/swap.h	2007-03-08 15:14:30.000000000 -0600
+++ linux-2.6/include/linux/swap.h	2007-03-08 15:14:33.000000000 -0600
@@ -48,14 +48,17 @@
  * old reserved area - some extra information. Note that the first
  * kilobyte is reserved for boot loader or disk label stuff...
  *
- * Having the magic at the end of the PAGE_SIZE makes detecting swap
- * areas somewhat tricky on machines that support multiple page sizes.
- * For 2.5 we'll probably want to move the magic to just beyond the
- * bootbits...
+ * Version 1 and 2 swap headers store the magic at the end of the
+ * PAGE_SIZE which causes problems for architectures with multiple
+ * page sizes. An architecture can define MIN_PAGE_SIZE to be used
+ * regardless of the kernel page size to get around this.
  */
+#ifndef MIN_PAGE_SIZE
+#define MIN_PAGE_SIZE PAGE_SIZE
+#endif
 union swap_header {
 	struct {
-		char reserved[PAGE_SIZE - 10];
+		char reserved[MIN_PAGE_SIZE - 10];
 		char magic[10];			/* SWAP-SPACE or SWAPSPACE2 */
 	} magic;
 	struct {
Index: linux-2.6/include/asm-powerpc/page.h
===================================================================
--- linux-2.6.orig/include/asm-powerpc/page.h	2007-03-08 15:14:30.000000000 -0600
+++ linux-2.6/include/asm-powerpc/page.h	2007-03-08 15:14:33.000000000 -0600
@@ -24,8 +24,10 @@
 #else
 #define PAGE_SHIFT		12
 #endif
+#define MIN_PAGE_SHIFT		12
 
 #define PAGE_SIZE		(ASM_CONST(1) << PAGE_SHIFT)
+#define MIN_PAGE_SIZE		(ASM_CONST(1) << MIN_PAGE_SHIFT)
 
 /* We do define AT_SYSINFO_EHDR but don't use the gate mechanism */
 #define __HAVE_ARCH_GATE_AREA		1
Index: linux-2.6/mm/swapfile.c
===================================================================
--- linux-2.6.orig/mm/swapfile.c	2007-03-08 14:48:03.000000000 -0600
+++ linux-2.6/mm/swapfile.c	2007-03-08 15:33:04.000000000 -0600
@@ -1568,6 +1568,12 @@
 		p->cluster_next = 1;
 
 		/*
+		 * last_page is in MIN_PAGE_SIZE chunks, scale to kernel
+		 * page size.
+		 */
+		swap_header->info.last_page >>= (PAGE_SHIFT - MIN_PAGE_SHIFT);
+
+		/*
 		 * Find out how many pages are allowed for a single swap
 		 * device. There are two limiting factors: 1) the number of
 		 * bits for the swap offset in the swp_entry_t type and

Re: PAGE_SIZE Availability Inconsistency

[Anton Blanchard]

Hi,

> Our current swap layout has issues with variable page size kernels.
> Instead of using the page size at runtime, base it on the minimum page
> size the architecture supports.

A hacked up patch to userspace utilities to test the kernel patch. BTW
It looks like there are some real bugs here:

            int pagesize = getpagesize();
            int rd;
            char buf[32768];

            rd = pagesize;
            if (rd < 8192)
                    rd = 8192;
            if (rd > sizeof(buf))
                    rd = sizeof(buf);
            if (lseek(fd, 0, SEEK_SET) != 0
                || read(fd, buf, rd) != rd)
                    goto io_error;
            if (may_be_swap(buf+pagesize) ||
                may_be_swap(buf+4096) || may_be_swap(buf+8192))
                    type = "swap";

If page size == 64kB wont we read past the end of buf?

Anton

Index: util-linux-2.12r/disk-utils/mkswap.c
===================================================================
--- util-linux-2.12r.orig/disk-utils/mkswap.c	2007-03-08 14:52:53.000000000 -0600
+++ util-linux-2.12r/disk-utils/mkswap.c	2007-03-08 14:58:09.000000000 -0600
@@ -169,7 +158,11 @@
 #ifdef PAGE_SIZE
 	defined_pagesize = PAGE_SIZE;
 #endif
+#ifdef __powerpc__
+	kernel_pagesize = 4096;
+#else
 	kernel_pagesize = getpagesize();
+#endif
 	pagesize = kernel_pagesize;
 
 	if (user_pagesize) {
Index: util-linux-2.12r/mount/get_label_uuid.c
===================================================================
--- util-linux-2.12r.orig/mount/get_label_uuid.c	2007-03-08 14:52:53.000000000 -0600
+++ util-linux-2.12r/mount/get_label_uuid.c	2007-03-08 14:58:09.000000000 -0600
@@ -79,7 +79,11 @@
 
 static int
 is_v1_swap_partition(int fd, char **label, char *uuid) {
+#ifdef __powerpc__
+	int n = 4096;
+#else
 	int n = getpagesize();
+#endif
 	char *buf = xmalloc(n);
 	struct swap_header_v1_2 *p = (struct swap_header_v1_2 *) buf;
 
Index: util-linux-2.12r/mount/mount_guess_fstype.c
===================================================================
--- util-linux-2.12r.orig/mount/mount_guess_fstype.c	2007-03-08 14:52:53.000000000 -0600
+++ util-linux-2.12r/mount/mount_guess_fstype.c	2007-03-08 14:54:25.000000000 -0600
@@ -462,7 +462,11 @@
     if (!type) {
 	    /* perhaps the user tries to mount the swap space
 	       on a new disk; warn her before she does mke2fs on it */
+#ifdef __powerpc__
+	    int pagesize = 4096;
+#else
 	    int pagesize = getpagesize();
+#endif
 	    int rd;
 	    char buf[32768];
 
Index: util-linux-2.12r/rescuept/rescuept.c
===================================================================
--- util-linux-2.12r.orig/rescuept/rescuept.c	2007-03-08 14:52:53.000000000 -0600
+++ util-linux-2.12r/rescuept/rescuept.c	2007-03-08 14:55:49.000000000 -0600
@@ -510,7 +510,11 @@
 		size = s.st_size / 512;
 	}
 
+#ifdef __powerpc__
+	pagesize = 4096;
+#else
 	pagesize = getpagesize();
+#endif
 	if (pagesize <= 0)
 		pagesize = 4096;
 	else if (pagesize > MAXPAGESZ) {

Re: PAGE_SIZE Availability Inconsistency

[David Miller]

From: Anton Blanchard <anton@samba.org>
Date: Thu, 8 Mar 2007 15:42:36 -0600

> > The easiest way to fix this would be to always park the swap magic at 
> > the offset of the smallest page size in use, which is 4K.  This is 
> > analogous how the offset for the ext2/3 superblock got fixed at 1K -- 
> > for 1K blocks, it's the second block, but for larger blocks, it's part 
> > of the first block.  If we fix the offset of the swap magic at 4096 
> > minus the offset that's already there, it will always fall in the first 
> > page regardless of page size.
> 
> Yeah that makes sense. I gave it a go by creating a MIN_PAGE_SIZE
> define, and allowing an architecture to override it if required.

I might be missing something but doesn't this break every
SWAP partition that was created with something other than
MIN_PAGE_SIZE?

Re: PAGE_SIZE Availability Inconsistency

[Anton Blanchard]

Hi,

> I might be missing something but doesn't this break every
> SWAP partition that was created with something other than
> MIN_PAGE_SIZE?

It does. I was thinking we could work around it in ppc64 (64kB is quite
new), but I forgot there are options on sparc64 to change the page size :)

The other option is to create a v3 swap format that doesnt use any
PAGE_SIZE parameters.

Anton

Re: PAGE_SIZE Availability Inconsistency

[H. Peter Anvin]

Anton Blanchard wrote:
> Hi,
> 
>> I might be missing something but doesn't this break every
>> SWAP partition that was created with something other than
>> MIN_PAGE_SIZE?
> 
> It does. I was thinking we could work around it in ppc64 (64kB is quite
> new), but I forgot there are options on sparc64 to change the page size :)
> 
> The other option is to create a v3 swap format that doesnt use any
> PAGE_SIZE parameters.
> 

The best thing to do would be to look for the magic both at PAGE_SIZE 
(for compatibility) and MIN_PAGE_SIZE (for sanity.)

	-hpa

Re: PAGE_SIZE Availability Inconsistency

[H. Peter Anvin]

David Miller wrote:
> From: "H. Peter Anvin" <hpa@zytor.com>
> Date: Thu, 08 Mar 2007 20:18:28 -0800
> 
>> Anton Blanchard wrote:
>>> The other option is to create a v3 swap format that doesnt use any
>>> PAGE_SIZE parameters.
>> The best thing to do would be to look for the magic both at PAGE_SIZE 
>> (for compatibility) and MIN_PAGE_SIZE (for sanity.)
> 
> That might work, but a large part of me says to go for v3
> and do it cleanly.

The advantage would be that it wouldn't require a v3 for platforms for 
which MIN_PAGE_SIZE == PAGE_SIZE, which accounts for a very large 
percentage of systems.

You still have to look for the darn magic in two places, so there is no 
reason for it to be different.

	-hpa

Re: PAGE_SIZE Availability Inconsistency

[David Miller]

From: "H. Peter Anvin" <hpa@zytor.com>
Date: Thu, 08 Mar 2007 20:31:05 -0800

> The advantage would be that it wouldn't require a v3 for platforms for 
> which MIN_PAGE_SIZE == PAGE_SIZE, which accounts for a very large 
> percentage of systems.
> 
> You still have to look for the darn magic in two places, so there is no 
> reason for it to be different.

Good point.

Re: PAGE_SIZE Availability Inconsistency

[Anton Blanchard]

Hi,

> The advantage would be that it wouldn't require a v3 for platforms for 
> which MIN_PAGE_SIZE == PAGE_SIZE, which accounts for a very large 
> percentage of systems.
> 
> You still have to look for the darn magic in two places, so there is no 
> reason for it to be different.

The problem is if you can hit in two places then what PAGE_SIZE should
you use to size the contents of the swap header while remaining backward
compatible.

Im leaning towards Dave suggestion of creating a clean v3 swap header.

Anton

Re: PAGE_SIZE Availability Inconsistency

[H. Peter Anvin]

Anton Blanchard wrote:
> Hi,
> 
>> The advantage would be that it wouldn't require a v3 for platforms for 
>> which MIN_PAGE_SIZE == PAGE_SIZE, which accounts for a very large 
>> percentage of systems.
>>
>> You still have to look for the darn magic in two places, so there is no 
>> reason for it to be different.
> 
> The problem is if you can hit in two places then what PAGE_SIZE should
> you use to size the contents of the swap header while remaining backward
> compatible.
> 
> Im leaning towards Dave suggestion of creating a clean v3 swap header.
> 

Changing the header format doesn't make *ANY* difference whatsoever.

You have to write two copies of the swap header, and the kernel should 
check for a header at MIN_PAGE_SIZE first and then at PAGE_SIZE.

If there are fields (other than position) in the v2 swap header that are 
dependent on PAGE_SIZE, then the copy at MIN_PAGE_SIZE should be sized 
using MIN_PAGE_SIZE, and the copy at PAGE_SIZE should be sized at 
PAGE_SIZE.  It's that simple.

Creating a new format will not help that one iota, and will create 
gratuitous incompatiblity for the very common case of PAGE_SIZE == 
MIN_PAGE_SIZE.

	-hpa


	-hpa

Re: PAGE_SIZE Availability Inconsistency

[H. Peter Anvin]

Anton Blanchard wrote:
> 
> 2. The badblocks stuff is PAGE_SIZEd too. Do we ever use it on modern
> disks? Maybe we can just remove this support.
> 

Badblocks is definitely still used in some configurations.

	-hpa

Re: PAGE_SIZE Availability Inconsistency

[David Brown]

On 3/8/07, Anton Blanchard <anton@samba.org> wrote:
>
> > By the way, it's a massive snafu that the swap area magic number is
> > dependent on PAGE_SIZE.  There is absolutely no good reason for that.
>
> Agreed, its been a big problem booting between 4kB and 64kB kernels on
> ppc64.

Okay this really seems like a couple of things need to happen, first
change swap dependency on PAGE_SIZE, then move the __KERNEL__ define
above the PAGE_SIZE and friends defines in the appropriate
asm-*/page.h files.

Do these tasks need to happen in this order? I haven't really looked
at the swap code at all...

Also, I'd be willing to help I've done kernel coding for experimental
projects and such but nothing for kernel.org so I might need some
shepherding.

- David Brown