Remove broken prefetching in free_one_pgd()

PATCH In review Luck, Tony 5 participants 10 messages Oct 24, 2003 – Oct 30, 2003

Subsystems (2)

Memory management MM core

lore

Share & tools

lore

Luck, Tony

author

2003-10-24 20:56 UTC #0 lore raw

PATCH 1/1 Remove broken prefetching in free_one_pgd()

This patch was accepted into 2.5.55, attributed to "davej@uk".

This code will prefetch from beyond the end of the page table being cleared ... which is clearly a bad thing if the page table in question is allocated from the last page of memory (or precedes a hole on a discontig mem system).

-Tony Luck

mm/memory.c +1 -3

diff -ru linux-2.4.23-pre8/mm/memory.c fix/mm/memory.c
@@ -120,10 +120,8 @@
 	}
 	pmd = pmd_offset(dir, 0);
 	pgd_clear(dir);
-	for (j = 0; j < PTRS_PER_PMD ; j++) {
-		prefetchw(pmd+j+(PREFETCH_STRIDE/16));
+	for (j = 0; j < PTRS_PER_PMD ; j++)
 		free_one_pmd(pmd+j);
-	}
 	pmd_free(pmd);
 }

David Mosberger

2003-10-24 22:01 UTC #1 lore raw

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Tony

Tony> This patch was accepted into 2.5.55, attributed to "davej@uk". Tony> This code will prefetch from beyond the end of the page table Tony> being cleared ... which is clearly a bad thing if the page Tony> table in question is allocated from the last page of memory Tony> (or precedes a hole on a discontig mem system).

Different arches behave differently, though. In the case of ia64, it'a always safe to prefetch (even with lfetch.fault).

	--david

Luck, Tony

author

2003-10-24 22:16 UTC #2 lore raw

In reply to Luck, Tony

Quoted 8 lines

Tony> This patch was accepted into 2.5.55, attributed to "davej@uk". Tony> This code will prefetch from beyond the end of the page table Tony> being cleared ... which is clearly a bad thing if the page Tony> table in question is allocated from the last page of memory Tony> (or precedes a hole on a discontig mem system).
Different arches behave differently, though. In the case of ia64, it'a always safe to prefetch (even with lfetch.fault).

Not quite always ... this was how I found the efi trim.bottom bug, since Linux had allocated a pgd at 0xa00000-16k, and the lfetch that reached out beyond the end of the page to the uncacheable address 0xa00000 took an MCA.

A pgd in the last page of a granule that is followed by an uncacheable address would do the same with lfetch.fault, wouldn't it?

-Tony

David Mosberger

2003-10-24 22:21 UTC #3 lore raw

In reply to Luck, Tony

Quoted 1 line

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Tony

Quoted 2 lines

Quoted 2 lines
Different arches behave differently, though. In the case of ia64, it'a always safe to prefetch (even with lfetch.fault).

Tony> Not quite always ... this was how I found the efi trim.bottom Tony> bug, since Linux had allocated a pgd at 0xa00000-16k, and the Tony> lfetch that reached out beyond the end of the page to the Tony> uncacheable address 0xa00000 took an MCA.

But don't confuse cause and effect! The MCA was caused by a bad TLB entry. The lfetch only triggered the latent bug (as might have a instruction-prefetch).

Tony> A pgd in the last page of a granule that is followed by an uncacheable Tony> address would do the same with lfetch.fault, wouldn't it?

No, lfetch to uncacheable translations have no effect.

	--david

Pavel Machek

2003-10-25 20:10 UTC #4 lore raw

In reply to Luck, Tony

Hi!

Quoted 1 line

This patch was accepted into 2.5.55, attributed to "davej@uk".

Dave Jones?n

Quoted 4 lines

This code will prefetch from beyond the end of the page table being cleared ... which is clearly a bad thing if the page table in question is allocated from the last page of memory (or precedes a hole on a discontig mem system).

Prefetching random addresses should be safe... Well for 2.4 we probably want to play it safe and kill it, but I guess quite a few pieces rely on prefretch(NULL) doing nothing...

Quoted 14 lines

diff -ru linux-2.4.23-pre8/mm/memory.c fix/mm/memory.c

mm/memory.c +1 -3

@@ -120,10 +120,8 @@
 	}
 	pmd = pmd_offset(dir, 0);
 	pgd_clear(dir);
-	for (j = 0; j < PTRS_PER_PMD ; j++) {
-		prefetchw(pmd+j+(PREFETCH_STRIDE/16));
+	for (j = 0; j < PTRS_PER_PMD ; j++)
 		free_one_pmd(pmd+j);
-	}
 	pmd_free(pmd);
 }

Signature · 2 lines

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Dave Jones

2003-10-26 14:51 UTC #5 lore raw

In reply to Luck, Tony

Quoted 2 lines · Pavel Machek

Quoted 1 line
This patch was accepted into 2.5.55, attributed to "davej@uk".
Dave Jones?n

Yep.

Quoted 5 lines

Quoted 4 lines
This code will prefetch from beyond the end of the page table being cleared ... which is clearly a bad thing if the page table in question is allocated from the last page of memory (or precedes a hole on a discontig mem system).
Prefetching random addresses should be safe...

It isn't on some CPUs. Early athlons go bang when you prefetch past the end of RAM into unmapped memory for eg.

Dave

Signature · 1 line

 Dave Jones     http://www.codemonkey.org.uk

Pavel Machek

2003-10-27 09:52 UTC #6 lore raw

In reply to Luck, Tony

Hi!

Quoted 8 lines

Quoted 5 lines
Quoted 4 lines
This code will prefetch from beyond the end of the page table being cleared ... which is clearly a bad thing if the page table in question is allocated from the last page of memory (or precedes a hole on a discontig mem system).
Prefetching random addresses should be safe...
It isn't on some CPUs. Early athlons go bang when you prefetch past the end of RAM into unmapped memory for eg.

Well, broken HW :-(, but andi's patch should fix up for that.

Pavel

Signature · 2 lines

When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Dave Jones

2003-10-27 11:58 UTC #7 lore raw

In reply to Luck, Tony

Quoted 4 lines · Pavel Machek

Quoted 2 lines
It isn't on some CPUs. Early athlons go bang when you prefetch past the end of RAM into unmapped memory for eg.
Well, broken HW :-(

There's a lot of it out there. If we didn't try to work around some of it, we'd end up running on less < 10% of machines out there 8-)

Quoted 1 line

but andi's patch should fix up for that.

Different bug.

Dave

Signature · 1 line

 Dave Jones     http://www.codemonkey.org.uk

Bjorn Helgaas

2003-10-30 20:57 UTC #8 lore raw

In reply to Luck, Tony

Quoted 4 lines · David Mosberger

Quoted 3 lines
Quoted 3 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line · Tony
Different arches behave differently, though. In the case of ia64, it'a always safe to prefetch (even with lfetch.fault).

Quoted 8 lines

Tony> Not quite always ... this was how I found the efi trim.bottom Tony> bug, since Linux had allocated a pgd at 0xa00000-16k, and the Tony> lfetch that reached out beyond the end of the page to the Tony> uncacheable address 0xa00000 took an MCA.
But don't confuse cause and effect! The MCA was caused by a bad TLB entry. The lfetch only triggered the latent bug (as might have a instruction-prefetch).

I'm assuming that the EFI memory map trim fixes prevent the bad TLB entry, and hence, the prefetching patch is not required by ia64 in 2.4. Tony, let me know if otherwise.

Bjorn

Luck, Tony

author

2003-10-30 21:36 UTC #9 lore raw

In reply to Luck, Tony

Quoted 14 lines · David Mosberger

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
On Fri, 24 Oct 2003 15:16:59 -0700, "Luck, Tony"
Quoted 1 line · <[email protected]> said:
Quoted 1 line
Quoted 1 line
Different arches behave differently, though. In the
case of ia64,
Quoted 3 lines
Quoted 1 line
Quoted 1 line
it'a always safe to prefetch (even with lfetch.fault).
Tony> Not quite always ... this was how I found the efi
trim.bottom
Quoted 1 line
Tony> bug, since Linux had allocated a pgd at
0xa00000-16k, and the
Quoted 3 lines
Tony> lfetch that reached out beyond the end of the page to the Tony> uncacheable address 0xa00000 took an MCA.

Quoted 7 lines

Quoted 3 lines
But don't confuse cause and effect! The MCA was caused by a bad TLB entry. The lfetch only triggered the latent bug (as might have a instruction-prefetch).
I'm assuming that the EFI memory map trim fixes prevent the bad TLB entry, and hence, the prefetching patch is not required by ia64 in 2.4. Tony, let me know if otherwise.

If EFI trim is doing its job (and the current version now seems to be handling all cases correctly), then you should no longer be able to have a TLB entry erroneously marking an uncacheable area of memory for cacheable access ... so you can keep the prefetch for ia64 (David pointed out that dropping this prefetch has a severe negative impact on lmbench fork+execve test).

-Tony