[PATCH] x86/mm: print domain IDs instead of pointers

[PATCH] x86/mm: print domain IDs instead of pointers

[Jan Beulich]

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

Printing pointers to struct domain isn't really useful for initial
problem analysis. In get_page() also drop the page only after issuing
the log message, so that at the time of printing the state can be
considered reasonably consistent.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
It also slightly concerns me that page_get_owner_and_reference() can
(at least theoretically) return NULL even after having taken a page
reference (when ->v.inuse._domain is clear), in which case that
reference would never get dropped again. One case where this might be a
problem is sh_set_allocation(), which - other than all other uses of
page_set_owner(, NULL) - doesn't zap the refcount.

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2082,14 +2082,16 @@ int get_page(struct page_info *page, str
     if ( likely(owner == domain) )
         return 1;
 
-    if ( owner != NULL )
-        put_page(page);
-
     if ( !paging_mode_refcounts(domain) && !domain->is_dying )
         gprintk(XENLOG_INFO,
-                "Error pfn %lx: rd=%p, od=%p, caf=%08lx, taf=%" PRtype_info "\n",
-                page_to_mfn(page), domain, owner,
-                page->count_info, page->u.inuse.type_info);
+                "Error pfn %lx: rd=%d od=%d caf=%08lx taf=%" PRtype_info "\n",
+                page_to_mfn(page), domain->domain_id,
+                owner ? owner->domain_id : DOMID_INVALID,
+                page->count_info - !!owner, page->u.inuse.type_info);
+
+    if ( owner )
+        put_page(page);
+
     return 0;
 }
 
@@ -4225,9 +4227,11 @@ int replace_grant_host_mapping(
 int donate_page(
     struct domain *d, struct page_info *page, unsigned int memflags)
 {
+    const struct domain *owner = dom_xen;
+
     spin_lock(&d->page_alloc_lock);
 
-    if ( is_xen_heap_page(page) || (page_get_owner(page) != NULL) )
+    if ( is_xen_heap_page(page) || ((owner = page_get_owner(page)) != NULL) )
         goto fail;
 
     if ( d->is_dying )
@@ -4252,9 +4256,10 @@ int donate_page(
 
  fail:
     spin_unlock(&d->page_alloc_lock);
-    MEM_LOG("Bad donate %p: ed=%p(%u), sd=%p, caf=%08lx, taf=%" PRtype_info,
-            (void *)page_to_mfn(page), d, d->domain_id,
-            page_get_owner(page), page->count_info, page->u.inuse.type_info);
+    MEM_LOG("Bad donate %lx: ed=%d sd=%d caf=%08lx taf=%" PRtype_info,
+            page_to_mfn(page), d->domain_id,
+            owner ? owner->domain_id : DOMID_INVALID,
+            page->count_info, page->u.inuse.type_info);
     return -1;
 }
 
@@ -4263,10 +4268,11 @@ int steal_page(
 {
     unsigned long x, y;
     bool_t drop_dom_ref = 0;
+    const struct domain *owner = dom_xen;
 
     spin_lock(&d->page_alloc_lock);
 
-    if ( is_xen_heap_page(page) || (page_get_owner(page) != d) )
+    if ( is_xen_heap_page(page) || ((owner = page_get_owner(page)) != d) )
         goto fail;
 
     /*
@@ -4301,9 +4307,10 @@ int steal_page(
 
  fail:
     spin_unlock(&d->page_alloc_lock);
-    MEM_LOG("Bad page %p: ed=%p(%u), sd=%p, caf=%08lx, taf=%" PRtype_info,
-            (void *)page_to_mfn(page), d, d->domain_id,
-            page_get_owner(page), page->count_info, page->u.inuse.type_info);
+    MEM_LOG("Bad page %lx: ed=%d sd=%d caf=%08lx taf=%" PRtype_info,
+            page_to_mfn(page), d->domain_id,
+            owner ? owner->domain_id : DOMID_INVALID,
+            page->count_info, page->u.inuse.type_info);
     return -1;
 }
 




[-- Attachment #2: x86-get_page-warning.patch --]
[-- Type: text/plain, Size: 3549 bytes --]

x86/mm: print domain IDs instead of pointers

Printing pointers to struct domain isn't really useful for initial
problem analysis. In get_page() also drop the page only after issuing
the log message, so that at the time of printing the state can be
considered reasonably consistent.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
It also slightly concerns me that page_get_owner_and_reference() can
(at least theoretically) return NULL even after having taken a page
reference (when ->v.inuse._domain is clear), in which case that
reference would never get dropped again. One case where this might be a
problem is sh_set_allocation(), which - other than all other uses of
page_set_owner(, NULL) - doesn't zap the refcount.

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2082,14 +2082,16 @@ int get_page(struct page_info *page, str
     if ( likely(owner == domain) )
         return 1;
 
-    if ( owner != NULL )
-        put_page(page);
-
     if ( !paging_mode_refcounts(domain) && !domain->is_dying )
         gprintk(XENLOG_INFO,
-                "Error pfn %lx: rd=%p, od=%p, caf=%08lx, taf=%" PRtype_info "\n",
-                page_to_mfn(page), domain, owner,
-                page->count_info, page->u.inuse.type_info);
+                "Error pfn %lx: rd=%d od=%d caf=%08lx taf=%" PRtype_info "\n",
+                page_to_mfn(page), domain->domain_id,
+                owner ? owner->domain_id : DOMID_INVALID,
+                page->count_info - !!owner, page->u.inuse.type_info);
+
+    if ( owner )
+        put_page(page);
+
     return 0;
 }
 
@@ -4225,9 +4227,11 @@ int replace_grant_host_mapping(
 int donate_page(
     struct domain *d, struct page_info *page, unsigned int memflags)
 {
+    const struct domain *owner = dom_xen;
+
     spin_lock(&d->page_alloc_lock);
 
-    if ( is_xen_heap_page(page) || (page_get_owner(page) != NULL) )
+    if ( is_xen_heap_page(page) || ((owner = page_get_owner(page)) != NULL) )
         goto fail;
 
     if ( d->is_dying )
@@ -4252,9 +4256,10 @@ int donate_page(
 
  fail:
     spin_unlock(&d->page_alloc_lock);
-    MEM_LOG("Bad donate %p: ed=%p(%u), sd=%p, caf=%08lx, taf=%" PRtype_info,
-            (void *)page_to_mfn(page), d, d->domain_id,
-            page_get_owner(page), page->count_info, page->u.inuse.type_info);
+    MEM_LOG("Bad donate %lx: ed=%d sd=%d caf=%08lx taf=%" PRtype_info,
+            page_to_mfn(page), d->domain_id,
+            owner ? owner->domain_id : DOMID_INVALID,
+            page->count_info, page->u.inuse.type_info);
     return -1;
 }
 
@@ -4263,10 +4268,11 @@ int steal_page(
 {
     unsigned long x, y;
     bool_t drop_dom_ref = 0;
+    const struct domain *owner = dom_xen;
 
     spin_lock(&d->page_alloc_lock);
 
-    if ( is_xen_heap_page(page) || (page_get_owner(page) != d) )
+    if ( is_xen_heap_page(page) || ((owner = page_get_owner(page)) != d) )
         goto fail;
 
     /*
@@ -4301,9 +4307,10 @@ int steal_page(
 
  fail:
     spin_unlock(&d->page_alloc_lock);
-    MEM_LOG("Bad page %p: ed=%p(%u), sd=%p, caf=%08lx, taf=%" PRtype_info,
-            (void *)page_to_mfn(page), d, d->domain_id,
-            page_get_owner(page), page->count_info, page->u.inuse.type_info);
+    MEM_LOG("Bad page %lx: ed=%d sd=%d caf=%08lx taf=%" PRtype_info,
+            page_to_mfn(page), d->domain_id,
+            owner ? owner->domain_id : DOMID_INVALID,
+            page->count_info, page->u.inuse.type_info);
     return -1;
 }
 

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

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

Re: [PATCH] x86/mm: print domain IDs instead of pointers

[Andrew Cooper]

On 05/06/15 11:27, Jan Beulich wrote:
> Printing pointers to struct domain isn't really useful for initial
> problem analysis. In get_page() also drop the page only after issuing
> the log message, so that at the time of printing the state can be
> considered reasonably consistent.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

There are a few areas of code which seem overzealous with printing Xen
pointers.

> ---
> It also slightly concerns me that page_get_owner_and_reference() can
> (at least theoretically) return NULL even after having taken a page
> reference (when ->v.inuse._domain is clear), in which case that
> reference would never get dropped again. One case where this might be a
> problem is sh_set_allocation(), which - other than all other uses of
> page_set_owner(, NULL) - doesn't zap the refcount.

Looks as if page_get_owner_and_reference() should drop the ref itself if
->v.inuse._domain is clear.

Alternatively, the implication is that page_get_owner_and_reference() is
called on a page with a real owner, which might justify an ASSERT/BUG.

A different area which concerns me is __acquire_grant_for_copy() which
ignores the return value.  The caller of __acquire_grant_for_copy() cant
actually be certain that a ref was taken on the page, based on a return
value of GNTST_okay.

~Andrew

Re: [PATCH] x86/mm: print domain IDs instead of pointers

[Jan Beulich]

>>> On 05.06.15 at 12:53, <andrew.cooper3@citrix.com> wrote:
> On 05/06/15 11:27, Jan Beulich wrote:
>> Printing pointers to struct domain isn't really useful for initial
>> problem analysis. In get_page() also drop the page only after issuing
>> the log message, so that at the time of printing the state can be
>> considered reasonably consistent.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> There are a few areas of code which seem overzealous with printing Xen
> pointers.
> 
>> ---
>> It also slightly concerns me that page_get_owner_and_reference() can
>> (at least theoretically) return NULL even after having taken a page
>> reference (when ->v.inuse._domain is clear), in which case that
>> reference would never get dropped again. One case where this might be a
>> problem is sh_set_allocation(), which - other than all other uses of
>> page_set_owner(, NULL) - doesn't zap the refcount.
> 
> Looks as if page_get_owner_and_reference() should drop the ref itself if
> ->v.inuse._domain is clear.
> 
> Alternatively, the implication is that page_get_owner_and_reference() is
> called on a page with a real owner, which might justify an ASSERT/BUG.

I think there ought to be an ASSERT(). Unless Keir or Tim know of a
reason why not.

> A different area which concerns me is __acquire_grant_for_copy() which
> ignores the return value.  The caller of __acquire_grant_for_copy() cant
> actually be certain that a ref was taken on the page, based on a return
> value of GNTST_okay.

This is in a path where an in-use active entry was found, i.e. the
page ought to have a non-zero refcount and be owned by the
granting domain (or maybe another one for transitive grants, or
dom_io if this is an MMIO page - I don't recall whether we allow
this or only meant to at some point). I.e. another ASSERT() (or
even BUG()) perhaps.

Jan