pgd_init() Patch

pgd_init() Patch

[Phil Thompson]

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

While trying to understand the changes needed for discontiguous memory I
came across this...

In paging_init() there are two calls to pgd_init()...

        /* Initialize the entire pgd.  */
        pgd_init((unsigned long)swapper_pg_dir);
        pgd_init((unsigned long)swapper_pg_dir + PAGE_SIZE / 2);

...where the assumption seems to be that the PGD is one page and each
call initialises half of it. Most of the CPU implementations of
pgd_init() nearly do this because they initialise USER_PTRS_PER_PGD
entries. The problems with this are...

- pg-r3k.c and pg-r4k.S don't use USER_PTRS_PER_PGD and initialise a
page's worth each time. This means the second call to pgd_init()
initialises half a page's worth of memory after the end of
swapper_pg_dir.

- USER_PTRS_PER_PGD is defined as TASK_SIZE/PGDIR_SIZE. However,
because, TASK_SIZE is actually defined as one less that the maximum task
size there is a rounding error that means that USER_PTRS_PER_PGD works
out at 511 rather than 512. This means that entries 511 and 1023 of
swapper_pg_dir don't get initialised.

The corresponding mips64 code has only the first call to pgd_init() and
each implementation of pgd_init() initialises PTRS_PER_PGD entries,
where PTRS_PER_PGD is simple defined as 1024.

The attached patch applies the mips64 approach to the mips code.

Should USER_PTRS_PER_PGD be defined as (TASK_SIZE/PGDIR_SIZE) + 1?

Phil

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

diff -ruN mm.orig/c-sb1.c mm/c-sb1.c
--- mm.orig/c-sb1.c	Wed Dec 12 16:25:01 2001
+++ mm/c-sb1.c	Mon Jan 28 12:37:20 2002
@@ -44,7 +44,7 @@
 	unsigned long *p = (unsigned long *) page;
 	int i;
 
-	for (i = 0; i < USER_PTRS_PER_PGD; i+=8) {
+	for (i = 0; i < PTRS_PER_PGD; i+=8) {
 		p[i + 0] = (unsigned long) invalid_pte_table;
 		p[i + 1] = (unsigned long) invalid_pte_table;
 		p[i + 2] = (unsigned long) invalid_pte_table;
diff -ruN mm.orig/init.c mm/init.c
--- mm.orig/init.c	Fri Jan 25 14:17:12 2002
+++ mm/init.c	Mon Jan 28 12:37:36 2002
@@ -152,7 +152,6 @@
 
 	/* Initialize the entire pgd.  */
 	pgd_init((unsigned long)swapper_pg_dir);
-	pgd_init((unsigned long)swapper_pg_dir + PAGE_SIZE / 2);
 
 	max_dma = virt_to_phys((char *)MAX_DMA_ADDRESS) >> PAGE_SHIFT;
 	low = max_low_pfn;
diff -ruN mm.orig/pg-mips32.c mm/pg-mips32.c
--- mm.orig/pg-mips32.c	Tue Oct 23 02:02:46 2001
+++ mm/pg-mips32.c	Mon Jan 28 12:37:56 2002
@@ -127,7 +127,7 @@
 	unsigned long *p = (unsigned long *) page;
 	int i;
 
-	for(i = 0; i < USER_PTRS_PER_PGD; i+=8) {
+	for(i = 0; i < PTRS_PER_PGD; i+=8) {
 		p[i + 0] = (unsigned long) invalid_pte_table;
 		p[i + 1] = (unsigned long) invalid_pte_table;
 		p[i + 2] = (unsigned long) invalid_pte_table;
diff -ruN mm.orig/pg-r5432.c mm/pg-r5432.c
--- mm.orig/pg-r5432.c	Tue Oct 23 02:02:46 2001
+++ mm/pg-r5432.c	Mon Jan 28 12:38:30 2002
@@ -104,7 +104,7 @@
 	unsigned long *p = (unsigned long *) page;
 	int i;
 
-	for(i = 0; i < USER_PTRS_PER_PGD; i+=8) {
+	for(i = 0; i < PTRS_PER_PGD; i+=8) {
 		p[i + 0] = (unsigned long) invalid_pte_table;
 		p[i + 1] = (unsigned long) invalid_pte_table;
 		p[i + 2] = (unsigned long) invalid_pte_table;
diff -ruN mm.orig/pg-rm7k.c mm/pg-rm7k.c
--- mm.orig/pg-rm7k.c	Tue Oct 23 02:02:46 2001
+++ mm/pg-rm7k.c	Mon Jan 28 12:38:38 2002
@@ -107,7 +107,7 @@
 	unsigned long *p = (unsigned long *) page;
 	int i;
 
-	for (i = 0; i < USER_PTRS_PER_PGD; i+=8) {
+	for (i = 0; i < PTRS_PER_PGD; i+=8) {
 		p[i + 0] = (unsigned long) invalid_pte_table;
 		p[i + 1] = (unsigned long) invalid_pte_table;
 		p[i + 2] = (unsigned long) invalid_pte_table;

Re: pgd_init() Patch

[Geert Uytterhoeven]

On Mon, 28 Jan 2002, Phil Thompson wrote:
> - USER_PTRS_PER_PGD is defined as TASK_SIZE/PGDIR_SIZE. However,
> because, TASK_SIZE is actually defined as one less that the maximum task
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> size there is a rounding error that means that USER_PTRS_PER_PGD works
  ^^^^
> out at 511 rather than 512. This means that entries 511 and 1023 of
> swapper_pg_dir don't get initialised.
> 
> The corresponding mips64 code has only the first call to pgd_init() and
> each implementation of pgd_init() initialises PTRS_PER_PGD entries,
> where PTRS_PER_PGD is simple defined as 1024.
> 
> The attached patch applies the mips64 approach to the mips code.
> 
> Should USER_PTRS_PER_PGD be defined as (TASK_SIZE/PGDIR_SIZE) + 1?

You mean ((TASK_SIZE)+1)/PGDIR_SIZE?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: pgd_init() Patch

[Pete Popov]

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

On Mon, 2002-01-28 at 11:53, Geert Uytterhoeven wrote:
> On Mon, 28 Jan 2002, Phil Thompson wrote:
> > - USER_PTRS_PER_PGD is defined as TASK_SIZE/PGDIR_SIZE. However,
> > because, TASK_SIZE is actually defined as one less that the maximum task
>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > size there is a rounding error that means that USER_PTRS_PER_PGD works
>   ^^^^
> > out at 511 rather than 512. This means that entries 511 and 1023 of
> > swapper_pg_dir don't get initialised.
> > 
> > The corresponding mips64 code has only the first call to pgd_init() and
> > each implementation of pgd_init() initialises PTRS_PER_PGD entries,
> > where PTRS_PER_PGD is simple defined as 1024.
> > 
> > The attached patch applies the mips64 approach to the mips code.
> > 
> > Should USER_PTRS_PER_PGD be defined as (TASK_SIZE/PGDIR_SIZE) + 1?
> 
> You mean ((TASK_SIZE)+1)/PGDIR_SIZE?

Or how about:

#define USER_PTRS_PER_PGD      ((TASK_SIZE-1)/PGDIR_SIZE + 1)


The above patch fixes a rather serious memory leak.  When a parent
process forks a large number of children and then it exits before the
children exit, we lose one page per child. I was able to narrow down the
problem and reproduce it with the attached program.  Ralf has the fix,
but was examining related issues before applying the patch.

Pete


[-- Attachment #2: mleak.c --]
[-- Type: text/x-c, Size: 394 bytes --]

 
#include <signal.h>

#define NUM_TSK 200
int main()
{
	int i, lastp;
	int pids[NUM_TSK];

	printf("mypid %d\n", getpid());
	for (i=0; i<NUM_TSK; i++) {
		switch (pids[i] = fork()) {
			case -1:
				printf("fork failed, lastp %d\n", lastp);
				goto killall;
			case 0:
				sleep(4);
				return 0;
				break;
			default:
				lastp = i;
		}
	}

killall:
	return 0;
}

Re: pgd_init() Patch

[Jun Sun]

Geert Uytterhoeven wrote:
> 
> On Mon, 28 Jan 2002, Phil Thompson wrote:
> > - USER_PTRS_PER_PGD is defined as TASK_SIZE/PGDIR_SIZE. However,
> > because, TASK_SIZE is actually defined as one less that the maximum task
>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > size there is a rounding error that means that USER_PTRS_PER_PGD works
>   ^^^^
> > out at 511 rather than 512. This means that entries 511 and 1023 of
> > swapper_pg_dir don't get initialised.
> >
> > The corresponding mips64 code has only the first call to pgd_init() and
> > each implementation of pgd_init() initialises PTRS_PER_PGD entries,
> > where PTRS_PER_PGD is simple defined as 1024.
> >
> > The attached patch applies the mips64 approach to the mips code.
> >
> > Should USER_PTRS_PER_PGD be defined as (TASK_SIZE/PGDIR_SIZE) + 1?
> 
> You mean ((TASK_SIZE)+1)/PGDIR_SIZE?
> 

No.  It should be

+#define USER_PTRS_PER_PGD      ((TASK_SIZE-1)/PGDIR_SIZE + 1)

Mathmatically, 

USER_PTRS_PER_PGD=ceil(TASK_SIZE/PGDIR_SIZE)

I submitted this patch to Ralf on Jan 03.  Ralf, any reason for not applying
this?  This formula ensures PTRS_PER_PGD is always correct irrespect of the
values of TAKS_SIZE and PGDIR_SIZE.

See this patch and other pending patches at

http://linux.junsun.net/patches/oss.sgi.com/submitted

Jun

Re: pgd_init() Patch

[Geert Uytterhoeven]

On Mon, 28 Jan 2002, Jun Sun wrote:
> Geert Uytterhoeven wrote:
> > On Mon, 28 Jan 2002, Phil Thompson wrote:
> > > Should USER_PTRS_PER_PGD be defined as (TASK_SIZE/PGDIR_SIZE) + 1?
> > 
> > You mean ((TASK_SIZE)+1)/PGDIR_SIZE?
> > 
> 
> No.  It should be
> 
> +#define USER_PTRS_PER_PGD      ((TASK_SIZE-1)/PGDIR_SIZE + 1)
> 
> Mathmatically, 
> 
> USER_PTRS_PER_PGD=ceil(TASK_SIZE/PGDIR_SIZE)

OK, ((TASK_SIZE+PGDIR_SIZE-1)/PGDIR_SIZE) in that case :-)

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds