mm: fix PageUptodate memory ordering bug

PATCH In review Nick Piggin 9 participants 29 messages Dec 18, 2007 – Jan 7, 2008

Subsystems (6)

Memory management x86 MM core Generic include/asm header files +2 more subsystems

lore

Share & tools

lore

Nick Piggin

author

2007-12-18 01:26 UTC #0 lore raw

PATCH 1/1 mm: fix PageUptodate memory ordering bug

Hi,

Here is another version of this fix. It is now much simpler in that no filesystems have to be audited and changed to SetPageUptodate under page lock. It also only executes the smp_rmb() conditionally if the page is actually uptodate as suggested by Hugh. I guess this will be a win on most architectures with out of order loads, but I don't know... maybe the branch will be worse. On x86 it is a noop (except the compiler barrier).

---
After running SetPageUptodate, preceeding stores to the page contents to actually bring it uptodate may not be ordered with the store to set the page uptodate.

Therefore, another CPU which checks PageUptodate is true, then reads the page contents can get stale data.

Fix this by having an smp_wmb before SetPageUptodate, and smp_rmb after PageUptodate.

Many places that test PageUptodate, do so with the page locked, and this would be enough to ensure memory ordering in those places if SetPageUptodate were only called while the page is locked. Unfortunately that is not always the case for some filesystems, but it could be an idea for the future.

One thing I like about it is that it brings the handling of anonymous page uptodateness in line with that of file backed page management, by marking anon pages as uptodate when they _are_ uptodate, rather than when our implementation requires that they be marked as such. Doing allows us to get rid of the smp_wmb's in the page copying functions, which were especially added for anonymous pages for an analogous memory ordering problem, and are now handled with the same code as the PageUptodate memory ordering problem.

Introduce a SetNewPageUptodate for these anonymous pages: it contains non atomic bitops so as not to introduce too much overhead into these paths.

Signed-off-by Nick Piggin <[email protected]>

include/linux/highmem.h +0 -4

Index: linux-2.6/include/linux/highmem.h
===================================================================
@@ -68,8 +68,6 @@ static inline void clear_user_highpage(s
 	void *addr = kmap_atomic(page, KM_USER0);
 	clear_user_page(addr, vaddr, page);
 	kunmap_atomic(addr, KM_USER0);
-	/* Make sure this page is cleared on other CPU's too before using it */
-	smp_wmb();
 }
 
 #ifndef __HAVE_ARCH_ALLOC_ZEROED_USER_HIGHPAGE

@@ -160,8 +158,6 @@ static inline void copy_user_highpage(st
 	copy_user_page(vto, vfrom, vaddr, to);
 	kunmap_atomic(vfrom, KM_USER0);
 	kunmap_atomic(vto, KM_USER1);
-	/* Make sure this page is cleared on other CPU's too before using it */
-	smp_wmb();
 }
 
 #endif

include/linux/page-flags.h +39 -3

55 lines not on this page · raw message

mm/hugetlb.c +2 -0

Index: linux-2.6/mm/hugetlb.c
===================================================================
@@ -753,6 +753,7 @@ static int hugetlb_cow(struct mm_struct 
 
 	spin_unlock(&mm->page_table_lock);
 	copy_huge_page(new_page, old_page, address, vma);
+	SetNewPageUptodate(new_page);
 	spin_lock(&mm->page_table_lock);
 
 	ptep = huge_pte_offset(mm, address & HPAGE_MASK);

@@ -798,6 +799,7 @@ retry:
 			goto out;
 		}
 		clear_huge_page(page, address);
+		SetNewPageUptodate(page);
 
 		if (vma->vm_flags & VM_SHARED) {
 			int err;

mm/memory.c +5 -4

Index: linux-2.6/mm/memory.c
===================================================================
@@ -1516,10 +1516,8 @@ static inline void cow_user_page(struct 
 			memset(kaddr, 0, PAGE_SIZE);
 		kunmap_atomic(kaddr, KM_USER0);
 		flush_dcache_page(dst);
-		return;
-
-	}
-	copy_user_highpage(dst, src, va, vma);
+	} else
+		copy_user_highpage(dst, src, va, vma);
 }
 
 /*

@@ -1628,6 +1626,7 @@ gotten:
 	if (!new_page)
 		goto oom;
 	cow_user_page(new_page, old_page, address, vma);
+	SetNewPageUptodate(new_page);
 
 	/*
 	 * Re-check the pte - we dropped the lock

@@ -2160,6 +2159,7 @@ static int do_anonymous_page(struct mm_s
 	page = alloc_zeroed_user_highpage_movable(vma, address);
 	if (!page)
 		goto oom;
+	SetNewPageUptodate(page);
 
 	entry = mk_pte(page, vma->vm_page_prot);
 	entry = maybe_mkwrite(pte_mkdirty(entry), vma);

@@ -2260,6 +2260,7 @@ static int __do_fault(struct mm_struct *
 				goto out;
 			}
 			copy_user_highpage(page, vmf.page, address, vma);
+			SetNewPageUptodate(page);
 		} else {
 			/*
 			 * If the page will be shareable, see if the backing

mm/page_io.c +1 -1

Index: linux-2.6/mm/page_io.c
===================================================================
@@ -126,7 +126,7 @@ int swap_readpage(struct file *file, str
 	int ret = 0;
 
 	BUG_ON(!PageLocked(page));
-	ClearPageUptodate(page);
+	BUG_ON(PageUptodate(page));
 	bio = get_swap_bio(GFP_KERNEL, page_private(page), page,
 				end_swap_bio_read);
 	if (bio == NULL) {

mm/swap_state.c +1 -1

Index: linux-2.6/mm/swap_state.c
===================================================================
@@ -152,6 +152,7 @@ int add_to_swap(struct page * page, gfp_
 	int err;
 
 	BUG_ON(!PageLocked(page));
+	BUG_ON(!PageUptodate(page));
 
 	for (;;) {
 		entry = get_swap_page();

@@ -174,7 +175,6 @@ int add_to_swap(struct page * page, gfp_
 
 		switch (err) {
 		case 0:				/* Success */
-			SetPageUptodate(page);
 			SetPageDirty(page);
 			INC_CACHE_INFO(add_total);
 			return 1;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Andrew Morton

maintainer

2007-12-22 08:57 UTC #1 lore raw

Quoted 23 lines · On Tue, 18 Dec 2007 02:26:32 +0100 Nick Piggin <[email protected]> wrote:

After running SetPageUptodate, preceeding stores to the page contents to actually bring it uptodate may not be ordered with the store to set the page uptodate.
Therefore, another CPU which checks PageUptodate is true, then reads the page contents can get stale data.
Fix this by having an smp_wmb before SetPageUptodate, and smp_rmb after PageUptodate.
Many places that test PageUptodate, do so with the page locked, and this would be enough to ensure memory ordering in those places if SetPageUptodate were only called while the page is locked. Unfortunately that is not always the case for some filesystems, but it could be an idea for the future.
One thing I like about it is that it brings the handling of anonymous page uptodateness in line with that of file backed page management, by marking anon pages as uptodate when they _are_ uptodate, rather than when our implementation requires that they be marked as such. Doing allows us to get rid of the smp_wmb's in the page copying functions, which were especially added for anonymous pages for an analogous memory ordering problem, and are now handled with the same code as the PageUptodate memory ordering problem.

Quoted 3 lines

Introduce a SetNewPageUptodate for these anonymous pages: it contains non atomic bitops so as not to introduce too much overhead into these paths.

hrm.

include/linux/page-flags.h line 155

Quoted 4 lines

include/linux/page-flags.h +4 -0

+static inline void SetNewPageUptodate(struct page *page)
+{
+	smp_wmb();
+	__set_bit(PG_uptodate, &(page)->flags);

argh. Put the pin back in that thing before you hurt someone.

Sigh. I guess it's fairly clear but it could do with a big fat warning over it before you go and kill someone.

Because if this little hand grenade gets used in the wrong place, it will cause a horrid, horrid data-corrupting bug which might take us literally years to hunt down and fix.

include/linux/page-flags.h line 159

Quoted 5 lines

#ifdef CONFIG_S390
include/linux/page-flags.h +3 -0
+	page_clear_dirty(page);
+#endif
+}

For an overall 0.5% increase in the i386 size of several core mm files. If you don't blow us up on the spot, you'll slowly bleed us to death.

Can it be improved?

Hugh Dickins

2007-12-22 12:14 UTC #2 lore raw

Quoted 17 lines · Andrew Morton

Quoted 15 lines · On Tue, 18 Dec 2007 02:26:32 +0100 Nick Piggin <[email protected]> wrote:
After running SetPageUptodate, preceeding stores to the page contents to actually bring it uptodate may not be ordered with the store to set the page uptodate.
Therefore, another CPU which checks PageUptodate is true, then reads the page contents can get stale data.
Fix this by having an smp_wmb before SetPageUptodate, and smp_rmb after PageUptodate.
Many places that test PageUptodate, do so with the page locked, and this would be enough to ensure memory ordering in those places if SetPageUptodate were only called while the page is locked. Unfortunately that is not always the case for some filesystems, but it could be an idea for the future.

Quoted 4 lines

Quoted 4 lines
One thing I like about it is that it brings the handling of anonymous page uptodateness in line with that of file backed page management, by marking anon pages as uptodate when they _are_ uptodate, rather than when our implementation requires that they be marked as such.

Nick, you're welcome to make that a separate, less controversial patch, to send in ahead. Though I think the last time this came around, I hit one of your BUGs in testing shmem.c swapout or swapin or swapoff:
something missing there that I've lost the record of - please do try testing that, maybe it's already fixed this time around.

Quoted 21 lines

Quoted 8 lines
Doing allows us to get rid of the
smp_wmb's in the page copying functions, which were especially added for anonymous pages for an analogous memory ordering problem, and are now handled with the same code as the PageUptodate memory ordering problem.
Introduce a SetNewPageUptodate for these anonymous pages: it contains non atomic bitops so as not to introduce too much overhead into these paths.
hrm.
include/linux/page-flags.h line 155
Quoted 4 lines
include/linux/page-flags.h +4 -0
+static inline void SetNewPageUptodate(struct page *page)
+{
+	smp_wmb();
+	__set_bit(PG_uptodate, &(page)->flags);
argh. Put the pin back in that thing before you hurt someone.
Sigh. I guess it's fairly clear but it could do with a big fat warning over it before you go and kill someone.

Quoted 8 lines

Because if this little hand grenade gets used in the wrong place, it will cause a horrid, horrid data-corrupting bug which might take us literally years to hunt down and fix.
include/linux/page-flags.h line 159
Quoted 4 lines
#ifdef CONFIG_S390
include/linux/page-flags.h +3 -0
+	page_clear_dirty(page);
+#endif
+}

That's an odd little extract, since page_clear_dirty only does anything on s390.

Quoted 4 lines

For an overall 0.5% increase in the i386 size of several core mm files. If you don't blow us up on the spot, you'll slowly bleed us to death.
Can it be improved?

I do wish it could be.

I never find the time to give it the thought it needs; and any criticism I make is probably unjust, probably patiently answered by Nick on a previous round.

I'm never convinced that SetPageUptodate is the right place for
this: what's wrong with doing it in those page copying functions? Or flush_dcache_page? Don't we need different kinds of barrier according to how the data got into the page (by DMA or not)? Doesn't that enter territory discussed down the years between James Bottomley and Russell King? Worth CC'ing them the original?

Let me fall silent for a few days...

Hugh

Nick Piggin

author

2007-12-23 06:54 UTC #3 lore raw

Quoted 23 lines · Hugh Dickins

Quoted 17 lines · Andrew Morton
Quoted 15 lines · On Tue, 18 Dec 2007 02:26:32 +0100 Nick Piggin <[email protected]> wrote:
After running SetPageUptodate, preceeding stores to the page contents to actually bring it uptodate may not be ordered with the store to set the page uptodate.
Therefore, another CPU which checks PageUptodate is true, then reads the page contents can get stale data.
Fix this by having an smp_wmb before SetPageUptodate, and smp_rmb after PageUptodate.
Many places that test PageUptodate, do so with the page locked, and this would be enough to ensure memory ordering in those places if SetPageUptodate were only called while the page is locked. Unfortunately that is not always the case for some filesystems, but it could be an idea for the future.
Quoted 4 lines
Quoted 4 lines
One thing I like about it is that it brings the handling of anonymous page uptodateness in line with that of file backed page management, by marking anon pages as uptodate when they _are_ uptodate, rather than when our implementation requires that they be marked as such.

Quoted 5 lines

Nick, you're welcome to make that a separate, less controversial patch, to send in ahead. Though I think the last time this came around, I hit one of your BUGs in testing shmem.c swapout or swapin or swapoff:
something missing there that I've lost the record of - please do try testing that, maybe it's already fixed this time around.

I've given it some hours in your patented swapping kbuild-on-ext2-on-loop-on-tmpfs stress testing (including swapoff). Haven't seen a problem as yet (except the tmpfs swapin deadlock, which I've been patching out).

But if you see anything, please let me know...

Quoted 7 lines

Quoted 4 lines
include/linux/page-flags.h line 159
Quoted 4 lines
#ifdef CONFIG_S390
include/linux/page-flags.h +3 -0
+	page_clear_dirty(page);
+#endif
+}
That's an odd little extract, since page_clear_dirty only does anything on s390.

Ah yeah, we could just get rid of the ifdef. Although I don't mind it too much, as it kind of helps the reader match the other ifdef there...

Quoted 7 lines

Quoted 4 lines
For an overall 0.5% increase in the i386 size of several core mm files. If you don't blow us up on the spot, you'll slowly bleed us to death.
Can it be improved?
I do wish it could be.

Quoted 7 lines

I never find the time to give it the thought it needs; and any criticism I make is probably unjust, probably patiently answered by Nick on a previous round.
I'm never convinced that SetPageUptodate is the right place for
this: what's wrong with doing it in those page copying functions? Or flush_dcache_page?

There are various places we _could_ do it, but I think PG_uptodate macros are logically the best, without being too intrusive.

Let me explain. Normally I think the convention would be to open-code the barriers in the callees (ie. between memset(); SetPageUptodate();, and if (PageUptodate()) { read from page }).

However I think that would require going through quite a bit of code (including filesystems) to audit. So I think having them in these macros is pretty reasonable, and amounts to less thinking required by others.

Why don't I like doing it in page copying functions? Just because there are more and more varied uses. I can't think of any reasons to rather do it in the page copying functions, and some reasons against.

flush_dcache_page? Well this bug really is a problem ordering stores to the page with store to page flags against loads from the same; nothing to do with cache aliasing. So putting the smp_wmb in flush_dcache_page leaves you without a natural complement to put the smp_rmb. Although it could be done, I think it makes it more tangled than having the ordering done in the macros. We also only need to order the *initial* stores which bring the page uptodate, rather than for each store, in the case of flush_dcache_page.

Quoted 2 lines

Don't we need different kinds of barrier
according to how the data got into the page (by DMA or not)?

I had thought of that (my previous patch had an XXX: help...) for this very issue. Without actually knowing what the underlying architecture does, I "concluded" that it should be done somewhere down at the block layer. I think it would be silly for the block layer to signal completion if the results are still incoherent with the CPU cache... but if the experts have a different opinion, then this needs to be solved with another call anyway (not in the page uptodate macros and it's not exactly a memory ordering issue). eg. direct IO reads would have the same DMA cache synchronisation before it completes to userspace, and this is completely independent of PG_uptodate...

Quoted 2 lines

Doesn't that enter territory discussed down the years between James Bottomley and Russell King? Worth CC'ing them the original?

... but since you bring this up again, I think that would be worthwhile. In the interest of maintaining this thread I'll just link the original:

http://marc.info/?l=linux-mm&m=119794127303483&w=2

The question is this:

Must read from net/disk/etc into page P.
Device DMAs into P, signals completion

CPU0: handles completion, store to ram to mark P uptodate

CPU0/1: load from ram sees P uptodate, load from P must only see uptodate data

Are we guaranteed to get uptodate data from above the block layer, or do we need to do anything special?

Nick Piggin

author

2007-12-23 05:57 UTC #4 lore raw

In reply to Andrew Morton

Quoted 38 lines · Andrew Morton

Quoted 23 lines · On Tue, 18 Dec 2007 02:26:32 +0100 Nick Piggin <[email protected]> wrote:
After running SetPageUptodate, preceeding stores to the page contents to actually bring it uptodate may not be ordered with the store to set the page uptodate.
Therefore, another CPU which checks PageUptodate is true, then reads the page contents can get stale data.
Fix this by having an smp_wmb before SetPageUptodate, and smp_rmb after PageUptodate.
Many places that test PageUptodate, do so with the page locked, and this would be enough to ensure memory ordering in those places if SetPageUptodate were only called while the page is locked. Unfortunately that is not always the case for some filesystems, but it could be an idea for the future.
One thing I like about it is that it brings the handling of anonymous page uptodateness in line with that of file backed page management, by marking anon pages as uptodate when they _are_ uptodate, rather than when our implementation requires that they be marked as such. Doing allows us to get rid of the smp_wmb's in the page copying functions, which were especially added for anonymous pages for an analogous memory ordering problem, and are now handled with the same code as the PageUptodate memory ordering problem.
Quoted 3 lines
Introduce a SetNewPageUptodate for these anonymous pages: it contains non atomic bitops so as not to introduce too much overhead into these paths.
hrm.
include/linux/page-flags.h line 155
Quoted 4 lines
include/linux/page-flags.h +4 -0
+static inline void SetNewPageUptodate(struct page *page)
+{
+	smp_wmb();
+	__set_bit(PG_uptodate, &(page)->flags);
argh. Put the pin back in that thing before you hurt someone.

Quoted 2 lines

Sigh. I guess it's fairly clear but it could do with a big fat warning over it before you go and kill someone.

Hmm, perhaps it should use the more conventional __SetPageUptodate. I had named it SetNewPageUptodate in an earlier version of the ptach which was slightly different.

Quoted 3 lines

Because if this little hand grenade gets used in the wrong place, it will cause a horrid, horrid data-corrupting bug which might take us literally years to hunt down and fix.

Yeah, like the other non-atomic bitops. I guess we just have to make sure they're used correctly... they really make a big difference to performance.

I guess the memory ordering bug would take years to hunt down and fix too, OTOH I guess it would probably be the powerpc guys, rather than you or I, that have to debug it ;)

Quoted 11 lines

include/linux/page-flags.h line 159
Quoted 5 lines
#ifdef CONFIG_S390
include/linux/page-flags.h +3 -0
+	page_clear_dirty(page);
+#endif
+}
For an overall 0.5% increase in the i386 size of several core mm files. If you don't blow us up on the spot, you'll slowly bleed us to death.
Can it be improved?

At first glance you'd think that, loads being in order on i386, it should be a noop, but we actually still require a barrier to be technically correct (even on i386). Which increases the size of some otherwise unchanged files.

Adding a few SetNewPageUptodates adds the rest, I guess. The alternative would be to have more open coded smp_wmb()s around. I like this way much better.

Given the amount of crap that's "pending", I'd be surprised if I was the one who bleeds us to death with bugfixes ;) But if you'd rather see some speedups, I could certainly rustle something up.

Anyway, thanks for picking it up. That's already tripled the amount of feedback it hsa got ;)

Andrew Morton

maintainer

2007-12-23 06:32 UTC #5 lore raw

Quoted 7 lines · On Sun, 23 Dec 2007 06:57:30 +0100 Nick Piggin <[email protected]> wrote:

Quoted 7 lines
include/linux/page-flags.h line 155
Quoted 4 lines
include/linux/page-flags.h +4 -0
+static inline void SetNewPageUptodate(struct page *page)
+{
+	smp_wmb();
+	__set_bit(PG_uptodate, &(page)->flags);
argh. Put the pin back in that thing before you hurt someone.

Quoted 6 lines

Quoted 2 lines
Sigh. I guess it's fairly clear but it could do with a big fat warning over it before you go and kill someone.
Hmm, perhaps it should use the more conventional __SetPageUptodate. I had named it SetNewPageUptodate in an earlier version of the ptach which was slightly different.

It's a death trap. __GFP_UPTODATE might be safer, dunno.

Quoted 5 lines

...
Quoted 3 lines
For an overall 0.5% increase in the i386 size of several core mm files. If you don't blow us up on the spot, you'll slowly bleed us to death.

Quoted 8 lines

Quoted 1 line
Can it be improved?
At first glance you'd think that, loads being in order on i386, it should be a noop, but we actually still require a barrier to be technically correct (even on i386). Which increases the size of some otherwise unchanged files.
Adding a few SetNewPageUptodates adds the rest, I guess. The alternative would be to have more open coded smp_wmb()s around. I like this way much better.

That's just speculation. Please find out why such a small patch caused such a large code size increase and see if it can be fixed.

Quoted 2 lines

Given the amount of crap that's "pending", I'd be surprised if I was the one who bleeds us to death with bugfixes ;)

Please consider spending some time reviewing other people's crap.

Quoted 5 lines

But if you'd rather see some speedups,
I could certainly rustle something up.
Anyway, thanks for picking it up. That's already tripled the amount of feedback it hsa got ;)

Rather than expecting others to review yours (not singling you out - you just had good timing).

I didn't actually include the patch in -mm due to Hugh's comments.

Nick Piggin

author

2007-12-23 07:15 UTC #6 lore raw

Quoted 9 lines · Andrew Morton

Quoted 7 lines · On Sun, 23 Dec 2007 06:57:30 +0100 Nick Piggin <[email protected]> wrote:
Quoted 7 lines
include/linux/page-flags.h line 155
Quoted 4 lines
include/linux/page-flags.h +4 -0
+static inline void SetNewPageUptodate(struct page *page)
+{
+	smp_wmb();
+	__set_bit(PG_uptodate, &(page)->flags);
argh. Put the pin back in that thing before you hurt someone.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
Sigh. I guess it's fairly clear but it could do with a big fat warning over it before you go and kill someone.
Hmm, perhaps it should use the more conventional __SetPageUptodate. I had named it SetNewPageUptodate in an earlier version of the ptach which was slightly different.
It's a death trap. __GFP_UPTODATE might be safer, dunno.

It's like natural selection for VM developers that don't know their page lifetime rules ;)

GFP_UPTODATE can't be done, because we're not only talking about zeroed pages here.

Quoted 9 lines

Quoted 9 lines
Quoted 4 lines
For an overall 0.5% increase in the i386 size of several core mm files. If you don't blow us up on the spot, you'll slowly bleed us to death.
Can it be improved?
At first glance you'd think that, loads being in order on i386, it should be a noop, but we actually still require a barrier to be technically correct (even on i386). Which increases the size of some otherwise unchanged files.

Quoted 5 lines

Quoted 2 lines
Adding a few SetNewPageUptodates adds the rest, I guess. The alternative would be to have more open coded smp_wmb()s around. I like this way much better.
That's just speculation. Please find out why such a small patch caused such a large code size increase and see if it can be fixed.

It's not actually increasing size by that much here... hmm, do you have CONFIG_X86_PPRO_FENCE defined, by any chance?

It looks like this gets defined by default for i386, and also probably for distro configs. Linus? This is a fairly heavy hammer for such an unlikely bug on such a small number of systems (that admittedly doesn't even fix the bug in all cases anyway). It's not only heavy for my proposed patch, but it also halves the speed of spinlocks. Can we have some special config option for this instead?

Quoted 4 lines

Quoted 2 lines
Given the amount of crap that's "pending", I'd be surprised if I was the one who bleeds us to death with bugfixes ;)
Please consider spending some time reviewing other people's crap.

Fixing existing bugs is important too. Granted this doesn't seem to be a pressing issue, but it's a bug. But I do review too.

Quoted 8 lines

Quoted 5 lines
But if you'd rather see some speedups,
I could certainly rustle something up.
Anyway, thanks for picking it up. That's already tripled the amount of feedback it hsa got ;)
Rather than expecting others to review yours (not singling you out - you just had good timing).

By crap, I wasn't referring to bugfixes. But if you have any bugfixes in my area in -mm that need review, I can take a look, sure. Actually I always try to read mm/ bugfixes that go by, though I often don't have anything extra to say.

Quoted 1 line

I didn't actually include the patch in -mm due to Hugh's comments.

How about my counter-comments? :)

Andrew Morton

maintainer

2007-12-23 07:29 UTC #7 lore raw

Quoted 5 lines · On Sun, 23 Dec 2007 08:15:29 +0100 Nick Piggin <[email protected]> wrote:

Quoted 2 lines
That's just speculation. Please find out why such a small patch caused such a large code size increase and see if it can be fixed.
It's not actually increasing size by that much here... hmm, do you have CONFIG_X86_PPRO_FENCE defined, by any chance?

I expect it was just allmodconfig, so: yes

It's a quite repeatable experiment though.

Quoted 5 lines

It looks like this gets defined by default for i386, and also probably for distro configs. Linus? This is a fairly heavy hammer for such an unlikely bug on such a small number of systems (that admittedly doesn't even fix the bug in all cases anyway). It's not only heavy for my proposed patch, but it also halves the speed of spinlocks. Can we have some special config option for this instead?

Sounds worthwhile, if we can't do it via altinstructions.

Nick Piggin

author

2007-12-23 09:14 UTC #8 lore raw

Quoted 11 lines · Andrew Morton

Quoted 5 lines · On Sun, 23 Dec 2007 08:15:29 +0100 Nick Piggin <[email protected]> wrote:
Quoted 2 lines
That's just speculation. Please find out why such a small patch caused such a large code size increase and see if it can be fixed.
It's not actually increasing size by that much here... hmm, do you have CONFIG_X86_PPRO_FENCE defined, by any chance?
I expect it was just allmodconfig, so: yes
It's a quite repeatable experiment though.

Yep. I only saw half the text size blot that you did, but it was still quite a bit.

i386 allmodconf: size mm/built-in.o
text data bss dec hex text ratio
vanilla: 163082 20372 40120 223574 36956 100.00%
bugfix : 163509 20372 40120 224001 36b01 0.26%
noppro : 162191 20372 40120 222683 365db - 0.55%
both : 162267 20372 40120 222759 36627 - 0.50% (+0.05% vs noppro)

So with the ppro memory ordering bug out of the way, the PG_uptodate fix only adds 76 bytes of text.

Quoted 7 lines

Quoted 5 lines
It looks like this gets defined by default for i386, and also probably for distro configs. Linus? This is a fairly heavy hammer for such an unlikely bug on such a small number of systems (that admittedly doesn't even fix the bug in all cases anyway). It's not only heavy for my proposed patch, but it also halves the speed of spinlocks. Can we have some special config option for this instead?
Sounds worthwhile, if we can't do it via altinstructions.

Altinstructions means we still have code bloat, and sometimes extra branches etc (an extra 900 bytes of icache in mm/ alone, even before my fix). I'll let Linus or one of the x86 guys weigh in, though. It's a really sad cost for distro kernels to carry.

---

arch/x86/Kconfig.cpu +11 -2

Index: linux-2.6/arch/x86/Kconfig.cpu
===================================================================
@@ -322,9 +322,18 @@ config X86_XADD
 	default y
 
 config X86_PPRO_FENCE
-	bool
+	bool "PentiumPro memory ordering errata workaround"
 	depends on M686 || M586MMX || M586TSC || M586 || M486 || M386 || MGEODEGX1
-	default y
+	default n
+	help
+	  Old PentiumPro multiprocessor systems had errata that could cause memory
+	  operations to violate the x86 ordering standard in rare cases. Enabling this
+	  option will attempt to work around some (but not all) occurances of
+	  this problem, at the cost of much heavier spinlock and memory barrier
+	  operations.
+
+	  If unsure, say n here. Even distro kernels should think twice before enabling
+	  this: there are few systems, and an unlikely bug.
 
 config X86_F00F_BUG
 	bool

Andrew Morton

maintainer

2007-12-23 09:28 UTC #9 lore raw

Quoted 6 lines · On Sun, 23 Dec 2007 10:14:05 +0100 Nick Piggin <[email protected]> wrote:

config X86_PPRO_FENCE

arch/x86/Kconfig.cpu +1 -2

-	bool
+	bool "PentiumPro memory ordering errata workaround"
 	depends on M686 || M586MMX || M586TSC || M586 || M486 || M386 || MGEODEGX1
-	default y

arch/x86/Kconfig.cpu config X86_XADD · line 336

Quoted 10 lines

arch/x86/Kconfig.cpu +10 -0

+	default n
+	help
+	  Old PentiumPro multiprocessor systems had errata that could cause memory
+	  operations to violate the x86 ordering standard in rare cases. Enabling this
+	  option will attempt to work around some (but not all) occurances of
+	  this problem, at the cost of much heavier spinlock and memory barrier
+	  operations.
+
+	  If unsure, say n here. Even distro kernels should think twice before enabling
+	  this: there are few systems, and an unlikely bug.

I think if we're going to do this then we should add a runtime check for the offending CPU then do panic("your kernel config ain't right").

Andi Kleen

2007-12-23 16:02 UTC #10 lore raw

Attacking that has been on my todo list forever.

I think the right way would be to define a separate Config symbol outside the normal CPU list for this case

CONFIG_X86_BROKEN_PPRO

or similar with Kconfig describing that it will have a large .text overhead. Then distributions can chose to not set it.

Quoted 2 lines

I think if we're going to do this then we should add a runtime check for the offending CPU then do panic("your kernel config ain't right").

Panic is not needed, it is sufficient to force the system to one CPU (= set cpu_possible_map to { 1 } ) and a warning to suggest enabling CONFOG_X86_BROKEN_PPRO

-Andi

Ingo Molnar

2007-12-30 16:33 UTC #11 lore raw

In reply to Nick Piggin

Quoted 6 lines · Nick Piggin

Quoted 1 line
Sounds worthwhile, if we can't do it via altinstructions.
Altinstructions means we still have code bloat, and sometimes extra branches etc (an extra 900 bytes of icache in mm/ alone, even before my fix). I'll let Linus or one of the x86 guys weigh in, though. It's a really sad cost for distro kernels to carry.

hm, we should at minimum display a warning if the workaround is not enabled and such a kernel is booted on a true PPro that is affected by this.

Ingo

Nick Piggin

author

2008-01-01 23:26 UTC #12 lore raw

Quoted 5 lines · Ingo Molnar

Quoted 2 lines · Nick Piggin
Quoted 1 line
Sounds worthwhile, if we can't do it via altinstructions.

Quoted 8 lines

Quoted 4 lines
Altinstructions means we still have code bloat, and sometimes extra branches etc (an extra 900 bytes of icache in mm/ alone, even before my fix). I'll let Linus or one of the x86 guys weigh in, though. It's a really sad cost for distro kernels to carry.
hm, we should at minimum display a warning if the workaround is not enabled and such a kernel is booted on a true PPro that is affected by this.

The patch does have the warning:
printk(KERN_INFO "Pentium Pro with Errata#66, #92 detected. Limiting maxcpus to 1.

         Enable CONFIG_X86_BROKEN_PPRO_SMP to run with multiple CPUs\n");

Andi Kleen

2008-01-02 21:01 UTC #13 lore raw

Quoted 11 lines · On Wednesday 02 January 2008 00:26:34 Nick Piggin wrote:

Quoted 10 lines · Ingo Molnar
Quoted 6 lines · Nick Piggin
Quoted 1 line
Sounds worthwhile, if we can't do it via altinstructions.
Altinstructions means we still have code bloat, and sometimes extra branches etc (an extra 900 bytes of icache in mm/ alone, even before my fix). I'll let Linus or one of the x86 guys weigh in, though. It's a really sad cost for distro kernels to carry.

Quoted 7 lines

Quoted 3 lines
hm, we should at minimum display a warning if the workaround is not enabled and such a kernel is booted on a true PPro that is affected by this.
The patch does have the warning:
printk(KERN_INFO "Pentium Pro with Errata#66, #92 detected. Limiting maxcpus to 1.
         Enable CONFIG_X86_BROKEN_PPRO_SMP to run with multiple CPUs\n");

Haven't seen the full patch, but the printk suggest you're changing the max_cpus variable. That is not 100% safe because user space could hot plug CPUs later using sysfs. The only safe way would be to limit cpu_possible_map

-Andi

Nick Piggin

author

2008-01-03 03:32 UTC #14 lore raw

Quoted 16 lines · Andi Kleen

Quoted 15 lines · On Wednesday 02 January 2008 00:26:34 Nick Piggin wrote:
Quoted 13 lines · Ingo Molnar
Quoted 6 lines · Nick Piggin
Quoted 1 line
Sounds worthwhile, if we can't do it via altinstructions.
Altinstructions means we still have code bloat, and sometimes extra branches etc (an extra 900 bytes of icache in mm/ alone, even before my fix). I'll let Linus or one of the x86 guys weigh in, though. It's a really sad cost for distro kernels to carry.
hm, we should at minimum display a warning if the workaround is not enabled and such a kernel is booted on a true PPro that is affected by this.

Quoted 7 lines

Quoted 3 lines
The patch does have the warning:
printk(KERN_INFO "Pentium Pro with Errata#66, #92 detected. Limiting maxcpus to 1.
         Enable CONFIG_X86_BROKEN_PPRO_SMP to run with multiple CPUs\n");
Haven't seen the full patch, but the printk suggest you're changing the max_cpus variable. That is not 100% safe because user space could hot plug CPUs later using sysfs. The only safe way would be to limit cpu_possible_map

Hmm, but I did want to allow it to be overridden via maxcpus= command line (or hotplug I guess, I hadn't thought of the hotplug case but it allows runtime override).

Andi Kleen

2008-01-03 13:08 UTC #15 lore raw

Quoted 3 lines

Hmm, but I did want to allow it to be overridden via maxcpus= command line (or hotplug I guess, I hadn't thought of the hotplug case but it allows runtime override).

In theory some user space could be set up to always boot with maxcpus=1 and then only hotplug CPUs later (e.g. might make sense to get faster initial booting on systems with a lot of CPUs)

It would be better to use a separate option for such workarounds.

-Andi

Linus Torvalds

2007-12-23 17:22 UTC #16 lore raw

In reply to Nick Piggin

Quoted 9 lines · Nick Piggin

It's not actually increasing size by that much here... hmm, do you have CONFIG_X86_PPRO_FENCE defined, by any chance?
It looks like this gets defined by default for i386, and also probably for distro configs. Linus? This is a fairly heavy hammer for such an unlikely bug on such a small number of systems (that admittedly doesn't even fix the bug in all cases anyway). It's not only heavy for my proposed patch, but it also halves the speed of spinlocks. Can we have some special config option for this instead?

A special config option isn't great, since vendors would probably then enable it for those old P6's.

But maybe an "alternative()" thing that depends on a CPU capability?

Of course, it definitely *is* true that the number of CPU's that have that bug _and_ are actually used in SMP environments is probably vanishingly small. So maybe even vendors don't really care any more, and we could make the PPRO_FENCE thing a thing of the past.

There's actually a few different PPro errata. There's #51, which is an IO ordering thing, and can happen on UP too. There's #66, which breaks CPU ordering, and is SMP-only (and which is probably at least *mostly* fixed by PPRO_FENCE), and there is #92 which can cause cache incoherency and where PPRO_FENCE *may* indirectly help.

We could decide to just ignore all of them, or perhaps ignore all but #51. I think Alan still has an old four-way PPro hidden away somewhere, but he's probably one of the few people who could even *test* this thing.

Or we could make PPRO_FENCE just be something that you have to enable by hand, rather than enabling it automatically for a set of CPU's. That way Alan can add it to his config, and everybody else can clear it if they decide that they don't have one of the old 200MHz PPro's any more..

Linus

Nick Piggin

author

2007-12-23 21:35 UTC #17 lore raw

Quoted 29 lines · Linus Torvalds

Quoted 9 lines · Nick Piggin
It's not actually increasing size by that much here... hmm, do you have CONFIG_X86_PPRO_FENCE defined, by any chance?
It looks like this gets defined by default for i386, and also probably for distro configs. Linus? This is a fairly heavy hammer for such an unlikely bug on such a small number of systems (that admittedly doesn't even fix the bug in all cases anyway). It's not only heavy for my proposed patch, but it also halves the speed of spinlocks. Can we have some special config option for this instead?
A special config option isn't great, since vendors would probably then enable it for those old P6's.
But maybe an "alternative()" thing that depends on a CPU capability?
Of course, it definitely *is* true that the number of CPU's that have that bug _and_ are actually used in SMP environments is probably vanishingly small. So maybe even vendors don't really care any more, and we could make the PPRO_FENCE thing a thing of the past.
There's actually a few different PPro errata. There's #51, which is an IO ordering thing, and can happen on UP too. There's #66, which breaks CPU ordering, and is SMP-only (and which is probably at least *mostly* fixed by PPRO_FENCE), and there is #92 which can cause cache incoherency and where PPRO_FENCE *may* indirectly help.

Quoted 8 lines

We could decide to just ignore all of them, or perhaps ignore all but #51. I think Alan still has an old four-way PPro hidden away somewhere, but he's probably one of the few people who could even *test* this thing.
Or we could make PPRO_FENCE just be something that you have to enable by hand, rather than enabling it automatically for a set of CPU's. That way Alan can add it to his config, and everybody else can clear it if they decide that they don't have one of the old 200MHz PPro's any more..

Yes I would prefer it to be something you can turn off completely. I think having alternative() would be slightly overkill, and it still costs a fair bit of icache. We could take Andi's good idea of printing a warning and enabling just a single CPU, which would probably be reasonable defence against #66 and #91.

Nick Piggin

author

2007-12-23 22:41 UTC #18 lore raw

In reply to Linus Torvalds

Quoted 6 lines · Linus Torvalds

There's actually a few different PPro errata. There's #51, which is an IO ordering thing, and can happen on UP too. There's #66, which breaks CPU ordering, and is SMP-only (and which is probably at least *mostly* fixed by PPRO_FENCE), and there is #92 which can cause cache incoherency and where PPRO_FENCE *may* indirectly help.

BTW. #66 seems to be an issue where a CPU may see the wrong results from a RAW, if another CPU has written to the cacheline in the meantime. #92 is a data loss bug (although it said they couldn't reproduce it on real hardware).

It says they're actually not a problem if semaphore operations are used to protect the data. However a) it is becoming increasingly common that we don't do that (eg. with lockless operations), and b) I don't know how the case of false sharing in a cacheline can be safe.

Anyway, I think it is very rare, even on those two systems (one being in Alan's basement) that run Linux... The number of cycles everybody else loses in spin_unlock combined far outweighs the number of cycles these additional CPUs are going to sit idle :)

Quoted 3 lines

We could decide to just ignore all of them, or perhaps ignore all but #51. I think Alan still has an old four-way PPro hidden away somewhere, but he's probably one of the few people who could even *test* this thing.

This patch uses both your and Andi's ideas... Untested though.

X86_PPRO_FENCE means we might encounter these systems, so workaround #51, and disable multiple cpus... unless X86_PPRO_FENCE_SMP, which includes the workarounds for #66 and #92.

---

arch/x86/Kconfig.cpu +1 -1

Index: linux-2.6/arch/x86/Kconfig.cpu
===================================================================
@@ -321,7 +321,7 @@ config X86_XADD
 	depends on X86_32 && !M386
 	default y
 
-config X86_PPRO_FENCE
+config X86_BROKEN_PPRO
 	bool
 	depends on M686 || M586MMX || M586TSC || M586 || M486 || M386 || MGEODEGX1
 	default y

include/asm-x86/io_32.h +1 -1

Index: linux-2.6/include/asm-x86/io_32.h
===================================================================
@@ -235,7 +235,7 @@ memcpy_toio(volatile void __iomem *dst, 
  *	2. Accidentally out of order processors (PPro errata #51)
  */
  
-#if defined(CONFIG_X86_OOSTORE) || defined(CONFIG_X86_PPRO_FENCE)
+#if defined(CONFIG_X86_OOSTORE) || defined(CONFIG_X86_BROKEN_PPRO)
 
 static inline void flush_write_buffers(void)
 {

include/asm-x86/spinlock_32.h +1 -1

Index: linux-2.6/include/asm-x86/spinlock_32.h
===================================================================
@@ -101,7 +101,7 @@ static inline int __raw_spin_trylock(raw
  * (PPro errata 66, 92)
  */
 
-#if !defined(CONFIG_X86_OOSTORE) && !defined(CONFIG_X86_PPRO_FENCE)
+#if !defined(CONFIG_X86_OOSTORE) && !defined(CONFIG_X86_BROKEN_PPRO_SMP)
 
 static inline void __raw_spin_unlock(raw_spinlock_t *lock)
 {

arch/x86/kernel/cpu/intel.c +19 -0

Index: linux-2.6/arch/x86/kernel/cpu/intel.c
===================================================================
@@ -108,6 +108,23 @@ static void __cpuinit trap_init_f00f_bug
 }
 #endif
 
+/*
+ * Errata #66, #92
+ */
+static void __cpuinit ppro_smp_fence_bug(void)
+{
+#if defined(CONFIG_SMP) && defined(CONFIG_X86_BROKEN_PPRO) && !defined(CONFIG_X86_BROKEN_PPRO_SMP)
+	extern unsigned int maxcpus;
+
+	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
+	    boot_cpu_data.x86 == 6 &&
+	    boot_cpu_data.x86_model == 1) {
+		printk(KERN_INFO "Pentium Pro with Errata#66, #92 detected. Limiting maxcpus to 1. Enable CONFIG_X86_BROKEN_PPRO_SMP to run with multiple CPUs\n");
+		maxcpus = 1;
+	}
+#endif
+}
+
 static void __cpuinit init_intel(struct cpuinfo_x86 *c)
 {
 	unsigned int l2 = 0;

@@ -132,6 +149,8 @@ static void __cpuinit init_intel(struct 
 	}
 #endif
 
+	ppro_smp_fence_bug();
+
 	select_idle_routine(c);
 	l2 = init_intel_cacheinfo(c);
 	if (c->cpuid_level > 9 ) {

include/asm-x86/system_32.h +1 -1

Index: linux-2.6/include/asm-x86/system_32.h
===================================================================
@@ -279,7 +279,7 @@ static inline unsigned long get_limit(un
 
 #ifdef CONFIG_SMP
 #define smp_mb()	mb()
-#ifdef CONFIG_X86_PPRO_FENCE
+#ifdef CONFIG_X86_BROKEN_PPRO_SMP
 # define smp_rmb()	rmb()
 #else
 # define smp_rmb()	barrier()

arch/x86/Kconfig +16 -0

Index: linux-2.6/arch/x86/Kconfig
===================================================================
@@ -378,6 +378,22 @@ config ES7000_CLUSTERED_APIC
 
 source "arch/x86/Kconfig.cpu"
 
+config X86_BROKEN_PPRO_SMP
+	bool "PentiumPro memory ordering errata workaround"
+	depends on X86_BROKEN_PPRO && SMP
+	default n
+	help
+	  Old PentiumPro multiprocessor systems had errata that could cause memory
+	  operations to violate the x86 ordering standard in rare cases. Enabling this
+	  option will attempt to work around some (but not all) occurances of these
+	  problems, at the cost of much heavier spinlock and memory barrier operations.
+
+	  If you say N here, these systems will be detected and limited to a single CPU
+	  at boot time.
+
+	  If unsure, say N here. Even distro kernels should think twice before enabling
+	  this: there are few systems, and an unlikely bug.
+
 config HPET_TIMER
 	bool
 	prompt "HPET Timer Support" if X86_32

Alan Cox

2008-01-01 23:41 UTC #19 lore raw

In reply to Linus Torvalds

Quoted 13 lines · On Sun, 23 Dec 2007 09:22:17 -0800 (PST) Linus Torvalds…

Quoted 9 lines · Nick Piggin
It's not actually increasing size by that much here... hmm, do you have CONFIG_X86_PPRO_FENCE defined, by any chance?
It looks like this gets defined by default for i386, and also probably for distro configs. Linus? This is a fairly heavy hammer for such an unlikely bug on such a small number of systems (that admittedly doesn't even fix the bug in all cases anyway). It's not only heavy for my proposed patch, but it also halves the speed of spinlocks. Can we have some special config option for this instead?

Quoted 8 lines

A special config option isn't great, since vendors would probably then enable it for those old P6's.
But maybe an "alternative()" thing that depends on a CPU capability? Of course, it definitely *is* true that the number of CPU's that have that bug _and_ are actually used in SMP environments is probably vanishingly small. So maybe even vendors don't really care any more, and we could make the PPRO_FENCE thing a thing of the past.

If the PPro fencing isn't built for SMP kernels for set for CPU of Pentium II or greater then nobody is going to care. The only reasons to build distro support for older processors is

- VIA C3/C5 to work around the gcc 686 cmov bug
- Geode (embedded and OLPC)

neither of which are exactly multiprocessor, and the VIA stuff can be handled by beating up the gcc options. I also doubt PPro will be terribly high on anyones enterprise product line list for the next generation of enterprise distributions.

Alan

Nick Piggin

author

2008-01-02 11:02 UTC #20 lore raw

PATCH 1/1 i386: avoid expensive ppro ordering workaround for default 686 kernels

Quoted 25 lines · Alan Cox

Quoted 13 lines · On Sun, 23 Dec 2007 09:22:17 -0800 (PST) Linus Torvalds…
Quoted 9 lines · Nick Piggin
It's not actually increasing size by that much here... hmm, do you have CONFIG_X86_PPRO_FENCE defined, by any chance?
It looks like this gets defined by default for i386, and also probably for distro configs. Linus? This is a fairly heavy hammer for such an unlikely bug on such a small number of systems (that admittedly doesn't even fix the bug in all cases anyway). It's not only heavy for my proposed patch, but it also halves the speed of spinlocks. Can we have some special config option for this instead?
Quoted 8 lines
A special config option isn't great, since vendors would probably then enable it for those old P6's.
But maybe an "alternative()" thing that depends on a CPU capability? Of course, it definitely *is* true that the number of CPU's that have that bug _and_ are actually used in SMP environments is probably vanishingly small. So maybe even vendors don't really care any more, and we could make the PPRO_FENCE thing a thing of the past.

Quoted 3 lines

If the PPro fencing isn't built for SMP kernels for set for CPU of Pentium II or greater then nobody is going to care. The only reasons to build distro support for older processors is

SLES10 seems to build for M586 by default, and I think it would be very reasonable to expect M686 builds to be common in future. Even for SMP kernels -- maybe we'd see fewer special case UP kernels with better kernel support and more common multicore chips.

What's more, it might give me a chance of getting this uninitialised-data -leaking, data-corrupting bugfix past Andrew's allyesconfig "sanity" check ;)

Quoted 7 lines

- VIA C3/C5 to work around the gcc 686 cmov bug
- Geode (embedded and OLPC)
neither of which are exactly multiprocessor, and the VIA stuff can be handled by beating up the gcc options. I also doubt PPro will be terribly high on anyones enterprise product line list for the next generation of enterprise distributions.

And that's also exactly why this patch is also pretty reasonable. Ingo, please apply? (have changed the info message to a warning).

--
The selection of many CPU architecture families causes pentium pro memory ordering errata workarounds to be enabled. This causes memory barriers and spinlocks to become much more expensive, just to provide a few hacks for a very rare (nowadays) class of system.

Take a different approach: after this patch, we just disable all but one CPU on those systems, and print a warning. Also printed is a suggestion for a new CONFIG option that can be enabled for the previous behaviour.

This is a big hammer for those few smp ppro systems, but it removes the big hammer that was there for everyone else. It is also arguably the most correct way to work around the problems -- from the description of at least one errata, loss of cache coherency, could cause problems when particular classes of lockless data accesses are used (even with the existing workarounds in place).

Signed-off-by Nick Piggin <[email protected]>

---

arch/x86/Kconfig.cpu +1 -1

Index: linux-2.6/arch/x86/Kconfig.cpu
===================================================================
@@ -321,7 +321,7 @@ config X86_XADD
 	depends on X86_32 && !M386
 	default y
 
-config X86_PPRO_FENCE
+config X86_BROKEN_PPRO
 	bool
 	depends on M686 || M586MMX || M586TSC || M586 || M486 || M386 || MGEODEGX1
 	default y

include/asm-x86/io_32.h +1 -1

Index: linux-2.6/include/asm-x86/io_32.h
===================================================================
@@ -235,7 +235,7 @@ memcpy_toio(volatile void __iomem *dst, 
  *	2. Accidentally out of order processors (PPro errata #51)
  */
  
-#if defined(CONFIG_X86_OOSTORE) || defined(CONFIG_X86_PPRO_FENCE)
+#if defined(CONFIG_X86_OOSTORE) || defined(CONFIG_X86_BROKEN_PPRO)
 
 static inline void flush_write_buffers(void)
 {

include/asm-x86/spinlock_32.h +1 -1

Index: linux-2.6/include/asm-x86/spinlock_32.h
===================================================================
@@ -101,7 +101,7 @@ static inline int __raw_spin_trylock(raw
  * (PPro errata 66, 92)
  */
 
-#if !defined(CONFIG_X86_OOSTORE) && !defined(CONFIG_X86_PPRO_FENCE)
+#if !defined(CONFIG_X86_OOSTORE) && !defined(CONFIG_X86_BROKEN_PPRO_SMP)
 
 static inline void __raw_spin_unlock(raw_spinlock_t *lock)
 {

arch/x86/kernel/cpu/intel.c +19 -0

Index: linux-2.6/arch/x86/kernel/cpu/intel.c
===================================================================
@@ -108,6 +108,23 @@ static void __cpuinit trap_init_f00f_bug
 }
 #endif
 
+/*
+ * Errata #66, #92
+ */
+static void __cpuinit ppro_smp_fence_bug(void)
+{
+#if defined(CONFIG_SMP) && defined(CONFIG_X86_BROKEN_PPRO) && !defined(CONFIG_X86_BROKEN_PPRO_SMP)
+	extern unsigned int maxcpus;
+
+	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
+	    boot_cpu_data.x86 == 6 &&
+	    boot_cpu_data.x86_model == 1) {
+		printk(KERN_WARNING "WARNING: Pentium Pro with Errata#66, #92 detected. Limiting maxcpus to 1. Enable CONFIG_X86_BROKEN_PPRO_SMP to run with multiple CPUs\n");
+		maxcpus = 1;
+	}
+#endif
+}
+
 static void __cpuinit init_intel(struct cpuinfo_x86 *c)
 {
 	unsigned int l2 = 0;

@@ -132,6 +149,8 @@ static void __cpuinit init_intel(struct 
 	}
 #endif
 
+	ppro_smp_fence_bug();
+
 	select_idle_routine(c);
 	l2 = init_intel_cacheinfo(c);
 	if (c->cpuid_level > 9 ) {

include/asm-x86/system_32.h +1 -1

Index: linux-2.6/include/asm-x86/system_32.h
===================================================================
@@ -279,7 +279,7 @@ static inline unsigned long get_limit(un
 
 #ifdef CONFIG_SMP
 #define smp_mb()	mb()
-#ifdef CONFIG_X86_PPRO_FENCE
+#ifdef CONFIG_X86_BROKEN_PPRO_SMP
 # define smp_rmb()	rmb()
 #else
 # define smp_rmb()	barrier()

arch/x86/Kconfig +16 -0

Index: linux-2.6/arch/x86/Kconfig
===================================================================
@@ -378,6 +378,22 @@ config ES7000_CLUSTERED_APIC
 
 source "arch/x86/Kconfig.cpu"
 
+config X86_BROKEN_PPRO_SMP
+	bool "PentiumPro memory ordering errata workaround"
+	depends on X86_BROKEN_PPRO && SMP
+	default n
+	help
+	  Old PentiumPro multiprocessor systems had errata that could cause memory
+	  operations to violate the x86 ordering standard in rare cases. Enabling this
+	  option will attempt to work around some (but not all) occurances of these
+	  problems, at the cost of much heavier spinlock and memory barrier operations.
+
+	  If you say N here, these systems will be detected and limited to a single CPU
+	  at boot time.
+
+	  If unsure, say N here. Even distro kernels should think twice before enabling
+	  this: there are few systems, and an unlikely bug.
+
 config HPET_TIMER
 	bool
 	prompt "HPET Timer Support" if X86_32

Alan Cox

2008-01-02 13:44 UTC #21 lore raw

Re: [patch] i386: avoid expensive ppro ordering workaround for default 686 kernels

Quoted 3 lines

Take a different approach: after this patch, we just disable all but one CPU on those systems, and print a warning. Also printed is a suggestion for a new CONFIG option that can be enabled for the previous behaviour.

How does that help. The processor isn't the only bus master.

Maybe this works as a SuSE specific convenience solution aligned to your particular build pattern but it isn't the right solution for upstream IMHO.

We should either

- re-order the assumed processor generations supported to put VIA C3/C5 above Preventium Pro
- fix the gcc or gcc settings not to generate invalid cmov instructions on 686. cmov is slower on all the modern processors anyway.

And you change the assumption that 586 < 686 < PPro < PII < PIII ...

to 586 < 686 < PPro < C3 < PII < ...

then selecting VIA C3 support will get you a kernel with the properties all the distribution vendors want for their higher end mainstream kernel - "runs on modern systems".

Alan

Nick Piggin

author

2008-01-03 04:17 UTC #22 lore raw

Re: [patch] i386: avoid expensive ppro ordering workaround for default 686 kernels

Quoted 5 lines · Alan Cox

Quoted 3 lines
Take a different approach: after this patch, we just disable all but one CPU on those systems, and print a warning. Also printed is a suggestion for a new CONFIG option that can be enabled for the previous behaviour.
How does that help. The processor isn't the only bus master.

Hmm, I don't understand what you mean. Obviously other busmasters aren't participating in any locking or smp_*mb() ordering protocols.

The non-smp_-prefixed barriers are retained.

Quoted 3 lines

Maybe this works as a SuSE specific convenience solution aligned to your particular build pattern but it isn't the right solution for upstream IMHO.

Actually it is nothing to do with SUSE but I was using SLES as a counterexample when you said nobody would care about M686 with SMP builds. The reason for the patch is because I noticed the suboptimal configuration (because Andrew flagged my patch).

Quoted 4 lines

We should either
- re-order the assumed processor generations supported to put VIA C3/C5 above Preventium Pro

Adrian Bunk's patch to make each CPU type explicitly selectable IMO is the best way to do this.

Quoted 2 lines

- fix the gcc or gcc settings not to generate invalid cmov instructions on 686. cmov is slower on all the modern processors anyway.

Not for unpredictable branches, but I agree gcc seems to use it too often (in my version, it seems to even use it for likely/unlikely branches :( ).

Quoted 7 lines

And you change the assumption that 586 < 686 < PPro < PII < PIII ...
to 586 < 686 < PPro < C3 < PII < ...
then selecting VIA C3 support will get you a kernel with the properties all the distribution vendors want for their higher end mainstream kernel - "runs on modern systems".

I don't agree. If we support those options, we should support them properly. And if you build an SMP kernel for a 586 for example, you should not get lumped with those pentiumpro workarounds.

Alan Cox

2008-01-03 14:23 UTC #23 lore raw

Re: [patch] i386: avoid expensive ppro ordering workaround for default 686 kernels

Quoted 2 lines

Hmm, I don't understand what you mean. Obviously other busmasters aren't participating in any locking or smp_*mb() ordering protocols.

The unlock paths are visible to busmasters

write to DMA buffer
unlock
can turn into

unlock
write to DMA buffer

Whether that actually matters in any code we have I don't know.

Quoted 5 lines

Quoted 2 lines
- re-order the assumed processor generations supported to put VIA C3/C5 above Preventium Pro
Adrian Bunk's patch to make each CPU type explicitly selectable IMO is the best way to do this.

If you get to pick the combination you want then yes.

Quoted 9 lines

Quoted 5 lines
to 586 < 686 < PPro < C3 < PII < ...
then selecting VIA C3 support will get you a kernel with the properties all the distribution vendors want for their higher end mainstream kernel - "runs on modern systems".
I don't agree. If we support those options, we should support them properly. And if you build an SMP kernel for a 586 for example, you should not get lumped with those pentiumpro workarounds.

The cost on a 586 SMP box (ie pentium) is basically nil. All cross CPU transactions are hideously slow anyway locked or not. On a PII or later I agree entirely you don't want them by default.

Alan

Benjamin Herrenschmidt

2008-01-03 20:20 UTC #24 lore raw

Re: [patch] i386: avoid expensive ppro ordering workaround for default 686 kernels

Quoted 5 lines · Alan Cox

Quoted 2 lines
Hmm, I don't understand what you mean. Obviously other busmasters aren't participating in any locking or smp_*mb() ordering protocols.
The unlock paths are visible to busmasters

Quoted 8 lines

write to DMA buffer
unlock
can turn into
unlock
write to DMA buffer
Whether that actually matters in any code we have I don't know.

It matters with writes to MMIO at least, I don't know if your PPro bug can cause that to be re-ordered tho.

Ben.

Alan Cox

2008-01-03 22:23 UTC #25 lore raw

Re: [patch] i386: avoid expensive ppro ordering workaround for default 686 kernels

On Fri, 04 Jan 2008 07:20:43 +1100

Quoted 4 lines

Quoted 1 line
Whether that actually matters in any code we have I don't know.
It matters with writes to MMIO at least, I don't know if your PPro bug can cause that to be re-ordered tho.

In certain cases yes - the 3Dfx Voodoo cards were particularly good at triggering it as they map the command registers cleverly to get bursts.

Nick Piggin

author

2008-01-03 23:10 UTC #26 lore raw

Re: [patch] i386: avoid expensive ppro ordering workaround for default 686 kernels

In reply to Alan Cox

Quoted 5 lines · Alan Cox

Quoted 2 lines
Hmm, I don't understand what you mean. Obviously other busmasters aren't participating in any locking or smp_*mb() ordering protocols.
The unlock paths are visible to busmasters

Quoted 8 lines

write to DMA buffer
unlock
can turn into
unlock
write to DMA buffer
Whether that actually matters in any code we have I don't know.

These actually require wmb() anyway, because we only guarantee that spinlocks contain operations to cacheable memory.

Observe that when !CONFIG_SMP, smp_*mb and spin_*lock are basically noops, even when the existing pentiumpro workaround is in place.

Though wmb() will actually provide the correct ordering.

Quoted 14 lines

Quoted 5 lines
Quoted 2 lines
- re-order the assumed processor generations supported to put VIA C3/C5 above Preventium Pro
Adrian Bunk's patch to make each CPU type explicitly selectable IMO is the best way to do this.
If you get to pick the combination you want then yes.
Quoted 6 lines
Quoted 5 lines
to 586 < 686 < PPro < C3 < PII < ...
then selecting VIA C3 support will get you a kernel with the properties all the distribution vendors want for their higher end mainstream kernel - "runs on modern systems".

Quoted 6 lines

Quoted 3 lines
I don't agree. If we support those options, we should support them properly. And if you build an SMP kernel for a 586 for example, you should not get lumped with those pentiumpro workarounds.
The cost on a 586 SMP box (ie pentium) is basically nil. All cross CPU transactions are hideously slow anyway locked or not.

That's wrong. Firstly, spinlocks are very often retaken by the same CPU, and they are also often released while the lock word is still in the cache of the acquiring CPU. Secondly, the extra code and lock ops in smp_* barriers is fairly expensive.

On 586 SMP systems, didn't lock ops actually go out on the bus, and hence are much more expensive than they are today (although today they are still one or two orders of magnitude more expensive than regular memory ops).

Quoted 2 lines

On a PII or later I
agree entirely you don't want them by default.

And if you build a kernel which is to support 686, PII, and later, you don't want them by default either, most probably.

Alan Cox

2008-01-04 16:27 UTC #27 lore raw

Re: [patch] i386: avoid expensive ppro ordering workaround for default 686 kernels

Quoted 4 lines

Quoted 2 lines
The cost on a 586 SMP box (ie pentium) is basically nil. All cross CPU transactions are hideously slow anyway locked or not.
That's wrong. Firstly, spinlocks are very often retaken by the same CPU, and they

Go time it. It certainly used to be the case.

Quoted 3 lines

On 586 SMP systems, didn't lock ops actually go out on the bus, and hence are much more expensive than they are today (although today they are still one or two orders of magnitude more expensive than regular memory ops).

All cross CPU cache stuff goes that way, not that may P5 SMP boxes are running today except in Mr Bottomley's residence.

Quoted 2 lines

And if you build a kernel which is to support 686, PII, and later, you don't want them by default either, most probably.

I want a kernel for VIA C3 or later which happens to include K6 (still common), PII+ and maybe not PPro.

So your patch should go in the bitbucket by the sound of it and Adrian's get merged.

Nick Piggin

author

2008-01-07 00:12 UTC #28 lore raw

Re: [patch] i386: avoid expensive ppro ordering workaround for default 686 kernels

Quoted 6 lines · Alan Cox

Quoted 4 lines
Quoted 2 lines
The cost on a 586 SMP box (ie pentium) is basically nil. All cross CPU transactions are hideously slow anyway locked or not.
That's wrong. Firstly, spinlocks are very often retaken by the same CPU, and they
Go time it. It certainly used to be the case.

I don't understand what your point is. You bring up socket-to-socket transfers, but we are talking about the cost of locked ops (which may or may not involve a cacheline transfer).

For example, a spin lock + unlock (in cache) is around 40 cycles on my Core2. A ppro style unlock makes it 70.

And it is quite common with a lot of locks for the cacheline *not* to be bouncing (eg. scheduler runqueue locks).

Quoted 4 lines

Quoted 3 lines
On 586 SMP systems, didn't lock ops actually go out on the bus, and hence are much more expensive than they are today (although today they are still one or two orders of magnitude more expensive than regular memory ops).

Quoted 8 lines

All cross CPU cache stuff goes that way, not that may P5 SMP boxes are running today except in Mr Bottomley's residence.
Quoted 2 lines
And if you build a kernel which is to support 686, PII, and later, you don't want them by default either, most probably.
I want a kernel for VIA C3 or later which happens to include K6 (still common), PII+ and maybe not PPro.

I want a kernel for all those and also ppros, but I'm not willing to hammer everyone else to work around some rare errata. There are probably quite fewer SMP ppros than UP ones too.

You're just skirting the whole issue by saying you don't want to support ppro at all. The issue is having a kernel that supports a wide range of CPUs including ppro. *You* may not want that. Others may.

Quoted 2 lines

So your patch should go in the bitbucket by the sound of it and Adrian's get merged.

Adrian's patch IMO should get merged, sure (although last time it came up, it got rejected for some maybe not completely valid reasons). But Adrian's patch does not attack the problem which mine does. If you select PPro + PII + PIII + etc..., then you will still have SMP ppro errata workarounds compiled in. You need another option specifically asking whether you want them or not. Ie. my patch.

What is your exact objection to making this configurable?