[PATCH] PoD: Check p2m assumption in debug builds

[PATCH] PoD: Check p2m assumption in debug builds

[George Dunlap]

[-- Attachment #1: Type: text/plain, Size: 286 bytes --]

The PoD code assumes that if:
* A page is in a domain's p2m table
* And it's owned by the domain
* And it's not a xenheap page
then:
* It's on the domain's page list.

This patch adds a check for this assumption when debug=y.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

[-- Attachment #2: 20090911-pod-verify-p2m-assumption.diff --]
[-- Type: text/x-diff, Size: 1450 bytes --]

diff -r bd54f26a3415 xen/arch/x86/mm/p2m.c
--- a/xen/arch/x86/mm/p2m.c	Fri Sep 11 11:34:42 2009 +0100
+++ b/xen/arch/x86/mm/p2m.c	Fri Sep 11 11:35:05 2009 +0100
@@ -1529,6 +1529,36 @@
                 9 : 0;
         else
             order = 0;
+
+#ifndef NDEBUG
+        /* PoD code assumes that a page owned by the domain, not from the xenheap, and in the p2m
+         * is on the domain page list.  Verify this assumption. */
+        if ( mfn_valid(mfn)
+             && p2m_is_ram(p2mt)
+             && page_get_owner(mfn_to_page(mfn))==d
+             && ( (mfn_to_page(mfn)->count_info & PGC_xen_heap) == 0 ) )
+        {
+            struct page_info *p, *q;
+
+            p = mfn_to_page(mfn);
+
+            spin_lock(&d->page_alloc_lock);
+           
+            /* Walk the domain page list and make sure this page is on it... */
+            for ( q = page_list_first(&d->page_list) ; q; q = page_list_next(q, &d->page_list) )
+                if ( q == p )
+                    break;
+            if ( !q )
+            {
+                printk("%s: mfn %lx owned by d%d, not xen_heap, but not on domain page_list!\n",
+                       __func__, mfn_x(mfn), d->domain_id);
+                BUG();
+            }   
+
+            spin_unlock(&d->page_alloc_lock);
+        }
+#endif
+
         rc = d->arch.p2m->set_entry(d, gfn, mfn, order, p2mt);
         gfn += 1ul << order;
         if ( mfn_x(mfn) != INVALID_MFN )

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] PoD: Check p2m assumption in debug builds

[Kouya Shimura]

Hi, 

c/s 20194:582970a2d2dc overloads the system.

[BEFORE]
$ time xm create hvm.cfg memory=1024
Using config file "./hvm.cfg".
Started domain hvm (id=3)

real	0m2.343s
user	0m0.040s
sys	0m0.030s

[AFTER]
$ time xm create hvm.cfg memory=1024
Using config file "./hvm.cfg".
Started domain hvm (id=2)

real	2m25.741s
user	0m0.090s
sys	0m0.020s

I'm about to lose my patience with it. :-)
Can you control it by a boot parameter? (ex. "pod_debug=nnn")

Thanks,
Kouya

George Dunlap writes:
> The PoD code assumes that if:
> * A page is in a domain's p2m table
> * And it's owned by the domain
> * And it's not a xenheap page
> then:
> * It's on the domain's page list.
> 
> This patch adds a check for this assumption when debug=y.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Re: [PATCH] PoD: Check p2m assumption in debug builds

[Keir Fraser]

On 17/09/2009 02:26, "Kouya Shimura" <kouya@jp.fujitsu.com> wrote:

> real 0m2.343s
> user 0m0.040s
> sys 0m0.030s
> 
> real 2m25.741s
> user 0m0.090s
> sys 0m0.020s
> 
> I'm about to lose my patience with it. :-)
> Can you control it by a boot parameter? (ex. "pod_debug=nnn")

Yeah, that's too slow for our normal debug build. I think we'll revert the
patch -- noone would ever enable it, unless maybe if helping George track
down a PoD bug.

 -- Keir

Re: [PATCH] PoD: Check p2m assumption in debug builds

[George Dunlap]

Keir Fraser wrote:
> On 17/09/2009 02:26, "Kouya Shimura" <kouya@jp.fujitsu.com> wrote:
>
>   
>> real 0m2.343s
>> user 0m0.040s
>> sys 0m0.030s
>>
>> real 2m25.741s
>> user 0m0.090s
>> sys 0m0.020s
>>
>> I'm about to lose my patience with it. :-)
>> Can you control it by a boot parameter? (ex. "pod_debug=nnn")
>>     
>
> Yeah, that's too slow for our normal debug build. I think we'll revert the
> patch -- noone would ever enable it, unless maybe if helping George track
> down a PoD bug.
>   
Hmm... my only concern is that the assumption seems like something which 
might easily change, with no one realizing that they'd broken the 
assumption, and the codepath not being tested since most OSS users 
probably won't be using PoD.

Hmm, perhaps we could wrap it with an if() statement checking if there 
are any outstanding PoD entries?  The assumption only really matters 
when PoD is in use.

Alternately, a separate #define for PoD debug might be useful, so we can 
easily turn on all these options when we need to.

 -George