Xen devel - Apr 2012 - [PATCH 0 of 2] Sharing bug fixes

Two small sharing bug fixes:

- Stats were going out of sync if unshare failed with ENOMEM.
- The test for a hole in the physmap in sharing_add_to_physmap was incomplete.
 This latter patch adds feedback from Tim Deegan.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

 xen/arch/x86/mm/mem_sharing.c |  2 ++
 xen/arch/x86/mm/mem_sharing.c |  7 ++++---
 xen/include/asm-x86/p2m.h     |  8 ++++++++
 3 files changed, 14 insertions(+), 3 deletions(-)

xen/arch/x86/mm/mem_sharing.c |  2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)


If unsharing fails, the decrease of the nr_saved_mfns stat was not being
undone. This would result in an underflow of the stat, as the retry would later
decrease the counter again.

Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 8a6f6d38cb84 -r 5be9a05f17fd xen/arch/x86/mm/mem_sharing.c
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1205,6 +1205,8 @@ int __mem_sharing_unshare_page(struct do
     page = alloc_domheap_page(d, 0);
     if ( !page ) 
     {
+        /* Undo dec of nr_saved_mfns, as the retry will decrease again. */
+        atomic_inc(&nr_saved_mfns);
         mem_sharing_page_unlock(old_page);
         put_gfn(d, gfn);
         /* Caller is responsible for placing an event

xen/arch/x86/mm/mem_sharing.c |  7 ++++---
 xen/include/asm-x86/p2m.h     |  8 ++++++++
 2 files changed, 12 insertions(+), 3 deletions(-)


Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

diff -r 5be9a05f17fd -r 51646b89b182 xen/arch/x86/mm/mem_sharing.c
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1073,9 +1073,10 @@ int mem_sharing_add_to_physmap(struct do
     if ( spage->sharing->handle != sh )
         goto err_unlock;
 
-    /* Make sure the target page is a hole in the physmap */
-    if ( mfn_valid(cmfn) ||
-         (!(p2m_is_ram(cmfn_type))) )
+    /* Make sure the target page is a hole in the physmap. These are typically
+     * p2m_mmio_dm, but also accept p2m_invalid and paged out pages. See the
+     * definition of p2m_is_hole in p2m.h. */
+    if ( !p2m_is_hole(cmfn_type) || mfn_valid(cmfn) )
     {
         ret = XENMEM_SHARING_OP_C_HANDLE_INVALID;
         goto err_unlock;
diff -r 5be9a05f17fd -r 51646b89b182 xen/include/asm-x86/p2m.h
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -133,6 +133,13 @@ typedef unsigned int p2m_query_t;
                        | p2m_to_mask(p2m_ram_paging_in)       \
                        | p2m_to_mask(p2m_ram_shared))
 
+/* Types that represent a physmap hole that is ok to replace with a shared
+ * entry */
+#define P2M_HOLE_TYPES (p2m_to_mask(p2m_mmio_dm)        \
+                       | p2m_to_mask(p2m_invalid)       \
+                       | p2m_to_mask(p2m_ram_paging_in) \
+                       | p2m_to_mask(p2m_ram_paged))
+
 /* Grant mapping types, which map to a real machine frame in another
  * VM */
 #define P2M_GRANT_TYPES (p2m_to_mask(p2m_grant_map_rw)  \
@@ -173,6 +180,7 @@ typedef unsigned int p2m_query_t;
 
 /* Useful predicates */
 #define p2m_is_ram(_t) (p2m_to_mask(_t) & P2M_RAM_TYPES)
+#define p2m_is_hole(_t) (p2m_to_mask(_t) & P2M_HOLE_TYPES)
 #define p2m_is_mmio(_t) (p2m_to_mask(_t) & P2M_MMIO_TYPES)
 #define p2m_is_readonly(_t) (p2m_to_mask(_t) & P2M_RO_TYPES)
 #define p2m_is_magic(_t) (p2m_to_mask(_t) & P2M_MAGIC_TYPES)

At 15:20 -0400 on 24 Apr (1335280819), Andres Lagar-Cavilla
wrote:
>  xen/arch/x86/mm/mem_sharing.c |  7 ++++---
>  xen/include/asm-x86/p2m.h     |  8 ++++++++
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> 
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> 
> diff -r 5be9a05f17fd -r 51646b89b182 xen/arch/x86/mm/mem_sharing.c
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1073,9 +1073,10 @@ int mem_sharing_add_to_physmap(struct do
>      if ( spage->sharing->handle != sh )
>          goto err_unlock;
>  
> -    /* Make sure the target page is a hole in the physmap */
> -    if ( mfn_valid(cmfn) ||
> -         (!(p2m_is_ram(cmfn_type))) )
> +    /* Make sure the target page is a hole in the physmap. These are
typically
> +     * p2m_mmio_dm, but also accept p2m_invalid and paged out pages. See
the
> +     * definition of p2m_is_hole in p2m.h. */
> +    if ( !p2m_is_hole(cmfn_type) || mfn_valid(cmfn) )
Hmm.  Is the mfn_valid() to handle p2m_ram_paging_in entries sometimes
having an MFN and sometimes not?  I think it would be nicer to either
always replace paging-in pages or never do it.  In any case, it''s bogus
to test mfn_valid() for any of the other types.

Cheers,

Tim.

> At 15:20 -0400 on 24 Apr (1335280819), Andres Lagar-Cavilla wrote:
>>  xen/arch/x86/mm/mem_sharing.c |  7 ++++---
>>  xen/include/asm-x86/p2m.h     |  8 ++++++++
>>  2 files changed, 12 insertions(+), 3 deletions(-)
>>
>>
>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>>
>> diff -r 5be9a05f17fd -r 51646b89b182 xen/arch/x86/mm/mem_sharing.c
>> --- a/xen/arch/x86/mm/mem_sharing.c
>> +++ b/xen/arch/x86/mm/mem_sharing.c
>> @@ -1073,9 +1073,10 @@ int mem_sharing_add_to_physmap(struct do
>>      if ( spage->sharing->handle != sh )
>>          goto err_unlock;
>>
>> -    /* Make sure the target page is a hole in the physmap */
>> -    if ( mfn_valid(cmfn) ||
>> -         (!(p2m_is_ram(cmfn_type))) )
>> +    /* Make sure the target page is a hole in the physmap. These are
>> typically
>> +     * p2m_mmio_dm, but also accept p2m_invalid and paged out pages.
>> See the
>> +     * definition of p2m_is_hole in p2m.h. */
>> +    if ( !p2m_is_hole(cmfn_type) || mfn_valid(cmfn) )
>
> Hmm.  Is the mfn_valid() to handle p2m_ram_paging_in entries sometimes
> having an MFN and sometimes not?  I think it would be nicer to either
> always replace paging-in pages or never do it.  In any case, it''s
bogus
> to test mfn_valid() for any of the other types.
Yes, that is the concern. paging_in entries may have a backing mfn. I am
not opposed to just replacing the entry if it is in paging_in state,
regardless of the mfn. It would be similar to any of the cases in which
guest_physmap_add_entry is used with a valid mfn in the entry to be
updated.

Andres
>
> Cheers,
>
> Tim.
>

At 15:20 -0400 on 24 Apr (1335280818), Andres Lagar-Cavilla
wrote:
>  xen/arch/x86/mm/mem_sharing.c |  2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> 
> If unsharing fails, the decrease of the nr_saved_mfns stat was not being
> undone. This would result in an underflow of the stat, as the retry would
later
> decrease the counter again.
Applied, thanks.