Skip to contentSkip to conversation

regression: 2f569af (CONFIG_HIGHPTE vs. sub-page page tables.)

DISCUSSION Uwe Kleine-König 4 participants 13 messages Feb 25, 2008 – Feb 29, 2008

Subsystems (4)
Memory management Arm (32-bit) MM core Generic include/asm header files
lore
Share & tools
lore

Uwe Kleine-König

author
#0 lore raw

Hello,

I see the following:

	/ # for i in 1 2 3; do grep PageTables /proc/meminfo; done
	PageTables:          4 kB
	PageTables:   4294967292 kB
	PageTables:   4294967284 kB

and I bisected it down to 2f569af (CONFIG_HIGHPTE vs. sub-page page tables.) This still happens in 2.6.25-rc3.

I have not investigated further, so I cannot tell if it's only the output in meminfo that is broken.

This is on ARCH=arm, on a Digi cc9p9360 with ns9xxx_defconfig.

Best regards
Uwe

Signature · 3 lines
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
#1 lore raw
Quoted 9 lines · Uwe Kleine-König

Hello,

I see the following:

	/ # for i in 1 2 3; do grep PageTables /proc/meminfo; done
	PageTables:          4 kB
	PageTables:   4294967292 kB
	PageTables:   4294967284 kB
Quoted 7 lines

and I bisected it down to 2f569af (CONFIG_HIGHPTE vs. sub-page page tables.) This still happens in 2.6.25-rc3.

I have not investigated further, so I cannot tell if it's only the output in meminfo that is broken.

This is on ARCH=arm, on a Digi cc9p9360 with ns9xxx_defconfig.

Note that there's been other reports of this on other ARM platforms as well.

Signature · 3 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:
author
#2 lore raw

Hello,

Quoted 13 lines · Russell King
Quoted 12 lines · Uwe Kleine-König

Hello,

I see the following:

	/ # for i in 1 2 3; do grep PageTables /proc/meminfo; done
	PageTables:          4 kB
	PageTables:   4294967292 kB
	PageTables:   4294967284 kB

and I bisected it down to 2f569af (CONFIG_HIGHPTE vs. sub-page page tables.) This still happens in 2.6.25-rc3.

Quoted 7 lines
Quoted 4 lines

I have not investigated further, so I cannot tell if it's only the output in meminfo that is broken.

This is on ARCH=arm, on a Digi cc9p9360 with ns9xxx_defconfig.

Note that there's been other reports of this on other ARM platforms as well.

I didn't see these. @Russell: can you point me to these reports?

With the patch below, I get two times printascii('-'), zero times printascii('+') and the following two stacktraces:

	[<c0120330>] (dump_stack+0x0/0x14) from [<c01232ec>] (free_pgd_slow+0xa4/0x10c)
	[<c0123248>] (free_pgd_slow+0x0/0x10c) from [<c01318d8>] (__mmdrop+0x24/0x54)
	 r7:c1c88000 r6:c14d8cc0 r5:c1c4cc40 r4:c14d8cc0
	[<c01318b4>] (__mmdrop+0x0/0x54) from [<c01319b4>] (mmput+0xac/0xc4)
	 r4:c14d8cc0
	[<c0131908>] (mmput+0x0/0xc4) from [<c018318c>] (flush_old_exec+0x320/0x680)
	 r4:c14d8b60
	[<c0182e6c>] (flush_old_exec+0x0/0x680) from [<c01b0460>] (load_elf_binary+0x3d4/0x15d0)
	[<c01b008c>] (load_elf_binary+0x0/0x15d0) from [<c0183af4>] (search_binary_handler+0xa4/0x1fc)
	[<c0183a50>] (search_binary_handler+0x0/0x1fc) from [<c0183db0>] (do_execve+0x164/0x188)
	[<c0183c4c>] (do_execve+0x0/0x188) from [<c011fb94>] (sys_execve+0x3c/0x5c)
	 r8:c011cb44 r7:c1c24000 r6:c1c89fb0 r5:0008cba8 r4:c1c24000
	[<c011fb58>] (sys_execve+0x0/0x5c) from [<c011c9a0>] (ret_fast_syscall+0x0/0x2c)
	 r7:0000000b r6:00000001 r5:0008cb88 r4:0008cba0

	[<c0120330>] (dump_stack+0x0/0x14) from [<c01232ec>] (free_pgd_slow+0xa4/0x10c)
	[<c0123248>] (free_pgd_slow+0x0/0x10c) from [<c01318d8>] (__mmdrop+0x24/0x54)
	 r7:c1c4cc40 r6:c14d8e20 r5:00000040 r4:c14d8b60
	[<c01318b4>] (__mmdrop+0x0/0x54) from [<c012d204>] (finish_task_switch+0x8c/0x90)
	 r4:c1c4cc40
	[<c012d178>] (finish_task_switch+0x0/0x90) from [<c0215614>] (schedule+0x1c0/0x2b0)
	 r5:c1c13bc0 r4:c14d8b60
	[<c0215454>] (schedule+0x0/0x2b0) from [<c01363d4>] (do_wait+0x278/0xbb0)
	[<c013615c>] (do_wait+0x0/0xbb0) from [<c0136da4>] (sys_wait4+0x98/0xd4)
	[<c0136d0c>] (sys_wait4+0x0/0xd4) from [<c011c9a0>] (ret_fast_syscall+0x0/0x2c)
	 r8:c011cb44 r7:00000072 r6:00075989 r5:00000000 r4:00090288

when executing /bin/true. I assume these are the two entries that go missing? Maybe this helps someone with a deeper understanding?

IMHO the last hunk is worth to make it into mainstream.

Best regards
Uwe

include/asm-arm/pgalloc.h +2 -2
index 163b030..0c85d94 100644
@@ -75,7 +75,7 @@ pte_alloc_one(struct mm_struct *mm, unsigned long addr)
 	if (pte) {
 		void *page = page_address(pte);
 		clean_dcache_area(page, sizeof(pte_t) * PTRS_PER_PTE);
-		pgtable_page_ctor(pte);
+		//pgtable_page_ctor(pte);
 	}
 
 	return pte;

@@ -94,7 +94,7 @@ static inline void pte_free_kernel(struct mm_struct *mm, pte_t *pte)
 
 static inline void pte_free(struct mm_struct *mm, pgtable_t pte)
 {
-	pgtable_page_dtor(pte);
+	//pgtable_page_dtor(pte);
 	__free_page(pte);
 }
include/linux/vmstat.h +11 -2
index 75370ec..5553d3f 100644
@@ -225,8 +225,13 @@ static inline void __mod_zone_page_state(struct zone *zone,
 	zone_page_state_add(delta, zone, item);
 }
 
+void printch(char);
 static inline void __inc_zone_state(struct zone *zone, enum zone_stat_item item)
 {
+	if (item == NR_PAGETABLE) {
+		printch('+');
+	}
+
 	atomic_long_inc(&zone->vm_stat[item]);
 	atomic_long_inc(&vm_stat[item]);
 }

@@ -239,6 +244,11 @@ static inline void __inc_zone_page_state(struct page *page,
 
 static inline void __dec_zone_state(struct zone *zone, enum zone_stat_item item)
 {
+	if (item == NR_PAGETABLE) {
+		printch('-');
+		dump_stack();
+	}
+
 	atomic_long_dec(&zone->vm_stat[item]);
 	atomic_long_dec(&vm_stat[item]);
 }

@@ -246,8 +256,7 @@ static inline void __dec_zone_state(struct zone *zone, enum zone_stat_item item)
 static inline void __dec_zone_page_state(struct page *page,
 			enum zone_stat_item item)
 {
-	atomic_long_dec(&page_zone(page)->vm_stat[item]);
-	atomic_long_dec(&vm_stat[item]);
+	__dec_zone_state(page_zone(page), item);
 }
 
 /*
Signature · 3 lines
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
author
#3 lore raw

let __dec_zone_page_state use __dec_zone_state

This removes code duplication and makes __dec_zone_page_state look like __inc_zone_page_state.

Signed-off-by Uwe Kleine-König <[email protected]>
Cc Christoph Lameter <[email protected]>

---

Quoted 1 line

IMHO the last hunk is worth to make it into mainstream.

So here comes the patch ...

Best regards
Uwe

 include/linux/vmstat.h |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
include/linux/vmstat.h +1 -2
index 75370ec..9f1b4b4 100644
@@ -246,8 +246,7 @@ static inline void __dec_zone_state(struct zone *zone, enum zone_stat_item item)
 static inline void __dec_zone_page_state(struct page *page,
 			enum zone_stat_item item)
 {
-	atomic_long_dec(&page_zone(page)->vm_stat[item]);
-	atomic_long_dec(&vm_stat[item]);
+	__dec_zone_state(page_zone(page), item);
 }
 
 /*
Signature · 6 lines
1.5.4.3

-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
author
#5 lore raw

Re: let __dec_zone_page_state use __dec_zone_state

Hello Christoph,

Quoted 6 lines · Christoph Lameter
Quoted 2 lines · Uwe Kleine-König

This removes code duplication and makes __dec_zone_page_state look like __inc_zone_page_state.

Acked-by Christoph Lameter <[email protected]>

Unless Linus or Andrew take that patch I think the best path for that patch into mainline is going over your tree.

Best regards
Uwe

Signature · 3 lines
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
#6 lore raw

Re: let __dec_zone_page_state use __dec_zone_state

Quoted 11 lines · Uwe Kleine-König

Hello Christoph,

Quoted 6 lines · Christoph Lameter
Quoted 2 lines · Uwe Kleine-König

This removes code duplication and makes __dec_zone_page_state look like __inc_zone_page_state.

Acked-by Christoph Lameter <[email protected]>

Unless Linus or Andrew take that patch I think the best path for that patch into mainline is going over your tree.

I only do slab stuff via my tree. This has to go through Andrew.

#7 lore raw
In reply to Uwe Kleine-König
Quoted 7 lines · Uwe Kleine-König

I see the following:

	/ # for i in 1 2 3; do grep PageTables /proc/meminfo; done
	PageTables:          4 kB
	PageTables:   4294967292 kB
	PageTables:   4294967284 kB
Quoted 7 lines

and I bisected it down to 2f569af (CONFIG_HIGHPTE vs. sub-page page tables.) This still happens in 2.6.25-rc3.

I have not investigated further, so I cannot tell if it's only the output in meminfo that is broken.

This is on ARCH=arm, on a Digi cc9p9360 with ns9xxx_defconfig.

Hmm, not good. The number obviously went negative. There is an imbalance in the number of pgtable_page_ctor vs. pgtable_page_dtor. Could you try this patch and watch for warnings?

Signature · 6 lines
blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.

---
include/asm-arm/pgalloc.h +1 -0
index 163b030..eeb973a 100644
@@ -94,6 +94,7 @@ static inline void pte_free_kernel(struct mm_struct *mm, pte_t *pte)
 
 static inline void pte_free(struct mm_struct *mm, pgtable_t pte)
 {
+	WARN_ON(!pte);
 	pgtable_page_dtor(pte);
 	__free_page(pte);
 }
author
#8 lore raw

Hello Martin,

Quoted 11 lines · Martin Schwidefsky
Quoted 10 lines · Uwe Kleine-König

I see the following:

	/ # for i in 1 2 3; do grep PageTables /proc/meminfo; done
	PageTables:          4 kB
	PageTables:   4294967292 kB
	PageTables:   4294967284 kB

and I bisected it down to 2f569af (CONFIG_HIGHPTE vs. sub-page page tables.) This still happens in 2.6.25-rc3.

Quoted 8 lines
Quoted 4 lines

I have not investigated further, so I cannot tell if it's only the output in meminfo that is broken.

This is on ARCH=arm, on a Digi cc9p9360 with ns9xxx_defconfig.

Hmm, not good. The number obviously went negative. There is an imbalance in the number of pgtable_page_ctor vs. pgtable_page_dtor. Could you try this patch and watch for warnings?

It doesn't trigger.

And your assumption that there is an imbalance between pgtable_page_ctor and pgtable_page_dtor is wrong, too. I added some debug output to both of the two functions and for one grep both are executed seven times.

Moreover a "grep PageTables /proc/meminfo" resulted in

	9x __dec_zone_state with item == NR_PAGETABLE
	7x __inc_zone_state with item == NR_PAGETABLE

All those 16 calles where done via __(inc|dec)_zone_page_state.

... some time later ...

I got it. This code is currently in free_pgd_slow():

        pte = pmd_page(*pmd);
        pmd_clear(pmd);
        dec_zone_page_state(virt_to_page((unsigned long *)pgd), NR_PAGETABLE);
        pte_lock_deinit(pte);
        pte_free(mm, pte);
        pmd_free(mm, pmd);

So because (since 2f569af) pte_free does dec_zone_page_state and pte_lock_deinit, these two happen twice here.

Removing the calls to dec_zone_page_state() and pte_lock_deinit() in free_pgd_slow() fixed it for me.

For a complete fix we might want to change the type of pte?

Best regards
Uwe

Signature · 3 lines
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
#9 lore raw

Hi Uwe,

Quoted 18 lines · Uwe Kleine-König

Moreover a "grep PageTables /proc/meminfo" resulted in

	9x __dec_zone_state with item == NR_PAGETABLE
	7x __inc_zone_state with item == NR_PAGETABLE

All those 16 calles where done via __(inc|dec)_zone_page_state.

... some time later ...

I got it. This code is currently in free_pgd_slow():

        pte = pmd_page(*pmd);
        pmd_clear(pmd);
        dec_zone_page_state(virt_to_page((unsigned long *)pgd), NR_PAGETABLE);
        pte_lock_deinit(pte);
        pte_free(mm, pte);
        pmd_free(mm, pmd);
Quoted 2 lines

So because (since 2f569af) pte_free does dec_zone_page_state and pte_lock_deinit, these two happen twice here.

Ah, good. I wonder if there are any other pte_lock_deinit hidden in the arch code... nope, arm is the only architecture that does it.

Quoted 2 lines

Removing the calls to dec_zone_page_state() and pte_lock_deinit() in free_pgd_slow() fixed it for me.

That is imho the best way to fix it.

Quoted 1 line

For a complete fix we might want to change the type of pte?

You mean instead of a "struct page *" use a pgtable_t ? Yes, that would be cleaner even if it is the same type.

Signature · 4 lines
blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.
#10 lore raw
Quoted 21 lines · Martin Schwidefsky

Hi Uwe,

Quoted 18 lines · Uwe Kleine-König

Moreover a "grep PageTables /proc/meminfo" resulted in

	9x __dec_zone_state with item == NR_PAGETABLE
	7x __inc_zone_state with item == NR_PAGETABLE

All those 16 calles where done via __(inc|dec)_zone_page_state.

... some time later ...

I got it. This code is currently in free_pgd_slow():

        pte = pmd_page(*pmd);
        pmd_clear(pmd);
        dec_zone_page_state(virt_to_page((unsigned long *)pgd), NR_PAGETABLE);
        pte_lock_deinit(pte);
        pte_free(mm, pte);
        pmd_free(mm, pmd);
Quoted 5 lines
Quoted 2 lines

So because (since 2f569af) pte_free does dec_zone_page_state and pte_lock_deinit, these two happen twice here.

Ah, good. I wonder if there are any other pte_lock_deinit hidden in the arch code... nope, arm is the only architecture that does it.

That's because we _may_ have a page table to map the vectors page at address 0 (which needs to exist all the time that the page tables are in use by the CPU.)

Quoted 4 lines
Quoted 2 lines

Removing the calls to dec_zone_page_state() and pte_lock_deinit() in free_pgd_slow() fixed it for me.

That is imho the best way to fix it.

... but at what cost?

Quoted 4 lines
Quoted 1 line

For a complete fix we might want to change the type of pte?

You mean instead of a "struct page *" use a pgtable_t ? Yes, that would be cleaner even if it is the same type.

Would someone mind investigating how the code ended up in this mess in the first place, so we can avoid this kind of thing in the future?

It looks like the pte_lock_deinit() appeared in
4c21e2f2441dc5fbb957b030333f5a3f2d02dea7.

The dec_zone_page_state() used to be dec_page_state(nr_page_table_pages); which then became a dec_zone_page_state() in df849a1529c106f7460e51479ca78fe07b07dc8c.

Both changes of those changes on their own look correct.

Ah, is it that someone skipped over ARM when they changed the page table freeing code? Yes - 2f569afd9ced9ebec9a6eb3dbf6f83429be0a7b4 has:

mm/memory.c +2 -4
index 153a54b..e5628a5 100644
@@ -134,11 +134,9 @@ void pmd_clear_bad(pmd_t *pmd)
  */
 static void free_pte_range(struct mmu_gather *tlb, pmd_t *pmd)
 {
-       struct page *page = pmd_page(*pmd);
+       pgtable_t token = pmd_pgtable(*pmd);
        pmd_clear(pmd);
-       pte_lock_deinit(page);
-       pte_free_tlb(tlb, page);
-       dec_zone_page_state(page, NR_PAGETABLE);
+       pte_free_tlb(tlb, token);
        tlb->mm->nr_ptes--;
 }

and no corresponding change to ARM. So, ARM needs to be fixed with this change before some other subtle breakage occurs.

Signature · 3 lines
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:
author
#11 lore raw

Hello Russell,

(I didn't add you to the recipients of that mail, because your mail had a Reply-To: header. So I assume that's what you want.)

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

Removing the calls to dec_zone_page_state() and pte_lock_deinit() in free_pgd_slow() fixed it for me.

That is imho the best way to fix it.

... but at what cost?

I don't see any costs. Can you please enlighten me?

Quoted 11 lines
Quoted 4 lines
Quoted 1 line

For a complete fix we might want to change the type of pte?

You mean instead of a "struct page *" use a pgtable_t ? Yes, that would be cleaner even if it is the same type.

Would someone mind investigating how the code ended up in this mess in the first place, so we can avoid this kind of thing in the future?

It looks like the pte_lock_deinit() appeared in
4c21e2f2441dc5fbb957b030333f5a3f2d02dea7.

Quoted 8 lines

The dec_zone_page_state() used to be dec_page_state(nr_page_table_pages); which then became a dec_zone_page_state() in df849a1529c106f7460e51479ca78fe07b07dc8c.

Both changes of those changes on their own look correct.

Ah, is it that someone skipped over ARM when they changed the page table freeing code? Yes - 2f569afd9ced9ebec9a6eb3dbf6f83429be0a7b4 has:

If you look at the subject line or the earlier mails in that thread, that is the exact commit I blamed, too.

So below comes a patch. In my eyes this has to go in before 2.6.25.

Best regards
Uwe

---->8----
Fix freeing of page tables for ARM in free_pgd_slow

Since 2f569af (CONFIG_HIGHPTE vs. sub-page page tables.) pte_free() calls pte_lock_deinit() and dec_zone_page_state(). So free_pgd_slow must not call the latter two when calling the first.

Signed-off-by Uwe Kleine-König <[email protected]>
Cc Russell King <[email protected]>
Cc Martin Schwidefsky <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Linus Torvalds <[email protected]>
---
 arch/arm/mm/pgd.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)
arch/arm/mm/pgd.c +2 -4
index 500c961..e0f19ab 100644
@@ -75,7 +75,7 @@ no_pgd:
 void free_pgd_slow(struct mm_struct *mm, pgd_t *pgd)
 {
 	pmd_t *pmd;
-	struct page *pte;
+	pgtable_t pte;
 
 	if (!pgd)
 		return;

@@ -90,10 +90,8 @@ void free_pgd_slow(struct mm_struct *mm, pgd_t *pgd)
 		goto free;
 	}
 
-	pte = pmd_page(*pmd);
+	pte = pmd_pgtable(*pmd);
 	pmd_clear(pmd);
-	dec_zone_page_state(virt_to_page((unsigned long *)pgd), NR_PAGETABLE);
-	pte_lock_deinit(pte);
 	pte_free(mm, pte);
 	pmd_free(mm, pmd);
 free:
Signature · 7 lines
1.5.4.3


-- 
Uwe Kleine-König, Software Engineer
Digi International GmbH Branch Breisach, Küferstrasse 8, 79206 Breisach, Germany
Tax: 315/5781/0242 / VAT: DE153662976 / Reg. Amtsgericht Dortmund HRB 13962
#12 lore raw
In reply to Russell King
Quoted 7 lines · Russell King
Quoted 4 lines
Quoted 1 line

For a complete fix we might want to change the type of pte?

You mean instead of a "struct page *" use a pgtable_t ? Yes, that would be cleaner even if it is the same type.

Would someone mind investigating how the code ended up in this mess in the first place, so we can avoid this kind of thing in the future?

One thing I have learned is that if I ever have to do a change over all architectures again I will split the patch into the common part and the arch parts and send it to the respective maintainers directly. Adding linux-arch seems NOT to be enough to get the attention of the arch maintainers (having the patch in -mm for 3 month is not doing the trick either).

Quoted 11 lines

It looks like the pte_lock_deinit() appeared in
4c21e2f2441dc5fbb957b030333f5a3f2d02dea7.

The dec_zone_page_state() used to be dec_page_state(nr_page_table_pages); which then became a dec_zone_page_state() in df849a1529c106f7460e51479ca78fe07b07dc8c.

Both changes of those changes on their own look correct.

Ah, is it that someone skipped over ARM when they changed the page table freeing code? Yes - 2f569afd9ced9ebec9a6eb3dbf6f83429be0a7b4 has:

Yes, sorry. I need one of these brown paper bags..

Signature · 4 lines
blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.