Sharing bug fixes

PATCH New Andres Lagar-Cavilla 2 participants 6 messages Apr 24, 2012 – Apr 26, 2012

lore

Share & tools

lore

Andres Lagar-Cavilla

author

2012-04-24 19:20 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

 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(-)

Andres Lagar-Cavilla

author

2012-04-24 19:20 UTC #1 lore raw

PATCH 1/1 x86/mem_sharing: Fix saved mfns stat for failed unsharing

 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 <[email protected]>

xen/arch/x86/mm/mem_sharing.c +2 -0

diff -r 8a6f6d38cb84 -r 5be9a05f17fd 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

Tim Deegan

Applied

2012-04-26 09:13 UTC #2 lore raw

Re: [PATCH 1 of 2] x86/mem_sharing: Fix saved mfns stat for failed unsharing

Quoted 7 lines · Andres Lagar-Cavilla

 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.

Andres Lagar-Cavilla

author

2012-04-24 19:20 UTC #3 lore raw

PATCH 1/1 x86/mem_sharing: Rectify test for "empty" physmap entry in sharing_add_to_physmap

In reply to Andres Lagar-Cavilla

 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 <[email protected]>

xen/arch/x86/mm/mem_sharing.c +4 -3

diff -r 5be9a05f17fd -r 51646b89b182 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;

xen/include/asm-x86/p2m.h +8 -0

diff -r 5be9a05f17fd -r 51646b89b182 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)

Tim Deegan

2012-04-25 12:41 UTC #4 lore raw

Re: [PATCH 2 of 2] x86/mem_sharing: Rectify test for "empty" physmap entry in sharing_add_to_physmap

Quoted 11 lines · Andres Lagar-Cavilla

 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 <[email protected]>
diff -r 5be9a05f17fd -r 51646b89b182 xen/arch/x86/mm/mem_sharing.c

xen/arch/x86/mm/mem_sharing.c int mem_sharing_add_to_physmap(struct do · line 1079

Quoted 10 lines

xen/arch/x86/mm/mem_sharing.c +4 -3

@@ -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.

Andres Lagar-Cavilla

author

2012-04-25 15:20 UTC #5 lore raw

Re: [PATCH 2 of 2] x86/mem_sharing: Rectify test for "empty" physmap entry in sharing_add_to_physmap

Quoted 15 lines

Quoted 14 lines · Andres Lagar-Cavilla
 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 <[email protected]>
diff -r 5be9a05f17fd -r 51646b89b182 xen/arch/x86/mm/mem_sharing.c
xen/arch/x86/mm/mem_sharing.c +0 -0
@@ -1073,9 +1073,10 @@ int mem_sharing_add_to_physmap(struct do
     if ( spage->sharing->handle != sh )
         goto err_unlock;
 

Quoted 14 lines

xen/arch/x86/mm/mem_sharing.c int mem_sharing_add_to_physmap(struct do · line 1079
Quoted 9 lines
xen/arch/x86/mm/mem_sharing.c +1 -3
-    /* 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
xen/arch/x86/mm/mem_sharing.c +2 -0
+     * 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

Quoted 5 lines

Cheers,
Tim.