[permalink] [raw]

Subject: [PATCH] fix x86_64-mm-cpa-cache-flush.patch in 2.6.22-rc4-mm2

* Andi Kleen ([email protected]) wrote:
> On Wednesday 20 June 2007 18:46, Mathieu Desnoyers wrote:
> > * Andi Kleen ([email protected]) wrote:
> > > On Tuesday 19 June 2007 22:01:36 Mathieu Desnoyers wrote:
> > > > Looking more closely into the code to find the cause of the
> > > > change_page_addr()/global_flush_tlb() inconsistency, I see where the
> > > > problem could be:
> > >
> > > Yes it's a known problem. I have a hack queued for .22 and there
> > > are proposed patches for .23 too.
> > >
> > > ftp://ftp.firstfloor.org/pub/ak/x86_64/late-merge/patches/cpa-flush
> > >
> > > -ANdi
> >
> > Hi Andi,
> >
> > Although I cannot find it at the specified URL, I suspect it is already
> > in Andrew's tree, in 2.6.22-rc4-mm2, under the name
>
> Try again
>
> > "x86_64-mm-cpa-cache-flush.patch"
>
> No, that's a different patch with also at least one known bug.
>
> -Andi

I just fixed x86_64 and i386, using a high order bit of private as a
flag "page needs deferred flush". It works well on i386, not tested on
x86_64.

x86_64 mm CPA cache flush fix for i386 and x86_64

Andi's patch introduced a hang for i386 machines when write protecting pages.

1st fix : use the appropritate checks in global flush tlb.
2nd fix : the hang was caused by multiple list_add of the same
kpte_page. Use a high order bit to keep track of which kpte_pages are
currently in the list and waiting for deferred flush.

This patch applies on top of the x86_64-mm-cpa-cache-flush.patch in the -mm
tree (2.6.22-rc4-mm2).

(note: the revert-x86_64-mm-cpa-cache-flush.patch must be discarded from the -mm
tree)

Signed-off-by: Mathieu Desnoyers <[email protected]>
---
arch/i386/mm/pageattr.c | 24 +++++++++++++++++++-----
arch/x86_64/mm/pageattr.c | 16 ++++++++++++----
include/asm-i386/cacheflush.h | 11 +++++++++++
include/asm-x86_64/cacheflush.h | 11 +++++++++++
4 files changed, 53 insertions(+), 9 deletions(-)

Index: linux-2.6-lttng/arch/i386/mm/pageattr.c
===================================================================
--- linux-2.6-lttng.orig/arch/i386/mm/pageattr.c 2007-06-20 12:51:10.000000000 -0400
+++ linux-2.6-lttng/arch/i386/mm/pageattr.c 2007-06-20 15:28:56.000000000 -0400
@@ -53,6 +53,9 @@
/*
* page_private is used to track the number of entries in
* the page table page that have non standard attributes.
+ * We use the highest bit to tell is the page needs to be flushed,
+ * therefore page_private_cpa_count() must be used to read the count.
+ * Count increment and decrement never overflow on the highest bit.
*/
SetPagePrivate(base);
page_private(base) = 0;
@@ -160,7 +163,7 @@
page_private(kpte_page)++;
} else if (!pte_huge(*kpte)) {
set_pte_atomic(kpte, mk_pte(page, PAGE_KERNEL));
- BUG_ON(page_private(kpte_page) == 0);
+ BUG_ON(page_private_cpa_count(kpte_page) == 0);
page_private(kpte_page)--;
} else
BUG();
@@ -170,10 +173,12 @@
* time (not via split_large_page) and in turn we must not
* replace it with a largepage.
*/
-
- list_add(&kpte_page->lru, &df_list);
+ if (!(page_private(kpte_page) & CPA_FLUSH)) {
+ page_private(kpte_page) |= CPA_FLUSH;
+ list_add(&kpte_page->lru, &df_list);
+ }
if (!PageReserved(kpte_page)) {
- if (cpu_has_pse && (page_private(kpte_page) == 0)) {
+ if (cpu_has_pse && (page_private_cpa_count(kpte_page) == 0)) {
paravirt_release_pt(page_to_pfn(kpte_page));
revert_page(kpte_page, address);
}
@@ -228,9 +233,13 @@
if (!cpu_has_clflush)
flush_map(NULL);
list_for_each_entry_safe(pg, next, &l, lru) {
+ list_del(&pg->lru);
+ page_private(pg) &= ~CPA_FLUSH;
if (cpu_has_clflush)
flush_map(page_address(pg));
- if (page_private(pg) != 0)
+
+ if (PageReserved(pg) || !cpu_has_pse
+ || page_private_cpa_count(pg) != 0)
continue;
ClearPagePrivate(pg);
__free_page(pg);
@@ -252,6 +261,11 @@
change_page_attr(page, numpages, enable ? PAGE_KERNEL : __pgprot(0));
/* we should perform an IPI and flush all tlbs,
* but that can deadlock->flush only current cpu.
+ *
+ * FIXME : this is utterly buggy; it does not clean the df_list
+ * populated by change_page_attr and could cause a double addition to
+ * this list. With what exactly would the IPI deadlock ?
+ * Mathieu Desnoyers
*/
__flush_tlb_all();
}
Index: linux-2.6-lttng/include/asm-i386/cacheflush.h
===================================================================
--- linux-2.6-lttng.orig/include/asm-i386/cacheflush.h 2007-06-20 14:53:39.000000000 -0400
+++ linux-2.6-lttng/include/asm-i386/cacheflush.h 2007-06-20 15:23:07.000000000 -0400
@@ -4,6 +4,17 @@
/* Keep includes the same across arches. */
#include <linux/mm.h>

+/* Use the highest bit of the page's private field to flag the kpte page as
+ * needing a flush. The lower bits are used as a counter of the number of ptes
+ * with special flags, within the page, which will never use the highest bit.
+ * pte_t being 8 bytes in size,
+ * 4096/sizeof(pte_t) = 512, which holds in 9 bits.
+ * For Large pages:
+ * 4MB/sizeof(pte_t) = 524288, which holds in 19 bits.
+ */
+#define CPA_FLUSH (1UL<<31)
+#define page_private_cpa_count(page) (page_private(page) & (~CPA_FLUSH))
+
/* Caches aren't brain-dead on the intel. */
#define flush_cache_all() do { } while (0)
#define flush_cache_mm(mm) do { } while (0)
Index: linux-2.6-lttng/include/asm-x86_64/cacheflush.h
===================================================================
--- linux-2.6-lttng.orig/include/asm-x86_64/cacheflush.h 2007-06-20 14:55:23.000000000 -0400
+++ linux-2.6-lttng/include/asm-x86_64/cacheflush.h 2007-06-20 15:22:49.000000000 -0400
@@ -4,6 +4,17 @@
/* Keep includes the same across arches. */
#include <linux/mm.h>

+/* Use the highest bit of the page's private field to flag the kpte page as
+ * needing a flush. The lower bits are used as a counter of the number of ptes
+ * with special flags, within the page, which will never use the highest bit.
+ * pte_t being 8 bytes in size,
+ * 4096/sizeof(pte_t) = 512, which holds in 9 bits.
+ * For Large pages:
+ * 4MB/sizeof(pte_t) = 524288, which holds in 19 bits.
+ */
+#define CPA_FLUSH (1UL<<63)
+#define page_private_cpa_count(page) (page_private(page) & (~CPA_FLUSH))
+
/* Caches aren't brain-dead on the intel. */
#define flush_cache_all() do { } while (0)
#define flush_cache_mm(mm) do { } while (0)
Index: linux-2.6-lttng/arch/x86_64/mm/pageattr.c
===================================================================
--- linux-2.6-lttng.orig/arch/x86_64/mm/pageattr.c 2007-06-20 15:00:04.000000000 -0400
+++ linux-2.6-lttng/arch/x86_64/mm/pageattr.c 2007-06-20 15:24:35.000000000 -0400
@@ -47,6 +47,9 @@
/*
* page_private is used to track the number of entries in
* the page table page have non standard attributes.
+ * We use the highest bit to tell is the page needs to be flushed,
+ * therefore page_private_cpa_count() must be used to read the count.
+ * Count increment and decrement never overflow on the highest bit.
*/
SetPagePrivate(base);
page_private(base) = 0;
@@ -79,6 +82,7 @@
asm volatile("wbinvd" ::: "memory");
list_for_each_entry(pg, l, lru) {
void *adr = page_address(pg);
+ page_private(pg) &= ~CPA_FLUSH;
if (cpu_has_clflush)
cache_flush_page(adr);
}
@@ -94,7 +98,10 @@

static inline void save_page(struct page *fpage)
{
- list_add(&fpage->lru, &deferred_pages);
+ if (!(page_private(fpage) & CPA_FLUSH)) {
+ page_private(kpte_page) |= CPA_FLUSH;
+ list_add(&fpage->lru, &deferred_pages);
+ }
}

/*
@@ -150,7 +157,7 @@
page_private(kpte_page)++;
} else if (!pte_huge(*kpte)) {
set_pte(kpte, pfn_pte(pfn, ref_prot));
- BUG_ON(page_private(kpte_page) == 0);
+ BUG_ON(page_private_cpa_count(kpte_page) == 0);
page_private(kpte_page)--;
} else
BUG();
@@ -159,7 +166,7 @@
BUG_ON(PageReserved(kpte_page));

save_page(kpte_page);
- if (page_private(kpte_page) == 0)
+ if (page_private_cpa_count(kpte_page) == 0)
revert_page(address, ref_prot);
return 0;
}
@@ -232,7 +239,8 @@
flush_map(&l);

list_for_each_entry_safe(pg, next, &l, lru) {
- if (page_private(pg) != 0)
+ list_del(&pg->lru);
+ if (page_private_cpa_count(pg) != 0)
continue;
ClearPagePrivate(pg);
__free_page(pg);
--
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68

2007-06-29 04:20:26

by Mathieu Desnoyers

[permalink] [raw]

Subject: Re: [PATCH] fix x86_64-mm-cpa-cache-flush.patch in 2.6.22-rc4-mm2

> Seems that Andi has changed
> ftp://ftp.firstfloor.org/pub/ak/x86_64/quilt/patches/cpa-cache-flush so
> hopefully these fixes will not be needed any more..
>

Hrm, actually, something is very wrong with this patch. Please see
comments below.

...
> Index: linux/arch/x86_64/mm/pageattr.c
> ===================================================================
> --- linux.orig/arch/x86_64/mm/pageattr.c
> +++ linux/arch/x86_64/mm/pageattr.c
> @@ -234,7 +235,10 @@ void global_flush_tlb(void)
> flush_map(&l);
>
> list_for_each_entry_safe(pg, next, &l, lru) {
> + if (page_private(pg) != 0)
> + continue;
> ClearPagePrivate(pg);
> + clear_bit(PG_arch_1, &pg->flags);

First issue:

clear_bit should be before the if (page_private(pg) != 0). As a result,
in the current patch, the flush won't be done the second time flags
are changed in a page, because the bit would only be cleared when each
pte within the page are set back to normal page attributes.

I would also suggest putting a list_del before the if (page_private(pg)
!= 0). It is not required, but would make sure the pointers are
poisoned.

> __free_page(pg);
> }
> }
> Index: linux/arch/i386/mm/pageattr.c
> ===================================================================
> --- linux.orig/arch/i386/mm/pageattr.c
> +++ linux/arch/i386/mm/pageattr.c
> @@ -82,7 +82,7 @@ static void flush_kernel_map(void *arg)
> struct page *p;
>
> /* High level code is not ready for clflush yet */
> - if (0 && cpu_has_clflush) {
> + if (cpu_has_clflush) {
> list_for_each_entry (p, lh, lru)
> cache_flush_page(p);
> } else if (boot_cpu_data.x86_model >= 4)
> @@ -136,6 +136,12 @@ static inline void revert_page(struct pa
> ref_prot));
> }
>
> +static inline void save_page(struct page *kpte_page)
> +{
> + if (!test_and_set_bit(PG_arch_1, &kpte_page->flags))
> + list_add(&kpte_page->lru, &df_list);
> +}
> +
> static int
> __change_page_attr(struct page *page, pgprot_t prot)
> {
> @@ -150,6 +156,9 @@ __change_page_attr(struct page *page, pg
> if (!kpte)
> return -EINVAL;
> kpte_page = virt_to_page(kpte);
> + BUG_ON(PageLRU(kpte_page));
> + BUG_ON(PageCompound(kpte_page));
> +
> if (pgprot_val(prot) != pgprot_val(PAGE_KERNEL)) {
> if (!pte_huge(*kpte)) {
> set_pte_atomic(kpte, mk_pte(page, prot));
> @@ -179,11 +188,11 @@ __change_page_attr(struct page *page, pg
> * time (not via split_large_page) and in turn we must not
> * replace it with a largepage.
> */
> +
> + save_page(kpte_page);
> if (!PageReserved(kpte_page)) {
> if (cpu_has_pse && (page_private(kpte_page) == 0)) {
> - ClearPagePrivate(kpte_page);
> paravirt_release_pt(page_to_pfn(kpte_page));
> - list_add(&kpte_page->lru, &df_list);
> revert_page(kpte_page, address);
> }
> }
> @@ -236,6 +245,10 @@ void global_flush_tlb(void)
> spin_unlock_irq(&cpa_lock);
> flush_map(&l);
> list_for_each_entry_safe(pg, next, &l, lru) {
> + if (page_private(pg) != 0)
> + continue;
> + ClearPagePrivate(pg);
> + clear_bit(PG_arch_1, &pg->flags);

Same two issues pointed for x86_64 applies here: clear_bit and a
list_del should be put before the if statement.

Mathieu

> __free_page(pg);
> }
> }

--
Mathieu Desnoyers
Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68