[PATCH 1/2] Revert "xen: properly account for _PAGE_NUMA during xen pte translations"

[PATCH 1/2] Revert "xen: properly account for _PAGE_NUMA during xen pte translations"

[David Vrabel]

This reverts commit a9c8e4beeeb64c22b84c803747487857fe424b68.

PTEs in Xen PV guests must contain machine addresses if _PAGE_PRESENT
is set and pseudo-physical addresses is _PAGE_PRESENT is clear.

This is because during a domain save/restore (migration) the page
table entries are "canonicalised" and uncanonicalised". i.e., MFNs are
converted to PFNs during domain save so that on a restore the page
table entries may be rewritten with the new MFNs on the destination.

This change resulted in writing PTEs with MFNs if _PAGE_NUMA was set
but _PAGE_PRESENT was clear.  These PTEs would be migrated as-is which
would result in unexpected behaviour in the destination domain.
Either a) the MFN would be translated to the wrong PFN/page; b)
setting the _PAGE_PRESENT bit would clear the PTE because the MFN is
no longer owned by the domain; or c) the present bit would not get
set.

Symptoms include "Bad page" reports when unmapping after migrating a
domain.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Steven Noonan <steven@uplinklabs.net>
Cc: Elena Ufimtseva <ufimtseva@gmail.com>
Cc: Mel Gorman <mgorman@suse.de>
Cc: <stable@vger.kernel.org>        [3.12+]
---
 arch/x86/include/asm/pgtable.h |   14 ++------------
 arch/x86/xen/mmu.c             |    4 ++--
 2 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
index 5ad38ad..bbc8b12 100644
--- a/arch/x86/include/asm/pgtable.h
+++ b/arch/x86/include/asm/pgtable.h
@@ -445,20 +445,10 @@ static inline int pte_same(pte_t a, pte_t b)
 	return a.pte == b.pte;
 }
 
-static inline int pteval_present(pteval_t pteval)
-{
-	/*
-	 * Yes Linus, _PAGE_PROTNONE == _PAGE_NUMA. Expressing it this
-	 * way clearly states that the intent is that protnone and numa
-	 * hinting ptes are considered present for the purposes of
-	 * pagetable operations like zapping, protection changes, gup etc.
-	 */
-	return pteval & (_PAGE_PRESENT | _PAGE_PROTNONE | _PAGE_NUMA);
-}
-
 static inline int pte_present(pte_t a)
 {
-	return pteval_present(pte_flags(a));
+	return pte_flags(a) & (_PAGE_PRESENT | _PAGE_PROTNONE |
+			       _PAGE_NUMA);
 }
 
 #define pte_accessible pte_accessible
diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
index 256282e..2423ef0 100644
--- a/arch/x86/xen/mmu.c
+++ b/arch/x86/xen/mmu.c
@@ -365,7 +365,7 @@ void xen_ptep_modify_prot_commit(struct mm_struct *mm, unsigned long addr,
 /* Assume pteval_t is equivalent to all the other *val_t types. */
 static pteval_t pte_mfn_to_pfn(pteval_t val)
 {
-	if (pteval_present(val)) {
+	if (val & _PAGE_PRESENT) {
 		unsigned long mfn = (val & PTE_PFN_MASK) >> PAGE_SHIFT;
 		unsigned long pfn = mfn_to_pfn(mfn);
 
@@ -381,7 +381,7 @@ static pteval_t pte_mfn_to_pfn(pteval_t val)
 
 static pteval_t pte_pfn_to_mfn(pteval_t val)
 {
-	if (pteval_present(val)) {
+	if (val & _PAGE_PRESENT) {
 		unsigned long pfn = (val & PTE_PFN_MASK) >> PAGE_SHIFT;
 		pteval_t flags = val & PTE_FLAGS_MASK;
 		unsigned long mfn;
-- 
1.7.2.5

Re: [PATCH 1/2] Revert "xen: properly account for _PAGE_NUMA during xen pte translations"

[David Sutton]

[-- Attachment #1.1: Type: text/plain, Size: 1636 bytes --]

David,

> This reverts commit a9c8e4beeeb64c22b84c803747487857fe424b68.
>
Please note: this particular patch also helped to resolve an issue reported
by some of the people using the xen package under Arch Linux, where they
would see stability issues running certain software (particularly firefox)
under dom0 - the software would start crashing under relatively light
usage, with a kernel BUG message.
>
> PTEs in Xen PV guests must contain machine addresses if _PAGE_PRESENT
> is set and pseudo-physical addresses is _PAGE_PRESENT is clear.
>
> This is because during a domain save/restore (migration) the page
> table entries are "canonicalised" and uncanonicalised". i.e., MFNs are
> converted to PFNs during domain save so that on a restore the page
> table entries may be rewritten with the new MFNs on the destination.
>
> This change resulted in writing PTEs with MFNs if _PAGE_NUMA was set
> but _PAGE_PRESENT was clear. These PTEs would be migrated as-is which
> would result in unexpected behaviour in the destination domain.
> Either a) the MFN would be translated to the wrong PFN/page; b)
> setting the _PAGE_PRESENT bit would clear the PTE because the MFN is
> no longer owned by the domain; or c) the present bit would not get
> set.
>
> Symptoms include "Bad page" reports when unmapping after migrating a
> domain.
>
> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: Steven Noonan <steven@xxxxxxxxxxxxxx>
> Cc: Elena Ufimtseva <ufimtseva@xxxxxxxxx>
> Cc: Mel Gorman <mgorman@xxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> [3.12+]
> ---

Regards,

  David

[-- Attachment #1.2: Type: text/html, Size: 2026 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/2] Revert "xen: properly account for _PAGE_NUMA during xen pte translations"

[David Vrabel]

On 25/03/14 15:28, David Sutton wrote:
> David,
> 
>> This reverts commit a9c8e4beeeb64c22b84c803747487857fe424b68.
>>
> Please note: this particular patch also helped to resolve an issue
> reported by some of the people using the xen package under Arch Linux,
> where they would see stability issues running certain software
> (particularly firefox) under dom0 - the software would start crashing
> under relatively light usage, with a kernel BUG message.

We are aware that Xen PV guests will not work if CONFIG_NUMA_BALANCING
is enabled with this revert.  But that patch introduced fundamental
breakage to how a guest must read/write non-present page tables entries
which caused even worse breakage.

I suspect that the problematic patch 1667918b6483 ("mm: numa: clear numa
hinting information on mprotect") is not needed on x86 and could be
safely reverted as a temporarily fix.

David

Re: [PATCH 1/2] Revert "xen: properly account for _PAGE_NUMA during xen pte translations"

[Konrad Rzeszutek Wilk]

On Tue, Mar 25, 2014 at 05:13:51PM +0000, David Vrabel wrote:
> On 25/03/14 15:28, David Sutton wrote:
> > David,
> > 
> >> This reverts commit a9c8e4beeeb64c22b84c803747487857fe424b68.
> >>
> > Please note: this particular patch also helped to resolve an issue
> > reported by some of the people using the xen package under Arch Linux,
> > where they would see stability issues running certain software
> > (particularly firefox) under dom0 - the software would start crashing
> > under relatively light usage, with a kernel BUG message.
> 
> We are aware that Xen PV guests will not work if CONFIG_NUMA_BALANCING
> is enabled with this revert.  But that patch introduced fundamental
> breakage to how a guest must read/write non-present page tables entries
> which caused even worse breakage.
> 
> I suspect that the problematic patch 1667918b6483 ("mm: numa: clear numa
> hinting information on mprotect") is not needed on x86 and could be
> safely reverted as a temporarily fix.

Hey Andrew,

In the rc3 time-frame Steve Noonan (Amazon EC2 engineering) found an issue
with Mel's "mm: numa: clear numa hinting information on mprotect" patch
(see http://lists.xen.org/archives/html/xen-devel/2014-02/msg00169.html)

A patch was proposed, looked good, you took it in and .. an rc later I
realized that migration was busted (of course only seen on my automated
test system). Boris Ostrovsky took a look and after a lot of digging realized
that it is an general issue wherein we lose page's contents after a migration
if they have been treated with the hinting information. I saw it with
'iscsid' halting, but Boris confirmed that it is a general problem:

"This can be easily triggered by Linux' page-types
(tools/vm/page-types.c) after save/restore.

All it does is it walks the page tables (in fs/proc/task_mmu.c) and
eventually trips on bad page. For example:

# /page-types -p  2273 -L> /tmp/new
[ 2634.501440] pfn 0x159f75 highest_memmap_pfn=0x3ffff
[ 2634.502345] BUG: Bad page map in process page-types pte:159f75420
pmd:3178a067
"
Which means that any application should be able to trigger this depending
on whether 'mprotect' was doing its stuff and migration happend.

Please note that these guests have no NUMA information exposed at all.

I should also mention that the initial report was reproduced when
CONFIG_NUMA_BALANCING was enabled, and the 'xen: properly account for _PAGE_NUMA
during xen pte translations' fixed that. However it introduced another
bug that can be seen with either CONFIG_NUMA_BALANCING enabled or disabled.

So, we are reverting the 'xen: properly account for _PAGE_NUMA during xen pte translations'
and an GIT PULL to Linus was sent today with said revert along with
another fix. However going forward there needs to be a fix for the
regression that '("mm: numa: clear numa hinting information on mprotect'
introduced.

David Vrabel is furiously trying to figure out a nice fix that will satisfy
everybody - and Linus has been saying he might do an rc8 - but Linus might
change his mind and the fix might take more than a week to be hammered out,
tested, Acked, etc.

With that in mind I was wondering whether it would be possible to revert 
Mel's 1667918b6483 ("mm: numa: clear numa hinting information on mprotect")
so there is a bit more time in 3.15 to re-introduce his patch and also a
proper fix so that Xen guests won't fall flat on their face?

Thanks!
> 
> David
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 1/2] Revert "xen: properly account for _PAGE_NUMA during xen pte translations"

[David Vrabel]

On 25/03/14 18:19, Konrad Rzeszutek Wilk wrote:
> 
> With that in mind I was wondering whether it would be possible to revert 
> Mel's 1667918b6483 ("mm: numa: clear numa hinting information on mprotect")
> so there is a bit more time in 3.15 to re-introduce his patch and also a
> proper fix so that Xen guests won't fall flat on their face?

Unfortunately, I think this can only be safely reverted on architectures
with ARCH_WANTS_PROT_NUMA_PROT_NONE (x86) and would cause regressions on
other architectures with only ARCH_SUPPORTS_NUMA_BALANCING (powerpc).

David

Re: [PATCH 1/2] Revert "xen: properly account for _PAGE_NUMA during xen pte translations"

[David Sutton]

[-- Attachment #1.1: Type: text/plain, Size: 1296 bytes --]

David,

On Tue, Mar 25, 2014 at 12:13 PM, David Vrabel <david.vrabel@citrix.com>wrote:

> On 25/03/14 15:28, David Sutton wrote:
> > David,
> >
> >> This reverts commit a9c8e4beeeb64c22b84c803747487857fe424b68.
> >>
> > Please note: this particular patch also helped to resolve an issue
> > reported by some of the people using the xen package under Arch Linux,
> > where they would see stability issues running certain software
> > (particularly firefox) under dom0 - the software would start crashing
> > under relatively light usage, with a kernel BUG message.
>
> We are aware that Xen PV guests will not work if CONFIG_NUMA_BALANCING
> is enabled with this revert.  But that patch introduced fundamental
> breakage to how a guest must read/write non-present page tables entries
> which caused even worse breakage.
>
> Understood - I just wanted to make sure that another symptom of the issue
was known in case it came up - some people read the issue description and
were confused as they weren't using any PV guests, only seeing the specific
application crashes under dom0


> I suspect that the problematic patch 1667918b6483 ("mm: numa: clear numa
> hinting information on mprotect") is not needed on x86 and could be
> safely reverted as a temporarily fix.
>
> David
>

Regards,

  David

[-- Attachment #1.2: Type: text/html, Size: 2002 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 1/2] Revert "xen: properly account for _PAGE_NUMA during xen pte translations"

[David Vrabel]

On 25/03/14 19:03, David Sutton wrote:
> David,
> 
> On Tue, Mar 25, 2014 at 12:13 PM, David Vrabel <david.vrabel@citrix.com
> <mailto:david.vrabel@citrix.com>> wrote:
> 
>     On 25/03/14 15:28, David Sutton wrote:
>     > David,
>     >
>     >> This reverts commit a9c8e4beeeb64c22b84c803747487857fe424b68.
>     >>
>     > Please note: this particular patch also helped to resolve an issue
>     > reported by some of the people using the xen package under Arch Linux,
>     > where they would see stability issues running certain software
>     > (particularly firefox) under dom0 - the software would start crashing
>     > under relatively light usage, with a kernel BUG message.
> 
>     We are aware that Xen PV guests will not work if CONFIG_NUMA_BALANCING
>     is enabled with this revert.  But that patch introduced fundamental
>     breakage to how a guest must read/write non-present page tables entries
>     which caused even worse breakage.
> 
> Understood - I just wanted to make sure that another symptom of the
> issue was known in case it came up - some people read the issue
> description and were confused as they weren't using any PV guests, only
> seeing the specific application crashes under dom0

dom0 is a PV guest so it is affected.

David