Re: [Xen-devel] PATastic fun

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Feb 26, 2013 at 06:11:42PM +0100, Stefan Bader wrote:
> On 26.02.2013 18:03, Konrad Rzeszutek Wilk wrote:
> > On Tue, Feb 26, 2013 at 04:32:37PM +0100, Stefan Bader wrote:
> >> On 25.02.2013 10:10, Stefan Bader wrote:
> >>> On 25.02.2013 04:15, Liu, Jinsong wrote:
> >>>> Konrad Rzeszutek Wilk wrote:
> >>>>> On Fri, Feb 22, 2013 at 02:54:16PM +0100, Stefan Bader wrote:
> >>>>>> Hi Konrad,
> >>>>>
> >>>>> Hey Stefan,
> >>>>>>
> >>>>>> here is another one from the hm-what? department:
> >>>>>
> >>>>> Heh - the really good-bug-hunting one. Lets also include Jinsong as
> >>>>> he has been tracking a similar one with mcelog.
> >>>>>>
> >>>>>> Colin discovered that running the attached program with the fork
> >>>>>> active (e.g. "./mmap-example -f 0x10000", the address can be that or
> >>>>>> iomem) this triggers the following weird messages: 
> >>>>>>
> >>>>>> [ 6824.453724] mmap-example:3481 map pfn expected mapping type
> >>>>>> write-back for [mem 0x00010000-0x00010fff], got uncached-minus
> >>>>>> [ 6824.453776] ------------[ cut here ]------------
> >>>>>> [ 6824.453796] WARNING: at
> >>>>>> /build/buildd/linux-3.8.0/arch/x86/mm/pat.c:774
> >>>>>> untrack_pfn+0xb8/0xd0() ... [ 6824.453920] Pid: 3481, comm:
> >>>>>> mmap-example Tainted: GF 
> >>>>>> 3.8.0-6-generic #13-Ubuntu
> >>>>>> [ 6824.453926] Call Trace:
> >>>>>> [ 6824.453944]  [<ffffffff8105879f>] warn_slowpath_common+0x7f/0xc0
> >>>>>> [ 6824.453954]  [<ffffffff810587fa>] warn_slowpath_null+0x1a/0x20
> >>>>>> [ 6824.453963]  [<ffffffff8104bcc8>] untrack_pfn+0xb8/0xd0
> >>>>>> [ 6824.453975]  [<ffffffff81156c1c>] unmap_single_vma+0xac/0x100
> >>>>>> [ 6824.453985]  [<ffffffff81157459>] unmap_vmas+0x49/0x90
> >>>>>> [ 6824.453995]  [<ffffffff8115f808>] exit_mmap+0x98/0x170
> >>>>>> [ 6824.454007]  [<ffffffff810559a4>] mmput+0x64/0x100
> >>>>>> [ 6824.454017]  [<ffffffff810560f5>] dup_mm+0x445/0x660
> >>>>>> [ 6824.454027]  [<ffffffff81056d9f>]
> >>>>>> copy_process.part.22+0xa5f/0x1510 [ 6824.454038] 
> >>>>>> [<ffffffff81057931>] do_fork+0x91/0x350 [ 6824.454048] 
> >>>>>> [<ffffffff81057c76>] sys_clone+0x16/0x20 [ 6824.454060] 
> >>>>>> [<ffffffff816ccbf9>] stub_clone+0x69/0x90 [ 6824.454069] 
> >>>>>> [<ffffffff816cc89d>] ? system_call_fastpath+0x1a/0x1f [ 6824.454076]
> >>>>>> ---[ end trace 4918cdd0a4c9fea4 ]--- 
> >>>>>>
> >>>>>> I found that this is related to your bandaid patch
> >>>>>>
> >>>>>> commit 8eaffa67b43e99ae581622c5133e20b0f48bcef1
> >>>>>> Author: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> >>>>>> Date:   Fri Feb 10 09:16:27 2012 -0500
> >>>>>>
> >>>>>>     xen/pat: Disable PAT support for now.
> >>>>>>
> >>>>>> I just do not understand how this happens. From the trace it seems
> >>>>>> the fork 
> >>>>>> fails when duplicating the VMAs (dup_mm calls mmput on failure). So
> >>>>>> maybe the 
> >>>>>> warning is just related to this. So primarily the question is how on
> >>>>>> fork the _PAGE_PCD bit can become set? That and _PAGE_PWT are
> >>>>>> cleared from the supported 
> >>>>>> mask by the patch, so somehow I would think nothing should be able
> >>>>>> to set it... 
> >>>>>> But apparently not so.
> >>>>>> Not sure it is a big deal since I never saw this in normal operation
> >>>>>> and it 
> >>>>>> seems to be ok when unapping before doing the fork. It is just plain
> >>>>>> odd. 
> >>>>>
> >>>>> Jinsong mentioned that there is some oddity with the MTRR. Somehow the
> >>>>> ranges are swapped or not correct. Jinsong, could you shed some light
> >>>>> on what you have found so far?
> >>>>>
> >>>>
> >>>> Yes, Sander once also reported a similar weird warning when start mcelog 
> >>>> daemon, as attached.
> >>>>
> >>>> Basically, it occurs when mcelog user daemon start, 
> >>>> do_fork
> >>>>   --> copy_process
> >>>>     --> dup_mm
> >>>>       --> dup_mmap
> >>>>         --> copy_page_range
> >>>>           --> track_pfn_copy
> >>>>             --> reserve_pfn_range
> >>
> >> So that makes it clearer as this will do
> >>
> >> reserve_memtype(...)
> >> --> pat_x_mtrr_type
> >>   --> mtrr_type_lookup
> >>     --> __mtrr_type_lookup
> >>
> >> And that can return -1/0xff in case of mtrr not being enabled/initialized. 
> >> Which
> >> is not the case (given there are no messages for it in dmesg). This is not 
> >> equal
> >> to MTRR_TYPE_WRBACK and thus becomes _PAGE_CACHE_UC_MINUS.
> >>
> >> It looks like the problem starts early in reserve_memtype:
> >>
> >>         if (!pat_enabled) {
> >>                 /* This is identical to page table setting without PAT */
> >>                 if (new_type) {
> >>                         if (req_type == _PAGE_CACHE_WC)
> >>                                 *new_type = _PAGE_CACHE_UC_MINUS;
> >>                         else
> >>                                 *new_type = req_type & _PAGE_CACHE_MASK;
> >>                 }
> >>                 return 0;
> >>         }
> >>
> >> This would be what we want, but only clearing the PWT and PCD flags from 
> >> the
> >> supported flags is not changing pat_enabled (which is 1 when PAT support is
> >> compiled into the kernel). Unfortunately the variable is local and since 
> >> there
> >> are not any messages about PAT in dmesg I would say pat_init() is not 
> >> called
> >> either. Which might be used to disable PAT support by clearing the CPU 
> >> feature
> >> flag.
> > 
> > Hm, so:
> > 
> > diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> > index 39928d1..9ac70c5 100644
> > --- a/arch/x86/xen/enlighten.c
> > +++ b/arch/x86/xen/enlighten.c
> > @@ -379,6 +379,7 @@ static void __init xen_init_cpuid_mask(void)
> >  
> >         cpuid_leaf1_edx_mask =
> >                 ~((1 << X86_FEATURE_MTRR) |  /* disable MTRR */
> > +                 (1 << X86_FEATURE_PAT) |   /* disable PAT */
> >                   (1 << X86_FEATURE_ACC));   /* thermal monitoring */
> >  
> >         if (!xen_initial_domain())
> > 
> > 
> > 
> > should do it right? Let me check on the troublesome machine I saw
> > it.
> > 
> I could try it as well but somehow my reading of pat_init() is that if that
> would have been called at all, there should be some message about PAT in 
> dmesg.
> And normal dom0 boots do not show anything.
> 
> It looks like pat_init only gets called from two places in mtrr code, and that
> probably is not done in dom0 either. So clearing the feature is one step, but 
> I
> would think that also there needs to be a call to pat_init...

So how about this super-complex patch:

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 39928d1..c8e1c7b 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -67,6 +67,7 @@
 #include <asm/hypervisor.h>
 #include <asm/mwait.h>
 #include <asm/pci_x86.h>
+#include <asm/pat.h>
 
 #ifdef CONFIG_ACPI
 #include <linux/acpi.h>
@@ -1417,7 +1418,14 @@ asmlinkage void __init xen_start_kernel(void)
         */
        acpi_numa = -1;
 #endif
-
+#ifdef CONFIG_X86_PAT
+       /*
+        * For right now disable the PAT. We should remove this once
+        * git commit 8eaffa67b43e99ae581622c5133e20b0f48bcef1
+        * (xen/pat: Disable PAT support for now) is reverted.
+        */
+       pat_enabled = 0;
+#endif
        /* Don't do the full vcpu_info placement stuff until we have a
           possible map and a non-dummy shared_info. */
        per_cpu(xen_vcpu, 0) = &HYPERVISOR_shared_info->vcpu_info[0];

trying it out now.
> 
> -Stefan
> 



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