[PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

Calling _put_page_type while also holding the page_lock
for that page can cause a deadlock.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
---
v3: simplified patch by keeping the additional references already in-place
---
 xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index dfc279d371..e2f74ac770 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct page_info *page)
         return -EBUSY;
     }
 
-    /* We can only change the type if count is one */
-    /* Because we are locking pages individually, we need to drop
-     * the lock here, while the page is typed. We cannot risk the 
-     * race of page_unlock and then put_page_type. */
     expected_type = (PGT_shared_page | PGT_validated | PGT_locked | 2);
     if ( page->u.inuse.type_info != expected_type )
     {
@@ -660,12 +656,12 @@ static int page_make_private(struct domain *d, struct page_info *page)
         return -EEXIST;
     }
 
-    /* Drop the final typecount */
-    put_page_and_type(page);
-
     /* Now that we've dropped the type, we can unlock */
     mem_sharing_page_unlock(page);
 
+    /* Drop the final typecount */
+    put_page_and_type(page);
+
     /* Change the owner */
     ASSERT(page_get_owner(page) == dom_cow);
     page_set_owner(page, d);
@@ -900,6 +896,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
     p2m_type_t smfn_type, cmfn_type;
     struct two_gfns tg;
     struct rmap_iterator ri;
+    unsigned long put_count = 0;
 
     get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
                  cd, cgfn, &cmfn_type, NULL, &cmfn, 0, &tg);
@@ -984,7 +981,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
          * Don't change the type of rmap for the client page. */
         rmap_del(gfn, cpage, 0);
         rmap_add(gfn, spage);
-        put_page_and_type(cpage);
+        put_count++;
         d = get_domain_by_id(gfn->domain);
         BUG_ON(!d);
         BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
@@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
     mem_sharing_page_unlock(secondpg);
     mem_sharing_page_unlock(firstpg);
 
+    BUG_ON(!put_count);
+    while ( put_count-- )
+        put_page_and_type(cpage);
+
     /* Free the client page */
     if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
         put_page(cpage);
@@ -1167,8 +1168,8 @@ int __mem_sharing_unshare_page(struct domain *d,
     {
         if ( !last_gfn )
             mem_sharing_gfn_destroy(page, d, gfn_info);
-        put_page_and_type(page);
         mem_sharing_page_unlock(page);
+        put_page_and_type(page);
         if ( last_gfn )
         {
             if ( !get_page(page, dom_cow) )
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

Calling _put_page_type while also holding the page_lock
for that page can cause a deadlock.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
---
v3: simplified patch by keeping the additional references already in-place
---
 xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index dfc279d371..e2f74ac770 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct page_info *page)
         return -EBUSY;
     }
 
-    /* We can only change the type if count is one */
-    /* Because we are locking pages individually, we need to drop
-     * the lock here, while the page is typed. We cannot risk the 
-     * race of page_unlock and then put_page_type. */
     expected_type = (PGT_shared_page | PGT_validated | PGT_locked | 2);
     if ( page->u.inuse.type_info != expected_type )
     {
@@ -660,12 +656,12 @@ static int page_make_private(struct domain *d, struct page_info *page)
         return -EEXIST;
     }
 
-    /* Drop the final typecount */
-    put_page_and_type(page);
-
     /* Now that we've dropped the type, we can unlock */
     mem_sharing_page_unlock(page);
 
+    /* Drop the final typecount */
+    put_page_and_type(page);
+
     /* Change the owner */
     ASSERT(page_get_owner(page) == dom_cow);
     page_set_owner(page, d);
@@ -900,6 +896,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
     p2m_type_t smfn_type, cmfn_type;
     struct two_gfns tg;
     struct rmap_iterator ri;
+    unsigned long put_count = 0;
 
     get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
                  cd, cgfn, &cmfn_type, NULL, &cmfn, 0, &tg);
@@ -984,7 +981,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
          * Don't change the type of rmap for the client page. */
         rmap_del(gfn, cpage, 0);
         rmap_add(gfn, spage);
-        put_page_and_type(cpage);
+        put_count++;
         d = get_domain_by_id(gfn->domain);
         BUG_ON(!d);
         BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
@@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
     mem_sharing_page_unlock(secondpg);
     mem_sharing_page_unlock(firstpg);
 
+    BUG_ON(!put_count);
+    while ( put_count-- )
+        put_page_and_type(cpage);
+
     /* Free the client page */
     if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
         put_page(cpage);
@@ -1167,8 +1168,8 @@ int __mem_sharing_unshare_page(struct domain *d,
     {
         if ( !last_gfn )
             mem_sharing_gfn_destroy(page, d, gfn_info);
-        put_page_and_type(page);
         mem_sharing_page_unlock(page);
+        put_page_and_type(page);
         if ( last_gfn )
         {
             if ( !get_page(page, dom_cow) )
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Tamas K Lengyel]

Patch cf4b30dca0a "Add debug code to detect illegal page_lock and put_page_type
ordering" added extra sanity checking to page_lock/page_unlock for debug builds
with the assumption that no hypervisor path ever locks two pages at once.

This assumption doesn't hold during memory sharing so we introduce separate
functions, page_lock_memshr and page_unlock_memshr, to be used exclusively
in the memory sharing subsystem.

Also placing these functions behind their appropriate kconfig gates.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
v3: this patch was "x86/mm: conditionally check page_lock/page_unlock ownership"
---
 xen/arch/x86/mm.c             | 46 ++++++++++++++++++++++++++++-------
 xen/arch/x86/mm/mem_sharing.c |  4 +--
 xen/include/asm-x86/mm.h      |  6 ++++-
 3 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 45fadbab61..c2c92a96ac 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2030,12 +2030,11 @@ static inline bool current_locked_page_ne_check(struct page_info *page) {
 #define current_locked_page_ne_check(x) true
 #endif
 
-int page_lock(struct page_info *page)
+#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
+static int _page_lock(struct page_info *page)
 {
     unsigned long x, nx;
 
-    ASSERT(current_locked_page_check(NULL));
-
     do {
         while ( (x = page->u.inuse.type_info) & PGT_locked )
             cpu_relax();
@@ -2046,17 +2045,13 @@ int page_lock(struct page_info *page)
             return 0;
     } while ( cmpxchg(&page->u.inuse.type_info, x, nx) != x );
 
-    current_locked_page_set(page);
-
     return 1;
 }
 
-void page_unlock(struct page_info *page)
+static void _page_unlock(struct page_info *page)
 {
     unsigned long x, nx, y = page->u.inuse.type_info;
 
-    ASSERT(current_locked_page_check(page));
-
     do {
         x = y;
         ASSERT((x & PGT_count_mask) && (x & PGT_locked));
@@ -2065,11 +2060,44 @@ void page_unlock(struct page_info *page)
         /* We must not drop the last reference here. */
         ASSERT(nx & PGT_count_mask);
     } while ( (y = cmpxchg(&page->u.inuse.type_info, x, nx)) != x );
+}
+#endif
 
-    current_locked_page_set(NULL);
+#ifdef CONFIG_HAS_MEM_SHARING
+int page_lock_memshr(struct page_info *page)
+{
+    return _page_lock(page);
 }
 
+void page_unlock_memshr(struct page_info *page)
+{
+    _page_unlock(page);
+}
+#endif
+
 #ifdef CONFIG_PV
+int page_lock(struct page_info *page)
+{
+    int rc;
+
+    ASSERT(current_locked_page_check(NULL));
+
+    rc = _page_lock(page);
+
+    current_locked_page_set(page);
+
+    return rc;
+}
+
+void page_unlock(struct page_info *page)
+{
+    ASSERT(current_locked_page_check(page));
+
+    _page_unlock(page);
+
+    current_locked_page_set(NULL);
+}
+
 /*
  * PTE flags that a guest may change without re-validating the PTE.
  * All other bits affect translation, caching, or Xen's safety.
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index e2f74ac770..4b60bab28b 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -118,7 +118,7 @@ static inline int mem_sharing_page_lock(struct page_info *pg)
     pg_lock_data_t *pld = &(this_cpu(__pld));
 
     page_sharing_mm_pre_lock();
-    rc = page_lock(pg);
+    rc = page_lock_memshr(pg);
     if ( rc )
     {
         preempt_disable();
@@ -135,7 +135,7 @@ static inline void mem_sharing_page_unlock(struct page_info *pg)
     page_sharing_mm_unlock(pld->mm_unlock_level, 
                            &pld->recurse_count);
     preempt_enable();
-    page_unlock(pg);
+    page_unlock_memshr(pg);
 }
 
 static inline shr_handle_t get_next_handle(void)
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 6faa563167..ba49eee24d 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -356,7 +356,8 @@ struct platform_bad_page {
 const struct platform_bad_page *get_platform_badpages(unsigned int *array_size);
 
 /* Per page locks:
- * page_lock() is used for two purposes: pte serialization, and memory sharing.
+ * page_lock() is used for pte serialization.
+ * page_lock_memshr() is used for memory sharing.
  *
  * All users of page lock for pte serialization live in mm.c, use it
  * to lock a page table page during pte updates, do not take other locks within
@@ -378,6 +379,9 @@ const struct platform_bad_page *get_platform_badpages(unsigned int *array_size);
 int page_lock(struct page_info *page);
 void page_unlock(struct page_info *page);
 
+int page_lock_memshr(struct page_info *page);
+void page_unlock_memshr(struct page_info *page);
+
 void put_page_type(struct page_info *page);
 int  get_page_type(struct page_info *page, unsigned long type);
 int  put_page_type_preemptible(struct page_info *page);
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Tamas K Lengyel]

Patch cf4b30dca0a "Add debug code to detect illegal page_lock and put_page_type
ordering" added extra sanity checking to page_lock/page_unlock for debug builds
with the assumption that no hypervisor path ever locks two pages at once.

This assumption doesn't hold during memory sharing so we introduce separate
functions, page_lock_memshr and page_unlock_memshr, to be used exclusively
in the memory sharing subsystem.

Also placing these functions behind their appropriate kconfig gates.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
v3: this patch was "x86/mm: conditionally check page_lock/page_unlock ownership"
---
 xen/arch/x86/mm.c             | 46 ++++++++++++++++++++++++++++-------
 xen/arch/x86/mm/mem_sharing.c |  4 +--
 xen/include/asm-x86/mm.h      |  6 ++++-
 3 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 45fadbab61..c2c92a96ac 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2030,12 +2030,11 @@ static inline bool current_locked_page_ne_check(struct page_info *page) {
 #define current_locked_page_ne_check(x) true
 #endif
 
-int page_lock(struct page_info *page)
+#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
+static int _page_lock(struct page_info *page)
 {
     unsigned long x, nx;
 
-    ASSERT(current_locked_page_check(NULL));
-
     do {
         while ( (x = page->u.inuse.type_info) & PGT_locked )
             cpu_relax();
@@ -2046,17 +2045,13 @@ int page_lock(struct page_info *page)
             return 0;
     } while ( cmpxchg(&page->u.inuse.type_info, x, nx) != x );
 
-    current_locked_page_set(page);
-
     return 1;
 }
 
-void page_unlock(struct page_info *page)
+static void _page_unlock(struct page_info *page)
 {
     unsigned long x, nx, y = page->u.inuse.type_info;
 
-    ASSERT(current_locked_page_check(page));
-
     do {
         x = y;
         ASSERT((x & PGT_count_mask) && (x & PGT_locked));
@@ -2065,11 +2060,44 @@ void page_unlock(struct page_info *page)
         /* We must not drop the last reference here. */
         ASSERT(nx & PGT_count_mask);
     } while ( (y = cmpxchg(&page->u.inuse.type_info, x, nx)) != x );
+}
+#endif
 
-    current_locked_page_set(NULL);
+#ifdef CONFIG_HAS_MEM_SHARING
+int page_lock_memshr(struct page_info *page)
+{
+    return _page_lock(page);
 }
 
+void page_unlock_memshr(struct page_info *page)
+{
+    _page_unlock(page);
+}
+#endif
+
 #ifdef CONFIG_PV
+int page_lock(struct page_info *page)
+{
+    int rc;
+
+    ASSERT(current_locked_page_check(NULL));
+
+    rc = _page_lock(page);
+
+    current_locked_page_set(page);
+
+    return rc;
+}
+
+void page_unlock(struct page_info *page)
+{
+    ASSERT(current_locked_page_check(page));
+
+    _page_unlock(page);
+
+    current_locked_page_set(NULL);
+}
+
 /*
  * PTE flags that a guest may change without re-validating the PTE.
  * All other bits affect translation, caching, or Xen's safety.
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index e2f74ac770..4b60bab28b 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -118,7 +118,7 @@ static inline int mem_sharing_page_lock(struct page_info *pg)
     pg_lock_data_t *pld = &(this_cpu(__pld));
 
     page_sharing_mm_pre_lock();
-    rc = page_lock(pg);
+    rc = page_lock_memshr(pg);
     if ( rc )
     {
         preempt_disable();
@@ -135,7 +135,7 @@ static inline void mem_sharing_page_unlock(struct page_info *pg)
     page_sharing_mm_unlock(pld->mm_unlock_level, 
                            &pld->recurse_count);
     preempt_enable();
-    page_unlock(pg);
+    page_unlock_memshr(pg);
 }
 
 static inline shr_handle_t get_next_handle(void)
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 6faa563167..ba49eee24d 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -356,7 +356,8 @@ struct platform_bad_page {
 const struct platform_bad_page *get_platform_badpages(unsigned int *array_size);
 
 /* Per page locks:
- * page_lock() is used for two purposes: pte serialization, and memory sharing.
+ * page_lock() is used for pte serialization.
+ * page_lock_memshr() is used for memory sharing.
  *
  * All users of page lock for pte serialization live in mm.c, use it
  * to lock a page table page during pte updates, do not take other locks within
@@ -378,6 +379,9 @@ const struct platform_bad_page *get_platform_badpages(unsigned int *array_size);
 int page_lock(struct page_info *page);
 void page_unlock(struct page_info *page);
 
+int page_lock_memshr(struct page_info *page);
+void page_unlock_memshr(struct page_info *page);
+
 void put_page_type(struct page_info *page);
 int  get_page_type(struct page_info *page, unsigned long type);
 int  put_page_type_preemptible(struct page_info *page);
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Jan Beulich]

>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> Patch cf4b30dca0a "Add debug code to detect illegal page_lock and put_page_type
> ordering" added extra sanity checking to page_lock/page_unlock for debug builds
> with the assumption that no hypervisor path ever locks two pages at once.
> 
> This assumption doesn't hold during memory sharing so we introduce separate
> functions, page_lock_memshr and page_unlock_memshr, to be used exclusively
> in the memory sharing subsystem.

Completely bypassing these checks looks undesirable to me, to
be honest. Otoh as discussed mem-sharing is abusing the lock
anyway. So if this is to remain that way, I wonder whether the
better course of action wouldn't be to stop abusing the
functions, cloning them to continue using the PGT_locked flag.
I would then wonder whether e.g. the checking of PGT_validated
would actually be needed in the cloned functions - that's again
a flag which ought to have meaning mainly (exclusively?) for
handling of PV guests.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2030,12 +2030,11 @@ static inline bool current_locked_page_ne_check(struct page_info *page) {
>  #define current_locked_page_ne_check(x) true
>  #endif
>  
> -int page_lock(struct page_info *page)
> +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
> +static int _page_lock(struct page_info *page)

As per above, personally I'm against introducing
page_{,un}lock_memshr(), as that makes the abuse even more
look like proper use. But if this was to be kept this way, may I
ask that you switch int -> bool in the return types at this occasion?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Jan Beulich]

>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> Patch cf4b30dca0a "Add debug code to detect illegal page_lock and put_page_type
> ordering" added extra sanity checking to page_lock/page_unlock for debug builds
> with the assumption that no hypervisor path ever locks two pages at once.
> 
> This assumption doesn't hold during memory sharing so we introduce separate
> functions, page_lock_memshr and page_unlock_memshr, to be used exclusively
> in the memory sharing subsystem.

Completely bypassing these checks looks undesirable to me, to
be honest. Otoh as discussed mem-sharing is abusing the lock
anyway. So if this is to remain that way, I wonder whether the
better course of action wouldn't be to stop abusing the
functions, cloning them to continue using the PGT_locked flag.
I would then wonder whether e.g. the checking of PGT_validated
would actually be needed in the cloned functions - that's again
a flag which ought to have meaning mainly (exclusively?) for
handling of PV guests.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -2030,12 +2030,11 @@ static inline bool current_locked_page_ne_check(struct page_info *page) {
>  #define current_locked_page_ne_check(x) true
>  #endif
>  
> -int page_lock(struct page_info *page)
> +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
> +static int _page_lock(struct page_info *page)

As per above, personally I'm against introducing
page_{,un}lock_memshr(), as that makes the abuse even more
look like proper use. But if this was to be kept this way, may I
ask that you switch int -> bool in the return types at this occasion?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 9:18 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> > Patch cf4b30dca0a "Add debug code to detect illegal page_lock and put_page_type
> > ordering" added extra sanity checking to page_lock/page_unlock for debug builds
> > with the assumption that no hypervisor path ever locks two pages at once.
> >
> > This assumption doesn't hold during memory sharing so we introduce separate
> > functions, page_lock_memshr and page_unlock_memshr, to be used exclusively
> > in the memory sharing subsystem.
>
> Completely bypassing these checks looks undesirable to me, to
> be honest. Otoh as discussed mem-sharing is abusing the lock
> anyway. So if this is to remain that way, I wonder whether the
> better course of action wouldn't be to stop abusing the
> functions, cloning them to continue using the PGT_locked flag.
> I would then wonder whether e.g. the checking of PGT_validated
> would actually be needed in the cloned functions - that's again
> a flag which ought to have meaning mainly (exclusively?) for
> handling of PV guests.

I don't have an answer to your question. I would prefer getting rid of
this whole thing myself. For now I'm just trying to make it usable
again.

>
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2030,12 +2030,11 @@ static inline bool current_locked_page_ne_check(struct page_info *page) {
> >  #define current_locked_page_ne_check(x) true
> >  #endif
> >
> > -int page_lock(struct page_info *page)
> > +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
> > +static int _page_lock(struct page_info *page)
>
> As per above, personally I'm against introducing
> page_{,un}lock_memshr(), as that makes the abuse even more
> look like proper use. But if this was to be kept this way, may I
> ask that you switch int -> bool in the return types at this occasion?

Switching them to bool would be fine. Replacing them with something
saner is unfortunately out-of-scope at the moment. Unless someone has
a specific solution that can be put in place. I don't have one.

Thanks,
Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 9:18 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> > Patch cf4b30dca0a "Add debug code to detect illegal page_lock and put_page_type
> > ordering" added extra sanity checking to page_lock/page_unlock for debug builds
> > with the assumption that no hypervisor path ever locks two pages at once.
> >
> > This assumption doesn't hold during memory sharing so we introduce separate
> > functions, page_lock_memshr and page_unlock_memshr, to be used exclusively
> > in the memory sharing subsystem.
>
> Completely bypassing these checks looks undesirable to me, to
> be honest. Otoh as discussed mem-sharing is abusing the lock
> anyway. So if this is to remain that way, I wonder whether the
> better course of action wouldn't be to stop abusing the
> functions, cloning them to continue using the PGT_locked flag.
> I would then wonder whether e.g. the checking of PGT_validated
> would actually be needed in the cloned functions - that's again
> a flag which ought to have meaning mainly (exclusively?) for
> handling of PV guests.

I don't have an answer to your question. I would prefer getting rid of
this whole thing myself. For now I'm just trying to make it usable
again.

>
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -2030,12 +2030,11 @@ static inline bool current_locked_page_ne_check(struct page_info *page) {
> >  #define current_locked_page_ne_check(x) true
> >  #endif
> >
> > -int page_lock(struct page_info *page)
> > +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
> > +static int _page_lock(struct page_info *page)
>
> As per above, personally I'm against introducing
> page_{,un}lock_memshr(), as that makes the abuse even more
> look like proper use. But if this was to be kept this way, may I
> ask that you switch int -> bool in the return types at this occasion?

Switching them to bool would be fine. Replacing them with something
saner is unfortunately out-of-scope at the moment. Unless someone has
a specific solution that can be put in place. I don't have one.

Thanks,
Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Jan Beulich]

>>> On 29.04.19 at 18:35, <tamas@tklengyel.com> wrote:
> On Mon, Apr 29, 2019 at 9:18 AM Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
>> > --- a/xen/arch/x86/mm.c
>> > +++ b/xen/arch/x86/mm.c
>> > @@ -2030,12 +2030,11 @@ static inline bool current_locked_page_ne_check(struct page_info *page) {
>> >  #define current_locked_page_ne_check(x) true
>> >  #endif
>> >
>> > -int page_lock(struct page_info *page)
>> > +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
>> > +static int _page_lock(struct page_info *page)
>>
>> As per above, personally I'm against introducing
>> page_{,un}lock_memshr(), as that makes the abuse even more
>> look like proper use. But if this was to be kept this way, may I
>> ask that you switch int -> bool in the return types at this occasion?
> 
> Switching them to bool would be fine. Replacing them with something
> saner is unfortunately out-of-scope at the moment. Unless someone has
> a specific solution that can be put in place. I don't have one.

I've outlined a solution already: Make a mem-sharing private variant
of page_{,un}lock(), derived from the PV ones (but with pieces
dropped you don't want/need).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Jan Beulich]

>>> On 29.04.19 at 18:35, <tamas@tklengyel.com> wrote:
> On Mon, Apr 29, 2019 at 9:18 AM Jan Beulich <JBeulich@suse.com> wrote:
>> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
>> > --- a/xen/arch/x86/mm.c
>> > +++ b/xen/arch/x86/mm.c
>> > @@ -2030,12 +2030,11 @@ static inline bool current_locked_page_ne_check(struct page_info *page) {
>> >  #define current_locked_page_ne_check(x) true
>> >  #endif
>> >
>> > -int page_lock(struct page_info *page)
>> > +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
>> > +static int _page_lock(struct page_info *page)
>>
>> As per above, personally I'm against introducing
>> page_{,un}lock_memshr(), as that makes the abuse even more
>> look like proper use. But if this was to be kept this way, may I
>> ask that you switch int -> bool in the return types at this occasion?
> 
> Switching them to bool would be fine. Replacing them with something
> saner is unfortunately out-of-scope at the moment. Unless someone has
> a specific solution that can be put in place. I don't have one.

I've outlined a solution already: Make a mem-sharing private variant
of page_{,un}lock(), derived from the PV ones (but with pieces
dropped you don't want/need).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Tamas K Lengyel]

On Tue, Apr 30, 2019 at 1:15 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 29.04.19 at 18:35, <tamas@tklengyel.com> wrote:
> > On Mon, Apr 29, 2019 at 9:18 AM Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> >> > --- a/xen/arch/x86/mm.c
> >> > +++ b/xen/arch/x86/mm.c
> >> > @@ -2030,12 +2030,11 @@ static inline bool current_locked_page_ne_check(struct page_info *page) {
> >> >  #define current_locked_page_ne_check(x) true
> >> >  #endif
> >> >
> >> > -int page_lock(struct page_info *page)
> >> > +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
> >> > +static int _page_lock(struct page_info *page)
> >>
> >> As per above, personally I'm against introducing
> >> page_{,un}lock_memshr(), as that makes the abuse even more
> >> look like proper use. But if this was to be kept this way, may I
> >> ask that you switch int -> bool in the return types at this occasion?
> >
> > Switching them to bool would be fine. Replacing them with something
> > saner is unfortunately out-of-scope at the moment. Unless someone has
> > a specific solution that can be put in place. I don't have one.
>
> I've outlined a solution already: Make a mem-sharing private variant
> of page_{,un}lock(), derived from the PV ones (but with pieces
> dropped you don't want/need).

Well, that's what I already did here in this patch. No?

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Tamas K Lengyel]

On Tue, Apr 30, 2019 at 1:15 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 29.04.19 at 18:35, <tamas@tklengyel.com> wrote:
> > On Mon, Apr 29, 2019 at 9:18 AM Jan Beulich <JBeulich@suse.com> wrote:
> >> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> >> > --- a/xen/arch/x86/mm.c
> >> > +++ b/xen/arch/x86/mm.c
> >> > @@ -2030,12 +2030,11 @@ static inline bool current_locked_page_ne_check(struct page_info *page) {
> >> >  #define current_locked_page_ne_check(x) true
> >> >  #endif
> >> >
> >> > -int page_lock(struct page_info *page)
> >> > +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
> >> > +static int _page_lock(struct page_info *page)
> >>
> >> As per above, personally I'm against introducing
> >> page_{,un}lock_memshr(), as that makes the abuse even more
> >> look like proper use. But if this was to be kept this way, may I
> >> ask that you switch int -> bool in the return types at this occasion?
> >
> > Switching them to bool would be fine. Replacing them with something
> > saner is unfortunately out-of-scope at the moment. Unless someone has
> > a specific solution that can be put in place. I don't have one.
>
> I've outlined a solution already: Make a mem-sharing private variant
> of page_{,un}lock(), derived from the PV ones (but with pieces
> dropped you don't want/need).

Well, that's what I already did here in this patch. No?

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Jan Beulich]

>>> On 30.04.19 at 10:28, <tamas@tklengyel.com> wrote:
> On Tue, Apr 30, 2019 at 1:15 AM Jan Beulich <JBeulich@suse.com> wrote:
>>
>> >>> On 29.04.19 at 18:35, <tamas@tklengyel.com> wrote:
>> > On Mon, Apr 29, 2019 at 9:18 AM Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
>> >> > --- a/xen/arch/x86/mm.c
>> >> > +++ b/xen/arch/x86/mm.c
>> >> > @@ -2030,12 +2030,11 @@ static inline bool 
> current_locked_page_ne_check(struct page_info *page) {
>> >> >  #define current_locked_page_ne_check(x) true
>> >> >  #endif
>> >> >
>> >> > -int page_lock(struct page_info *page)
>> >> > +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
>> >> > +static int _page_lock(struct page_info *page)
>> >>
>> >> As per above, personally I'm against introducing
>> >> page_{,un}lock_memshr(), as that makes the abuse even more
>> >> look like proper use. But if this was to be kept this way, may I
>> >> ask that you switch int -> bool in the return types at this occasion?
>> >
>> > Switching them to bool would be fine. Replacing them with something
>> > saner is unfortunately out-of-scope at the moment. Unless someone has
>> > a specific solution that can be put in place. I don't have one.
>>
>> I've outlined a solution already: Make a mem-sharing private variant
>> of page_{,un}lock(), derived from the PV ones (but with pieces
>> dropped you don't want/need).
> 
> Well, that's what I already did here in this patch. No?

No - you've retained a shared _page_{,un}lock(), whereas my
suggestion was to have a completely independent pair of
functions in mem_sharing.c. The only thing needed by both PV
and HVM would then be the PGT_locked flag.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Jan Beulich]

>>> On 30.04.19 at 10:28, <tamas@tklengyel.com> wrote:
> On Tue, Apr 30, 2019 at 1:15 AM Jan Beulich <JBeulich@suse.com> wrote:
>>
>> >>> On 29.04.19 at 18:35, <tamas@tklengyel.com> wrote:
>> > On Mon, Apr 29, 2019 at 9:18 AM Jan Beulich <JBeulich@suse.com> wrote:
>> >> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
>> >> > --- a/xen/arch/x86/mm.c
>> >> > +++ b/xen/arch/x86/mm.c
>> >> > @@ -2030,12 +2030,11 @@ static inline bool 
> current_locked_page_ne_check(struct page_info *page) {
>> >> >  #define current_locked_page_ne_check(x) true
>> >> >  #endif
>> >> >
>> >> > -int page_lock(struct page_info *page)
>> >> > +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
>> >> > +static int _page_lock(struct page_info *page)
>> >>
>> >> As per above, personally I'm against introducing
>> >> page_{,un}lock_memshr(), as that makes the abuse even more
>> >> look like proper use. But if this was to be kept this way, may I
>> >> ask that you switch int -> bool in the return types at this occasion?
>> >
>> > Switching them to bool would be fine. Replacing them with something
>> > saner is unfortunately out-of-scope at the moment. Unless someone has
>> > a specific solution that can be put in place. I don't have one.
>>
>> I've outlined a solution already: Make a mem-sharing private variant
>> of page_{,un}lock(), derived from the PV ones (but with pieces
>> dropped you don't want/need).
> 
> Well, that's what I already did here in this patch. No?

No - you've retained a shared _page_{,un}lock(), whereas my
suggestion was to have a completely independent pair of
functions in mem_sharing.c. The only thing needed by both PV
and HVM would then be the PGT_locked flag.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Tamas K Lengyel]

On Tue, Apr 30, 2019 at 2:44 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 30.04.19 at 10:28, <tamas@tklengyel.com> wrote:
> > On Tue, Apr 30, 2019 at 1:15 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>
> >> >>> On 29.04.19 at 18:35, <tamas@tklengyel.com> wrote:
> >> > On Mon, Apr 29, 2019 at 9:18 AM Jan Beulich <JBeulich@suse.com> wrote:
> >> >> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> >> >> > --- a/xen/arch/x86/mm.c
> >> >> > +++ b/xen/arch/x86/mm.c
> >> >> > @@ -2030,12 +2030,11 @@ static inline bool
> > current_locked_page_ne_check(struct page_info *page) {
> >> >> >  #define current_locked_page_ne_check(x) true
> >> >> >  #endif
> >> >> >
> >> >> > -int page_lock(struct page_info *page)
> >> >> > +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
> >> >> > +static int _page_lock(struct page_info *page)
> >> >>
> >> >> As per above, personally I'm against introducing
> >> >> page_{,un}lock_memshr(), as that makes the abuse even more
> >> >> look like proper use. But if this was to be kept this way, may I
> >> >> ask that you switch int -> bool in the return types at this occasion?
> >> >
> >> > Switching them to bool would be fine. Replacing them with something
> >> > saner is unfortunately out-of-scope at the moment. Unless someone has
> >> > a specific solution that can be put in place. I don't have one.
> >>
> >> I've outlined a solution already: Make a mem-sharing private variant
> >> of page_{,un}lock(), derived from the PV ones (but with pieces
> >> dropped you don't want/need).
> >
> > Well, that's what I already did here in this patch. No?
>
> No - you've retained a shared _page_{,un}lock(), whereas my
> suggestion was to have a completely independent pair of
> functions in mem_sharing.c. The only thing needed by both PV
> and HVM would then be the PGT_locked flag.

I see. Sure.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Tamas K Lengyel]

On Tue, Apr 30, 2019 at 2:44 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 30.04.19 at 10:28, <tamas@tklengyel.com> wrote:
> > On Tue, Apr 30, 2019 at 1:15 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>
> >> >>> On 29.04.19 at 18:35, <tamas@tklengyel.com> wrote:
> >> > On Mon, Apr 29, 2019 at 9:18 AM Jan Beulich <JBeulich@suse.com> wrote:
> >> >> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> >> >> > --- a/xen/arch/x86/mm.c
> >> >> > +++ b/xen/arch/x86/mm.c
> >> >> > @@ -2030,12 +2030,11 @@ static inline bool
> > current_locked_page_ne_check(struct page_info *page) {
> >> >> >  #define current_locked_page_ne_check(x) true
> >> >> >  #endif
> >> >> >
> >> >> > -int page_lock(struct page_info *page)
> >> >> > +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
> >> >> > +static int _page_lock(struct page_info *page)
> >> >>
> >> >> As per above, personally I'm against introducing
> >> >> page_{,un}lock_memshr(), as that makes the abuse even more
> >> >> look like proper use. But if this was to be kept this way, may I
> >> >> ask that you switch int -> bool in the return types at this occasion?
> >> >
> >> > Switching them to bool would be fine. Replacing them with something
> >> > saner is unfortunately out-of-scope at the moment. Unless someone has
> >> > a specific solution that can be put in place. I don't have one.
> >>
> >> I've outlined a solution already: Make a mem-sharing private variant
> >> of page_{,un}lock(), derived from the PV ones (but with pieces
> >> dropped you don't want/need).
> >
> > Well, that's what I already did here in this patch. No?
>
> No - you've retained a shared _page_{,un}lock(), whereas my
> suggestion was to have a completely independent pair of
> functions in mem_sharing.c. The only thing needed by both PV
> and HVM would then be the PGT_locked flag.

I see. Sure.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[George Dunlap]

On 4/30/19 9:44 AM, Jan Beulich wrote:
>>>> On 30.04.19 at 10:28, <tamas@tklengyel.com> wrote:
>> On Tue, Apr 30, 2019 at 1:15 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>
>>>>>> On 29.04.19 at 18:35, <tamas@tklengyel.com> wrote:
>>>> On Mon, Apr 29, 2019 at 9:18 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
>>>>>> --- a/xen/arch/x86/mm.c
>>>>>> +++ b/xen/arch/x86/mm.c
>>>>>> @@ -2030,12 +2030,11 @@ static inline bool 
>> current_locked_page_ne_check(struct page_info *page) {
>>>>>>  #define current_locked_page_ne_check(x) true
>>>>>>  #endif
>>>>>>
>>>>>> -int page_lock(struct page_info *page)
>>>>>> +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
>>>>>> +static int _page_lock(struct page_info *page)
>>>>>
>>>>> As per above, personally I'm against introducing
>>>>> page_{,un}lock_memshr(), as that makes the abuse even more
>>>>> look like proper use. But if this was to be kept this way, may I
>>>>> ask that you switch int -> bool in the return types at this occasion?
>>>>
>>>> Switching them to bool would be fine. Replacing them with something
>>>> saner is unfortunately out-of-scope at the moment. Unless someone has
>>>> a specific solution that can be put in place. I don't have one.
>>>
>>> I've outlined a solution already: Make a mem-sharing private variant
>>> of page_{,un}lock(), derived from the PV ones (but with pieces
>>> dropped you don't want/need).
>>
>> Well, that's what I already did here in this patch. No?
> 
> No - you've retained a shared _page_{,un}lock(), whereas my
> suggestion was to have a completely independent pair of
> functions in mem_sharing.c. The only thing needed by both PV
> and HVM would then be the PGT_locked flag.

But it wasn't obvious to me how the implementations of the actual lock
function would be be different.  And there's no point in having two
identical implementations; in fact, it would be harmful.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[George Dunlap]

On 4/30/19 9:44 AM, Jan Beulich wrote:
>>>> On 30.04.19 at 10:28, <tamas@tklengyel.com> wrote:
>> On Tue, Apr 30, 2019 at 1:15 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>
>>>>>> On 29.04.19 at 18:35, <tamas@tklengyel.com> wrote:
>>>> On Mon, Apr 29, 2019 at 9:18 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
>>>>>> --- a/xen/arch/x86/mm.c
>>>>>> +++ b/xen/arch/x86/mm.c
>>>>>> @@ -2030,12 +2030,11 @@ static inline bool 
>> current_locked_page_ne_check(struct page_info *page) {
>>>>>>  #define current_locked_page_ne_check(x) true
>>>>>>  #endif
>>>>>>
>>>>>> -int page_lock(struct page_info *page)
>>>>>> +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
>>>>>> +static int _page_lock(struct page_info *page)
>>>>>
>>>>> As per above, personally I'm against introducing
>>>>> page_{,un}lock_memshr(), as that makes the abuse even more
>>>>> look like proper use. But if this was to be kept this way, may I
>>>>> ask that you switch int -> bool in the return types at this occasion?
>>>>
>>>> Switching them to bool would be fine. Replacing them with something
>>>> saner is unfortunately out-of-scope at the moment. Unless someone has
>>>> a specific solution that can be put in place. I don't have one.
>>>
>>> I've outlined a solution already: Make a mem-sharing private variant
>>> of page_{,un}lock(), derived from the PV ones (but with pieces
>>> dropped you don't want/need).
>>
>> Well, that's what I already did here in this patch. No?
> 
> No - you've retained a shared _page_{,un}lock(), whereas my
> suggestion was to have a completely independent pair of
> functions in mem_sharing.c. The only thing needed by both PV
> and HVM would then be the PGT_locked flag.

But it wasn't obvious to me how the implementations of the actual lock
function would be be different.  And there's no point in having two
identical implementations; in fact, it would be harmful.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Tamas K Lengyel]

On Tue, Apr 30, 2019 at 8:43 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 4/30/19 9:44 AM, Jan Beulich wrote:
> >>>> On 30.04.19 at 10:28, <tamas@tklengyel.com> wrote:
> >> On Tue, Apr 30, 2019 at 1:15 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>>
> >>>>>> On 29.04.19 at 18:35, <tamas@tklengyel.com> wrote:
> >>>> On Mon, Apr 29, 2019 at 9:18 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>>>>>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> >>>>>> --- a/xen/arch/x86/mm.c
> >>>>>> +++ b/xen/arch/x86/mm.c
> >>>>>> @@ -2030,12 +2030,11 @@ static inline bool
> >> current_locked_page_ne_check(struct page_info *page) {
> >>>>>>  #define current_locked_page_ne_check(x) true
> >>>>>>  #endif
> >>>>>>
> >>>>>> -int page_lock(struct page_info *page)
> >>>>>> +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
> >>>>>> +static int _page_lock(struct page_info *page)
> >>>>>
> >>>>> As per above, personally I'm against introducing
> >>>>> page_{,un}lock_memshr(), as that makes the abuse even more
> >>>>> look like proper use. But if this was to be kept this way, may I
> >>>>> ask that you switch int -> bool in the return types at this occasion?
> >>>>
> >>>> Switching them to bool would be fine. Replacing them with something
> >>>> saner is unfortunately out-of-scope at the moment. Unless someone has
> >>>> a specific solution that can be put in place. I don't have one.
> >>>
> >>> I've outlined a solution already: Make a mem-sharing private variant
> >>> of page_{,un}lock(), derived from the PV ones (but with pieces
> >>> dropped you don't want/need).
> >>
> >> Well, that's what I already did here in this patch. No?
> >
> > No - you've retained a shared _page_{,un}lock(), whereas my
> > suggestion was to have a completely independent pair of
> > functions in mem_sharing.c. The only thing needed by both PV
> > and HVM would then be the PGT_locked flag.
>
> But it wasn't obvious to me how the implementations of the actual lock
> function would be be different.  And there's no point in having two
> identical implementations; in fact, it would be harmful.

I also think it's wasteful and an invitation for future breakage. But
right now I just want the functions working without them intentionally
crashing the hypervisor under me - which is the case right now.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Tamas K Lengyel]

On Tue, Apr 30, 2019 at 8:43 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 4/30/19 9:44 AM, Jan Beulich wrote:
> >>>> On 30.04.19 at 10:28, <tamas@tklengyel.com> wrote:
> >> On Tue, Apr 30, 2019 at 1:15 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>>
> >>>>>> On 29.04.19 at 18:35, <tamas@tklengyel.com> wrote:
> >>>> On Mon, Apr 29, 2019 at 9:18 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>>>>>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> >>>>>> --- a/xen/arch/x86/mm.c
> >>>>>> +++ b/xen/arch/x86/mm.c
> >>>>>> @@ -2030,12 +2030,11 @@ static inline bool
> >> current_locked_page_ne_check(struct page_info *page) {
> >>>>>>  #define current_locked_page_ne_check(x) true
> >>>>>>  #endif
> >>>>>>
> >>>>>> -int page_lock(struct page_info *page)
> >>>>>> +#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
> >>>>>> +static int _page_lock(struct page_info *page)
> >>>>>
> >>>>> As per above, personally I'm against introducing
> >>>>> page_{,un}lock_memshr(), as that makes the abuse even more
> >>>>> look like proper use. But if this was to be kept this way, may I
> >>>>> ask that you switch int -> bool in the return types at this occasion?
> >>>>
> >>>> Switching them to bool would be fine. Replacing them with something
> >>>> saner is unfortunately out-of-scope at the moment. Unless someone has
> >>>> a specific solution that can be put in place. I don't have one.
> >>>
> >>> I've outlined a solution already: Make a mem-sharing private variant
> >>> of page_{,un}lock(), derived from the PV ones (but with pieces
> >>> dropped you don't want/need).
> >>
> >> Well, that's what I already did here in this patch. No?
> >
> > No - you've retained a shared _page_{,un}lock(), whereas my
> > suggestion was to have a completely independent pair of
> > functions in mem_sharing.c. The only thing needed by both PV
> > and HVM would then be the PGT_locked flag.
>
> But it wasn't obvious to me how the implementations of the actual lock
> function would be be different.  And there's no point in having two
> identical implementations; in fact, it would be harmful.

I also think it's wasteful and an invitation for future breakage. But
right now I just want the functions working without them intentionally
crashing the hypervisor under me - which is the case right now.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Jan Beulich]

>>> On 30.04.19 at 16:43, <george.dunlap@citrix.com> wrote:
> On 4/30/19 9:44 AM, Jan Beulich wrote:
>>>>> On 30.04.19 at 10:28, <tamas@tklengyel.com> wrote:
>>> On Tue, Apr 30, 2019 at 1:15 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>> I've outlined a solution already: Make a mem-sharing private variant
>>>> of page_{,un}lock(), derived from the PV ones (but with pieces
>>>> dropped you don't want/need).
>>>
>>> Well, that's what I already did here in this patch. No?
>> 
>> No - you've retained a shared _page_{,un}lock(), whereas my
>> suggestion was to have a completely independent pair of
>> functions in mem_sharing.c. The only thing needed by both PV
>> and HVM would then be the PGT_locked flag.
> 
> But it wasn't obvious to me how the implementations of the actual lock
> function would be be different.  And there's no point in having two
> identical implementations; in fact, it would be harmful.

The main difference would be the one that Tamas is after - not
doing the checking that we do for PV. Whether other bits could
be dropped for a mem-sharing special variant I don't know (yet).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Jan Beulich]

>>> On 30.04.19 at 16:43, <george.dunlap@citrix.com> wrote:
> On 4/30/19 9:44 AM, Jan Beulich wrote:
>>>>> On 30.04.19 at 10:28, <tamas@tklengyel.com> wrote:
>>> On Tue, Apr 30, 2019 at 1:15 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>> I've outlined a solution already: Make a mem-sharing private variant
>>>> of page_{,un}lock(), derived from the PV ones (but with pieces
>>>> dropped you don't want/need).
>>>
>>> Well, that's what I already did here in this patch. No?
>> 
>> No - you've retained a shared _page_{,un}lock(), whereas my
>> suggestion was to have a completely independent pair of
>> functions in mem_sharing.c. The only thing needed by both PV
>> and HVM would then be the PGT_locked flag.
> 
> But it wasn't obvious to me how the implementations of the actual lock
> function would be be different.  And there's no point in having two
> identical implementations; in fact, it would be harmful.

The main difference would be the one that Tamas is after - not
doing the checking that we do for PV. Whether other bits could
be dropped for a mem-sharing special variant I don't know (yet).

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[George Dunlap]

On 4/30/19 4:06 PM, Jan Beulich wrote:
>>>> On 30.04.19 at 16:43, <george.dunlap@citrix.com> wrote:
>> On 4/30/19 9:44 AM, Jan Beulich wrote:
>>>>>> On 30.04.19 at 10:28, <tamas@tklengyel.com> wrote:
>>>> On Tue, Apr 30, 2019 at 1:15 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>>> I've outlined a solution already: Make a mem-sharing private variant
>>>>> of page_{,un}lock(), derived from the PV ones (but with pieces
>>>>> dropped you don't want/need).
>>>>
>>>> Well, that's what I already did here in this patch. No?
>>>
>>> No - you've retained a shared _page_{,un}lock(), whereas my
>>> suggestion was to have a completely independent pair of
>>> functions in mem_sharing.c. The only thing needed by both PV
>>> and HVM would then be the PGT_locked flag.
>>
>> But it wasn't obvious to me how the implementations of the actual lock
>> function would be be different.  And there's no point in having two
>> identical implementations; in fact, it would be harmful.
> 
> The main difference would be the one that Tamas is after - not
> doing the checking that we do for PV. Whether other bits could
> be dropped for a mem-sharing special variant I don't know (yet).

The "checking" being that the type count doesn't go to 0?

It's not just page_lock() that does that checking; it's also
_put_page_type().  We can't really change one but leave the other alone.

The approach I'm exploring now is to have _put_page_type() only spin on
the last page reference if the type <= PGT_l4_page_table (i.e., if
_put_final_page_type() would be called for this type).

Thoughts?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[George Dunlap]

On 4/30/19 4:06 PM, Jan Beulich wrote:
>>>> On 30.04.19 at 16:43, <george.dunlap@citrix.com> wrote:
>> On 4/30/19 9:44 AM, Jan Beulich wrote:
>>>>>> On 30.04.19 at 10:28, <tamas@tklengyel.com> wrote:
>>>> On Tue, Apr 30, 2019 at 1:15 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>>> I've outlined a solution already: Make a mem-sharing private variant
>>>>> of page_{,un}lock(), derived from the PV ones (but with pieces
>>>>> dropped you don't want/need).
>>>>
>>>> Well, that's what I already did here in this patch. No?
>>>
>>> No - you've retained a shared _page_{,un}lock(), whereas my
>>> suggestion was to have a completely independent pair of
>>> functions in mem_sharing.c. The only thing needed by both PV
>>> and HVM would then be the PGT_locked flag.
>>
>> But it wasn't obvious to me how the implementations of the actual lock
>> function would be be different.  And there's no point in having two
>> identical implementations; in fact, it would be harmful.
> 
> The main difference would be the one that Tamas is after - not
> doing the checking that we do for PV. Whether other bits could
> be dropped for a mem-sharing special variant I don't know (yet).

The "checking" being that the type count doesn't go to 0?

It's not just page_lock() that does that checking; it's also
_put_page_type().  We can't really change one but leave the other alone.

The approach I'm exploring now is to have _put_page_type() only spin on
the last page reference if the type <= PGT_l4_page_table (i.e., if
_put_final_page_type() would be called for this type).

Thoughts?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Jan Beulich]

>>> On 30.04.19 at 18:03, <george.dunlap@citrix.com> wrote:
> On 4/30/19 4:06 PM, Jan Beulich wrote:
>>>>> On 30.04.19 at 16:43, <george.dunlap@citrix.com> wrote:
>>> On 4/30/19 9:44 AM, Jan Beulich wrote:
>>>>>>> On 30.04.19 at 10:28, <tamas@tklengyel.com> wrote:
>>>>> On Tue, Apr 30, 2019 at 1:15 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> I've outlined a solution already: Make a mem-sharing private variant
>>>>>> of page_{,un}lock(), derived from the PV ones (but with pieces
>>>>>> dropped you don't want/need).
>>>>>
>>>>> Well, that's what I already did here in this patch. No?
>>>>
>>>> No - you've retained a shared _page_{,un}lock(), whereas my
>>>> suggestion was to have a completely independent pair of
>>>> functions in mem_sharing.c. The only thing needed by both PV
>>>> and HVM would then be the PGT_locked flag.
>>>
>>> But it wasn't obvious to me how the implementations of the actual lock
>>> function would be be different.  And there's no point in having two
>>> identical implementations; in fact, it would be harmful.
>> 
>> The main difference would be the one that Tamas is after - not
>> doing the checking that we do for PV. Whether other bits could
>> be dropped for a mem-sharing special variant I don't know (yet).
> 
> The "checking" being that the type count doesn't go to 0?
> 
> It's not just page_lock() that does that checking; it's also
> _put_page_type().  We can't really change one but leave the other alone.

No, I mean the extra debug checking (current_locked_page_*()).
See his patch as to what he keeps for mem-sharing, and what he
drops.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Jan Beulich]

>>> On 30.04.19 at 18:03, <george.dunlap@citrix.com> wrote:
> On 4/30/19 4:06 PM, Jan Beulich wrote:
>>>>> On 30.04.19 at 16:43, <george.dunlap@citrix.com> wrote:
>>> On 4/30/19 9:44 AM, Jan Beulich wrote:
>>>>>>> On 30.04.19 at 10:28, <tamas@tklengyel.com> wrote:
>>>>> On Tue, Apr 30, 2019 at 1:15 AM Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> I've outlined a solution already: Make a mem-sharing private variant
>>>>>> of page_{,un}lock(), derived from the PV ones (but with pieces
>>>>>> dropped you don't want/need).
>>>>>
>>>>> Well, that's what I already did here in this patch. No?
>>>>
>>>> No - you've retained a shared _page_{,un}lock(), whereas my
>>>> suggestion was to have a completely independent pair of
>>>> functions in mem_sharing.c. The only thing needed by both PV
>>>> and HVM would then be the PGT_locked flag.
>>>
>>> But it wasn't obvious to me how the implementations of the actual lock
>>> function would be be different.  And there's no point in having two
>>> identical implementations; in fact, it would be harmful.
>> 
>> The main difference would be the one that Tamas is after - not
>> doing the checking that we do for PV. Whether other bits could
>> be dropped for a mem-sharing special variant I don't know (yet).
> 
> The "checking" being that the type count doesn't go to 0?
> 
> It's not just page_lock() that does that checking; it's also
> _put_page_type().  We can't really change one but leave the other alone.

No, I mean the extra debug checking (current_locked_page_*()).
See his patch as to what he keeps for mem-sharing, and what he
drops.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[George Dunlap]

On 4/29/19 4:18 PM, Jan Beulich wrote:
>>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
>> Patch cf4b30dca0a "Add debug code to detect illegal page_lock and put_page_type
>> ordering" added extra sanity checking to page_lock/page_unlock for debug builds
>> with the assumption that no hypervisor path ever locks two pages at once.
>>
>> This assumption doesn't hold during memory sharing so we introduce separate
>> functions, page_lock_memshr and page_unlock_memshr, to be used exclusively
>> in the memory sharing subsystem.
> 
> Completely bypassing these checks looks undesirable to me, to
> be honest. Otoh as discussed mem-sharing is abusing the lock
> anyway.

I'm not sure what you mean by "abusing"; would it be any different if
page_struct() had a spinlock element called "page_lock", that was only
used by PV guests?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[George Dunlap]

On 4/29/19 4:18 PM, Jan Beulich wrote:
>>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
>> Patch cf4b30dca0a "Add debug code to detect illegal page_lock and put_page_type
>> ordering" added extra sanity checking to page_lock/page_unlock for debug builds
>> with the assumption that no hypervisor path ever locks two pages at once.
>>
>> This assumption doesn't hold during memory sharing so we introduce separate
>> functions, page_lock_memshr and page_unlock_memshr, to be used exclusively
>> in the memory sharing subsystem.
> 
> Completely bypassing these checks looks undesirable to me, to
> be honest. Otoh as discussed mem-sharing is abusing the lock
> anyway.

I'm not sure what you mean by "abusing"; would it be any different if
page_struct() had a spinlock element called "page_lock", that was only
used by PV guests?

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Jan Beulich]

>>> On 29.04.19 at 18:42, <george.dunlap@citrix.com> wrote:
> On 4/29/19 4:18 PM, Jan Beulich wrote:
>>>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
>>> Patch cf4b30dca0a "Add debug code to detect illegal page_lock and 
> put_page_type
>>> ordering" added extra sanity checking to page_lock/page_unlock for debug 
> builds
>>> with the assumption that no hypervisor path ever locks two pages at once.
>>>
>>> This assumption doesn't hold during memory sharing so we introduce separate
>>> functions, page_lock_memshr and page_unlock_memshr, to be used exclusively
>>> in the memory sharing subsystem.
>> 
>> Completely bypassing these checks looks undesirable to me, to
>> be honest. Otoh as discussed mem-sharing is abusing the lock
>> anyway.
> 
> I'm not sure what you mean by "abusing"; would it be any different if
> page_struct() had a spinlock element called "page_lock", that was only
> used by PV guests?

No, it wouldn't. By "abusing" I mean it is using something for HVM
which is meant to be used for PV only. It is mem-sharing's use alone
which prevents page_{,un}lock() to be put inside #ifdef CONFIG_PV.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 2/4] x86/mem_sharing: introduce and use page_lock_memshr instead of page_lock

[Jan Beulich]

>>> On 29.04.19 at 18:42, <george.dunlap@citrix.com> wrote:
> On 4/29/19 4:18 PM, Jan Beulich wrote:
>>>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
>>> Patch cf4b30dca0a "Add debug code to detect illegal page_lock and 
> put_page_type
>>> ordering" added extra sanity checking to page_lock/page_unlock for debug 
> builds
>>> with the assumption that no hypervisor path ever locks two pages at once.
>>>
>>> This assumption doesn't hold during memory sharing so we introduce separate
>>> functions, page_lock_memshr and page_unlock_memshr, to be used exclusively
>>> in the memory sharing subsystem.
>> 
>> Completely bypassing these checks looks undesirable to me, to
>> be honest. Otoh as discussed mem-sharing is abusing the lock
>> anyway.
> 
> I'm not sure what you mean by "abusing"; would it be any different if
> page_struct() had a spinlock element called "page_lock", that was only
> used by PV guests?

No, it wouldn't. By "abusing" I mean it is using something for HVM
which is meant to be used for PV only. It is mem-sharing's use alone
which prevents page_{,un}lock() to be put inside #ifdef CONFIG_PV.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 3/4] x86/mem_sharing: enable mem_share audit mode only in debug builds

[Tamas K Lengyel]

Improves performance for release builds.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
---
 xen/include/asm-x86/mem_sharing.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/include/asm-x86/mem_sharing.h b/xen/include/asm-x86/mem_sharing.h
index 0e77b7d935..52ea91efa0 100644
--- a/xen/include/asm-x86/mem_sharing.h
+++ b/xen/include/asm-x86/mem_sharing.h
@@ -25,7 +25,9 @@
 #include <public/memory.h>
 
 /* Auditing of memory sharing code? */
+#ifndef NDEBUG
 #define MEM_SHARING_AUDIT 1
+#endif
 
 typedef uint64_t shr_handle_t; 
 
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 3/4] x86/mem_sharing: enable mem_share audit mode only in debug builds

[Tamas K Lengyel]

Improves performance for release builds.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
---
 xen/include/asm-x86/mem_sharing.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/include/asm-x86/mem_sharing.h b/xen/include/asm-x86/mem_sharing.h
index 0e77b7d935..52ea91efa0 100644
--- a/xen/include/asm-x86/mem_sharing.h
+++ b/xen/include/asm-x86/mem_sharing.h
@@ -25,7 +25,9 @@
 #include <public/memory.h>
 
 /* Auditing of memory sharing code? */
+#ifndef NDEBUG
 #define MEM_SHARING_AUDIT 1
+#endif
 
 typedef uint64_t shr_handle_t; 
 
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 3/4] x86/mem_sharing: enable mem_share audit mode only in debug builds

[Jan Beulich]

>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> --- a/xen/include/asm-x86/mem_sharing.h
> +++ b/xen/include/asm-x86/mem_sharing.h
> @@ -25,7 +25,9 @@
>  #include <public/memory.h>
>  
>  /* Auditing of memory sharing code? */
> +#ifndef NDEBUG
>  #define MEM_SHARING_AUDIT 1
> +#endif

Since consumers use #if (not #ifdef), I think an #else would
be on order here, even if (I think) not strictly necessary with
gcc.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 3/4] x86/mem_sharing: enable mem_share audit mode only in debug builds

[Jan Beulich]

>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> --- a/xen/include/asm-x86/mem_sharing.h
> +++ b/xen/include/asm-x86/mem_sharing.h
> @@ -25,7 +25,9 @@
>  #include <public/memory.h>
>  
>  /* Auditing of memory sharing code? */
> +#ifndef NDEBUG
>  #define MEM_SHARING_AUDIT 1
> +#endif

Since consumers use #if (not #ifdef), I think an #else would
be on order here, even if (I think) not strictly necessary with
gcc.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 3/4] x86/mem_sharing: enable mem_share audit mode only in debug builds

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 8:57 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> > --- a/xen/include/asm-x86/mem_sharing.h
> > +++ b/xen/include/asm-x86/mem_sharing.h
> > @@ -25,7 +25,9 @@
> >  #include <public/memory.h>
> >
> >  /* Auditing of memory sharing code? */
> > +#ifndef NDEBUG
> >  #define MEM_SHARING_AUDIT 1
> > +#endif
>
> Since consumers use #if (not #ifdef), I think an #else would
> be on order here, even if (I think) not strictly necessary with
> gcc.

Sure.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 3/4] x86/mem_sharing: enable mem_share audit mode only in debug builds

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 8:57 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> > --- a/xen/include/asm-x86/mem_sharing.h
> > +++ b/xen/include/asm-x86/mem_sharing.h
> > @@ -25,7 +25,9 @@
> >  #include <public/memory.h>
> >
> >  /* Auditing of memory sharing code? */
> > +#ifndef NDEBUG
> >  #define MEM_SHARING_AUDIT 1
> > +#endif
>
> Since consumers use #if (not #ifdef), I think an #else would
> be on order here, even if (I think) not strictly necessary with
> gcc.

Sure.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v3 4/4] x86/mem_sharing: compile mem_sharing subsystem only when kconfig is enabled

[Tamas K Lengyel]

Disable it by default as it is only an experimental subsystem.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/arch/x86/Kconfig              |  8 +++++++-
 xen/arch/x86/domctl.c             |  2 ++
 xen/arch/x86/mm.c                 |  4 ++--
 xen/arch/x86/mm/Makefile          |  2 +-
 xen/arch/x86/x86_64/compat/mm.c   |  2 ++
 xen/arch/x86/x86_64/mm.c          |  2 ++
 xen/common/Kconfig                |  3 ---
 xen/common/domain.c               |  2 +-
 xen/common/grant_table.c          |  2 +-
 xen/common/memory.c               |  2 +-
 xen/common/vm_event.c             |  6 +++---
 xen/include/asm-x86/mem_sharing.h | 31 +++++++++++++++++++++++++++++++
 xen/include/asm-x86/mm.h          |  3 +++
 xen/include/xen/sched.h           |  2 +-
 xen/include/xsm/dummy.h           |  2 +-
 xen/include/xsm/xsm.h             |  4 ++--
 xen/xsm/dummy.c                   |  2 +-
 xen/xsm/flask/hooks.c             |  4 ++--
 18 files changed, 63 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 4b8b07b549..af7c25543f 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -17,7 +17,6 @@ config X86
 	select HAS_KEXEC
 	select MEM_ACCESS_ALWAYS_ON
 	select HAS_MEM_PAGING
-	select HAS_MEM_SHARING
 	select HAS_NS16550
 	select HAS_PASSTHROUGH
 	select HAS_PCI
@@ -198,6 +197,13 @@ config PV_SHIM_EXCLUSIVE
 	  firmware, and will not function correctly in other scenarios.
 
 	  If unsure, say N.
+
+config MEM_SHARING
+    bool
+    default n
+    depends on HVM
+    prompt "Xen memory sharing support" if EXPERT = "y"
+
 endmenu
 
 source "common/Kconfig"
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 9bf2d0820f..bc9e024ccc 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1231,9 +1231,11 @@ long arch_do_domctl(
         break;
     }
 
+#ifdef CONFIG_MEM_SHARING
     case XEN_DOMCTL_mem_sharing_op:
         ret = mem_sharing_domctl(d, &domctl->u.mem_sharing_op);
         break;
+#endif
 
 #if P2M_AUDIT && defined(CONFIG_HVM)
     case XEN_DOMCTL_audit_p2m:
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index c2c92a96ac..c3b0f3115c 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2030,7 +2030,7 @@ static inline bool current_locked_page_ne_check(struct page_info *page) {
 #define current_locked_page_ne_check(x) true
 #endif
 
-#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
+#if defined(CONFIG_PV) || defined(CONFIG_MEM_SHARING)
 static int _page_lock(struct page_info *page)
 {
     unsigned long x, nx;
@@ -2063,7 +2063,7 @@ static void _page_unlock(struct page_info *page)
 }
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 int page_lock_memshr(struct page_info *page)
 {
     return _page_lock(page);
diff --git a/xen/arch/x86/mm/Makefile b/xen/arch/x86/mm/Makefile
index 5a17646f98..5010a29d6c 100644
--- a/xen/arch/x86/mm/Makefile
+++ b/xen/arch/x86/mm/Makefile
@@ -6,7 +6,7 @@ obj-$(CONFIG_HVM) += guest_walk_2.o guest_walk_3.o guest_walk_4.o
 obj-$(CONFIG_SHADOW_PAGING) += guest_walk_2.o guest_walk_3.o guest_walk_4.o
 obj-$(CONFIG_MEM_ACCESS) += mem_access.o
 obj-y += mem_paging.o
-obj-y += mem_sharing.o
+obj-$(CONFIG_MEM_SHARING) += mem_sharing.o
 obj-y += p2m.o p2m-pt.o
 obj-$(CONFIG_HVM) += p2m-ept.o p2m-pod.o
 obj-y += paging.o
diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
index 32410ed273..d4c6be3032 100644
--- a/xen/arch/x86/x86_64/compat/mm.c
+++ b/xen/arch/x86/x86_64/compat/mm.c
@@ -152,8 +152,10 @@ int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     case XENMEM_paging_op:
         return mem_paging_memop(guest_handle_cast(arg, xen_mem_paging_op_t));
 
+#ifdef CONFIG_MEM_SHARING
     case XENMEM_sharing_op:
         return mem_sharing_memop(guest_handle_cast(arg, xen_mem_sharing_op_t));
+#endif
 
     default:
         rc = -ENOSYS;
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index d8f558bc3a..51d1d511f2 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -993,8 +993,10 @@ long subarch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     case XENMEM_paging_op:
         return mem_paging_memop(guest_handle_cast(arg, xen_mem_paging_op_t));
 
+#ifdef CONFIG_MEM_SHARING
     case XENMEM_sharing_op:
         return mem_sharing_memop(guest_handle_cast(arg, xen_mem_sharing_op_t));
+#endif
 
     default:
         rc = -ENOSYS;
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index c838506241..80575cac10 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -45,9 +45,6 @@ config MEM_ACCESS
 config HAS_MEM_PAGING
 	bool
 
-config HAS_MEM_SHARING
-	bool
-
 config HAS_PDX
 	bool
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 88bbe984bc..bb072cf93f 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -926,7 +926,7 @@ static void complete_domain_destroy(struct rcu_head *head)
     xfree(d->vm_event_paging);
 #endif
     xfree(d->vm_event_monitor);
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     xfree(d->vm_event_share);
 #endif
 
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 80728ea57d..6c40dccae9 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3760,7 +3760,7 @@ void grant_table_init_vcpu(struct vcpu *v)
     v->maptrack_tail = MAPTRACK_TAIL;
 }
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
                             gfn_t *gfn, uint16_t *status)
 {
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 86567e6117..915f2cee1a 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1676,7 +1676,7 @@ int check_get_page_from_gfn(struct domain *d, gfn_t gfn, bool readonly,
         return -EAGAIN;
     }
 #endif
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     if ( (q & P2M_UNSHARE) && p2m_is_shared(p2mt) )
     {
         if ( page )
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 6e68be47bc..163a671cea 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -544,7 +544,7 @@ static void monitor_notification(struct vcpu *v, unsigned int port)
     vm_event_resume(v->domain, v->domain->vm_event_monitor);
 }
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 /* Registered with Xen-bound event channel for incoming notifications. */
 static void mem_sharing_notification(struct vcpu *v, unsigned int port)
 {
@@ -574,7 +574,7 @@ void vm_event_cleanup(struct domain *d)
         destroy_waitqueue_head(&d->vm_event_monitor->wq);
         (void)vm_event_disable(d, &d->vm_event_monitor);
     }
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     if ( vm_event_check_ring(d->vm_event_share) )
     {
         destroy_waitqueue_head(&d->vm_event_share->wq);
@@ -720,7 +720,7 @@ int vm_event_domctl(struct domain *d, struct xen_domctl_vm_event_op *vec,
     }
     break;
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     case XEN_DOMCTL_VM_EVENT_OP_SHARING:
     {
         rc = -EINVAL;
diff --git a/xen/include/asm-x86/mem_sharing.h b/xen/include/asm-x86/mem_sharing.h
index 52ea91efa0..e0f8792f52 100644
--- a/xen/include/asm-x86/mem_sharing.h
+++ b/xen/include/asm-x86/mem_sharing.h
@@ -24,6 +24,8 @@
 #include <public/domctl.h>
 #include <public/memory.h>
 
+#ifdef CONFIG_MEM_SHARING
+
 /* Auditing of memory sharing code? */
 #ifndef NDEBUG
 #define MEM_SHARING_AUDIT 1
@@ -98,4 +100,33 @@ void mem_sharing_init(void);
  */
 int relinquish_shared_pages(struct domain *d);
 
+#else
+
+static inline unsigned int mem_sharing_get_nr_saved_mfns(void)
+{
+    return 0;
+}
+static inline unsigned int mem_sharing_get_nr_shared_mfns(void)
+{
+    return 0;
+}
+static inline int mem_sharing_unshare_page(struct domain *d,
+                                           unsigned long gfn,
+                                           uint16_t flags)
+{
+    return 0;
+}
+static inline int mem_sharing_notify_enomem(struct domain *d, unsigned long gfn,
+                              bool allow_sleep)
+{
+    return 0;
+}
+static inline int relinquish_shared_pages(struct domain *d)
+{
+    return 0;
+}
+static inline void mem_sharing_init(void) {}
+
+#endif
+
 #endif /* __MEM_SHARING_H__ */
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index ba49eee24d..a3c754cb2f 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -127,6 +127,8 @@ struct page_info
         /* For non-pinnable single-page shadows, a higher entry that points
          * at us. */
         paddr_t up;
+
+#ifdef CONFIG_MEM_SHARING
         /* For shared/sharable pages, we use a doubly-linked list
          * of all the {pfn,domain} pairs that map this page. We also include
          * an opaque handle, which is effectively a version, so that clients
@@ -134,6 +136,7 @@ struct page_info
          * This list is allocated and freed when a page is shared/unshared.
          */
         struct page_sharing_info *sharing;
+#endif
     };
 
     /* Reference count and various PGC_xxx flags and fields. */
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 748bb0f2f9..17cf8785fb 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -462,7 +462,7 @@ struct domain
     /* Various vm_events */
 
     /* Memory sharing support */
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     struct vm_event_domain *vm_event_share;
 #endif
     /* Memory paging support */
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index e628b1c6af..8afdec9fe8 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -603,7 +603,7 @@ static XSM_INLINE int xsm_mem_paging(XSM_DEFAULT_ARG struct domain *d)
 }
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 static XSM_INLINE int xsm_mem_sharing(XSM_DEFAULT_ARG struct domain *d)
 {
     XSM_ASSERT_ACTION(XSM_DM_PRIV);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 8a78d8abd3..8ec6b1a6e8 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -151,7 +151,7 @@ struct xsm_operations {
     int (*mem_paging) (struct domain *d);
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     int (*mem_sharing) (struct domain *d);
 #endif
 
@@ -603,7 +603,7 @@ static inline int xsm_mem_paging (xsm_default_t def, struct domain *d)
 }
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 static inline int xsm_mem_sharing (xsm_default_t def, struct domain *d)
 {
     return xsm_ops->mem_sharing(d);
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 1fe0e746fa..6158dce814 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -129,7 +129,7 @@ void __init xsm_fixup_ops (struct xsm_operations *ops)
     set_to_dummy_if_null(ops, mem_paging);
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     set_to_dummy_if_null(ops, mem_sharing);
 #endif
 
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 3d00c747f6..f5f3b42e6e 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1270,7 +1270,7 @@ static int flask_mem_paging(struct domain *d)
 }
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 static int flask_mem_sharing(struct domain *d)
 {
     return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__MEM_SHARING);
@@ -1838,7 +1838,7 @@ static struct xsm_operations flask_ops = {
     .mem_paging = flask_mem_paging,
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     .mem_sharing = flask_mem_sharing,
 #endif
 
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 4/4] x86/mem_sharing: compile mem_sharing subsystem only when kconfig is enabled

[Tamas K Lengyel]

Disable it by default as it is only an experimental subsystem.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Roger Pau Monne <roger.pau@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/arch/x86/Kconfig              |  8 +++++++-
 xen/arch/x86/domctl.c             |  2 ++
 xen/arch/x86/mm.c                 |  4 ++--
 xen/arch/x86/mm/Makefile          |  2 +-
 xen/arch/x86/x86_64/compat/mm.c   |  2 ++
 xen/arch/x86/x86_64/mm.c          |  2 ++
 xen/common/Kconfig                |  3 ---
 xen/common/domain.c               |  2 +-
 xen/common/grant_table.c          |  2 +-
 xen/common/memory.c               |  2 +-
 xen/common/vm_event.c             |  6 +++---
 xen/include/asm-x86/mem_sharing.h | 31 +++++++++++++++++++++++++++++++
 xen/include/asm-x86/mm.h          |  3 +++
 xen/include/xen/sched.h           |  2 +-
 xen/include/xsm/dummy.h           |  2 +-
 xen/include/xsm/xsm.h             |  4 ++--
 xen/xsm/dummy.c                   |  2 +-
 xen/xsm/flask/hooks.c             |  4 ++--
 18 files changed, 63 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 4b8b07b549..af7c25543f 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -17,7 +17,6 @@ config X86
 	select HAS_KEXEC
 	select MEM_ACCESS_ALWAYS_ON
 	select HAS_MEM_PAGING
-	select HAS_MEM_SHARING
 	select HAS_NS16550
 	select HAS_PASSTHROUGH
 	select HAS_PCI
@@ -198,6 +197,13 @@ config PV_SHIM_EXCLUSIVE
 	  firmware, and will not function correctly in other scenarios.
 
 	  If unsure, say N.
+
+config MEM_SHARING
+    bool
+    default n
+    depends on HVM
+    prompt "Xen memory sharing support" if EXPERT = "y"
+
 endmenu
 
 source "common/Kconfig"
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 9bf2d0820f..bc9e024ccc 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1231,9 +1231,11 @@ long arch_do_domctl(
         break;
     }
 
+#ifdef CONFIG_MEM_SHARING
     case XEN_DOMCTL_mem_sharing_op:
         ret = mem_sharing_domctl(d, &domctl->u.mem_sharing_op);
         break;
+#endif
 
 #if P2M_AUDIT && defined(CONFIG_HVM)
     case XEN_DOMCTL_audit_p2m:
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index c2c92a96ac..c3b0f3115c 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2030,7 +2030,7 @@ static inline bool current_locked_page_ne_check(struct page_info *page) {
 #define current_locked_page_ne_check(x) true
 #endif
 
-#if defined(CONFIG_PV) || defined(CONFIG_HAS_MEM_SHARING)
+#if defined(CONFIG_PV) || defined(CONFIG_MEM_SHARING)
 static int _page_lock(struct page_info *page)
 {
     unsigned long x, nx;
@@ -2063,7 +2063,7 @@ static void _page_unlock(struct page_info *page)
 }
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 int page_lock_memshr(struct page_info *page)
 {
     return _page_lock(page);
diff --git a/xen/arch/x86/mm/Makefile b/xen/arch/x86/mm/Makefile
index 5a17646f98..5010a29d6c 100644
--- a/xen/arch/x86/mm/Makefile
+++ b/xen/arch/x86/mm/Makefile
@@ -6,7 +6,7 @@ obj-$(CONFIG_HVM) += guest_walk_2.o guest_walk_3.o guest_walk_4.o
 obj-$(CONFIG_SHADOW_PAGING) += guest_walk_2.o guest_walk_3.o guest_walk_4.o
 obj-$(CONFIG_MEM_ACCESS) += mem_access.o
 obj-y += mem_paging.o
-obj-y += mem_sharing.o
+obj-$(CONFIG_MEM_SHARING) += mem_sharing.o
 obj-y += p2m.o p2m-pt.o
 obj-$(CONFIG_HVM) += p2m-ept.o p2m-pod.o
 obj-y += paging.o
diff --git a/xen/arch/x86/x86_64/compat/mm.c b/xen/arch/x86/x86_64/compat/mm.c
index 32410ed273..d4c6be3032 100644
--- a/xen/arch/x86/x86_64/compat/mm.c
+++ b/xen/arch/x86/x86_64/compat/mm.c
@@ -152,8 +152,10 @@ int compat_arch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     case XENMEM_paging_op:
         return mem_paging_memop(guest_handle_cast(arg, xen_mem_paging_op_t));
 
+#ifdef CONFIG_MEM_SHARING
     case XENMEM_sharing_op:
         return mem_sharing_memop(guest_handle_cast(arg, xen_mem_sharing_op_t));
+#endif
 
     default:
         rc = -ENOSYS;
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index d8f558bc3a..51d1d511f2 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -993,8 +993,10 @@ long subarch_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     case XENMEM_paging_op:
         return mem_paging_memop(guest_handle_cast(arg, xen_mem_paging_op_t));
 
+#ifdef CONFIG_MEM_SHARING
     case XENMEM_sharing_op:
         return mem_sharing_memop(guest_handle_cast(arg, xen_mem_sharing_op_t));
+#endif
 
     default:
         rc = -ENOSYS;
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index c838506241..80575cac10 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -45,9 +45,6 @@ config MEM_ACCESS
 config HAS_MEM_PAGING
 	bool
 
-config HAS_MEM_SHARING
-	bool
-
 config HAS_PDX
 	bool
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 88bbe984bc..bb072cf93f 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -926,7 +926,7 @@ static void complete_domain_destroy(struct rcu_head *head)
     xfree(d->vm_event_paging);
 #endif
     xfree(d->vm_event_monitor);
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     xfree(d->vm_event_share);
 #endif
 
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 80728ea57d..6c40dccae9 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -3760,7 +3760,7 @@ void grant_table_init_vcpu(struct vcpu *v)
     v->maptrack_tail = MAPTRACK_TAIL;
 }
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 int mem_sharing_gref_to_gfn(struct grant_table *gt, grant_ref_t ref,
                             gfn_t *gfn, uint16_t *status)
 {
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 86567e6117..915f2cee1a 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1676,7 +1676,7 @@ int check_get_page_from_gfn(struct domain *d, gfn_t gfn, bool readonly,
         return -EAGAIN;
     }
 #endif
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     if ( (q & P2M_UNSHARE) && p2m_is_shared(p2mt) )
     {
         if ( page )
diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
index 6e68be47bc..163a671cea 100644
--- a/xen/common/vm_event.c
+++ b/xen/common/vm_event.c
@@ -544,7 +544,7 @@ static void monitor_notification(struct vcpu *v, unsigned int port)
     vm_event_resume(v->domain, v->domain->vm_event_monitor);
 }
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 /* Registered with Xen-bound event channel for incoming notifications. */
 static void mem_sharing_notification(struct vcpu *v, unsigned int port)
 {
@@ -574,7 +574,7 @@ void vm_event_cleanup(struct domain *d)
         destroy_waitqueue_head(&d->vm_event_monitor->wq);
         (void)vm_event_disable(d, &d->vm_event_monitor);
     }
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     if ( vm_event_check_ring(d->vm_event_share) )
     {
         destroy_waitqueue_head(&d->vm_event_share->wq);
@@ -720,7 +720,7 @@ int vm_event_domctl(struct domain *d, struct xen_domctl_vm_event_op *vec,
     }
     break;
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     case XEN_DOMCTL_VM_EVENT_OP_SHARING:
     {
         rc = -EINVAL;
diff --git a/xen/include/asm-x86/mem_sharing.h b/xen/include/asm-x86/mem_sharing.h
index 52ea91efa0..e0f8792f52 100644
--- a/xen/include/asm-x86/mem_sharing.h
+++ b/xen/include/asm-x86/mem_sharing.h
@@ -24,6 +24,8 @@
 #include <public/domctl.h>
 #include <public/memory.h>
 
+#ifdef CONFIG_MEM_SHARING
+
 /* Auditing of memory sharing code? */
 #ifndef NDEBUG
 #define MEM_SHARING_AUDIT 1
@@ -98,4 +100,33 @@ void mem_sharing_init(void);
  */
 int relinquish_shared_pages(struct domain *d);
 
+#else
+
+static inline unsigned int mem_sharing_get_nr_saved_mfns(void)
+{
+    return 0;
+}
+static inline unsigned int mem_sharing_get_nr_shared_mfns(void)
+{
+    return 0;
+}
+static inline int mem_sharing_unshare_page(struct domain *d,
+                                           unsigned long gfn,
+                                           uint16_t flags)
+{
+    return 0;
+}
+static inline int mem_sharing_notify_enomem(struct domain *d, unsigned long gfn,
+                              bool allow_sleep)
+{
+    return 0;
+}
+static inline int relinquish_shared_pages(struct domain *d)
+{
+    return 0;
+}
+static inline void mem_sharing_init(void) {}
+
+#endif
+
 #endif /* __MEM_SHARING_H__ */
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index ba49eee24d..a3c754cb2f 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -127,6 +127,8 @@ struct page_info
         /* For non-pinnable single-page shadows, a higher entry that points
          * at us. */
         paddr_t up;
+
+#ifdef CONFIG_MEM_SHARING
         /* For shared/sharable pages, we use a doubly-linked list
          * of all the {pfn,domain} pairs that map this page. We also include
          * an opaque handle, which is effectively a version, so that clients
@@ -134,6 +136,7 @@ struct page_info
          * This list is allocated and freed when a page is shared/unshared.
          */
         struct page_sharing_info *sharing;
+#endif
     };
 
     /* Reference count and various PGC_xxx flags and fields. */
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 748bb0f2f9..17cf8785fb 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -462,7 +462,7 @@ struct domain
     /* Various vm_events */
 
     /* Memory sharing support */
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     struct vm_event_domain *vm_event_share;
 #endif
     /* Memory paging support */
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index e628b1c6af..8afdec9fe8 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -603,7 +603,7 @@ static XSM_INLINE int xsm_mem_paging(XSM_DEFAULT_ARG struct domain *d)
 }
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 static XSM_INLINE int xsm_mem_sharing(XSM_DEFAULT_ARG struct domain *d)
 {
     XSM_ASSERT_ACTION(XSM_DM_PRIV);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 8a78d8abd3..8ec6b1a6e8 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -151,7 +151,7 @@ struct xsm_operations {
     int (*mem_paging) (struct domain *d);
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     int (*mem_sharing) (struct domain *d);
 #endif
 
@@ -603,7 +603,7 @@ static inline int xsm_mem_paging (xsm_default_t def, struct domain *d)
 }
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 static inline int xsm_mem_sharing (xsm_default_t def, struct domain *d)
 {
     return xsm_ops->mem_sharing(d);
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 1fe0e746fa..6158dce814 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -129,7 +129,7 @@ void __init xsm_fixup_ops (struct xsm_operations *ops)
     set_to_dummy_if_null(ops, mem_paging);
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     set_to_dummy_if_null(ops, mem_sharing);
 #endif
 
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 3d00c747f6..f5f3b42e6e 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1270,7 +1270,7 @@ static int flask_mem_paging(struct domain *d)
 }
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
 static int flask_mem_sharing(struct domain *d)
 {
     return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__MEM_SHARING);
@@ -1838,7 +1838,7 @@ static struct xsm_operations flask_ops = {
     .mem_paging = flask_mem_paging,
 #endif
 
-#ifdef CONFIG_HAS_MEM_SHARING
+#ifdef CONFIG_MEM_SHARING
     .mem_sharing = flask_mem_sharing,
 #endif
 
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 4/4] x86/mem_sharing: compile mem_sharing subsystem only when kconfig is enabled

[Jan Beulich]

>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -17,7 +17,6 @@ config X86
>  	select HAS_KEXEC
>  	select MEM_ACCESS_ALWAYS_ON
>  	select HAS_MEM_PAGING
> -	select HAS_MEM_SHARING
>  	select HAS_NS16550
>  	select HAS_PASSTHROUGH
>  	select HAS_PCI
> @@ -198,6 +197,13 @@ config PV_SHIM_EXCLUSIVE
>  	  firmware, and will not function correctly in other scenarios.
>  
>  	  If unsure, say N.
> +
> +config MEM_SHARING
> +    bool
> +    default n
> +    depends on HVM
> +    prompt "Xen memory sharing support" if EXPERT = "y"
> +

As per all the context above, please use proper (tab) indentation.
Also please omit the pointless "default n". And then the "bool"
and "prompt ..." can (and hence should) be combined into a
single line.

I also wonder whether this shouldn't be in common/Kconfig, but
then again it can be moved there if Arm would ever gain
mem-sharing support.

> @@ -98,4 +100,33 @@ void mem_sharing_init(void);
>   */
>  int relinquish_shared_pages(struct domain *d);
>  
> +#else
> +
> +static inline unsigned int mem_sharing_get_nr_saved_mfns(void)
> +{
> +    return 0;
> +}
> +static inline unsigned int mem_sharing_get_nr_shared_mfns(void)
> +{
> +    return 0;
> +}

I follow you for these.

> +static inline int mem_sharing_unshare_page(struct domain *d,
> +                                           unsigned long gfn,
> +                                           uint16_t flags)
> +{
> +    return 0;
> +}

But shouldn't this one (just as an example, there may be more
below) return -EOPNOTSUPP?

And in general - if these inline functions are needed, shouldn't
all of them (or at least some) gain ASSERT_UNREACHABLE(), as we
do elsewhere?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 4/4] x86/mem_sharing: compile mem_sharing subsystem only when kconfig is enabled

[Jan Beulich]

>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -17,7 +17,6 @@ config X86
>  	select HAS_KEXEC
>  	select MEM_ACCESS_ALWAYS_ON
>  	select HAS_MEM_PAGING
> -	select HAS_MEM_SHARING
>  	select HAS_NS16550
>  	select HAS_PASSTHROUGH
>  	select HAS_PCI
> @@ -198,6 +197,13 @@ config PV_SHIM_EXCLUSIVE
>  	  firmware, and will not function correctly in other scenarios.
>  
>  	  If unsure, say N.
> +
> +config MEM_SHARING
> +    bool
> +    default n
> +    depends on HVM
> +    prompt "Xen memory sharing support" if EXPERT = "y"
> +

As per all the context above, please use proper (tab) indentation.
Also please omit the pointless "default n". And then the "bool"
and "prompt ..." can (and hence should) be combined into a
single line.

I also wonder whether this shouldn't be in common/Kconfig, but
then again it can be moved there if Arm would ever gain
mem-sharing support.

> @@ -98,4 +100,33 @@ void mem_sharing_init(void);
>   */
>  int relinquish_shared_pages(struct domain *d);
>  
> +#else
> +
> +static inline unsigned int mem_sharing_get_nr_saved_mfns(void)
> +{
> +    return 0;
> +}
> +static inline unsigned int mem_sharing_get_nr_shared_mfns(void)
> +{
> +    return 0;
> +}

I follow you for these.

> +static inline int mem_sharing_unshare_page(struct domain *d,
> +                                           unsigned long gfn,
> +                                           uint16_t flags)
> +{
> +    return 0;
> +}

But shouldn't this one (just as an example, there may be more
below) return -EOPNOTSUPP?

And in general - if these inline functions are needed, shouldn't
all of them (or at least some) gain ASSERT_UNREACHABLE(), as we
do elsewhere?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 4/4] x86/mem_sharing: compile mem_sharing subsystem only when kconfig is enabled

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 9:32 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> > --- a/xen/arch/x86/Kconfig
> > +++ b/xen/arch/x86/Kconfig
> > @@ -17,7 +17,6 @@ config X86
> >       select HAS_KEXEC
> >       select MEM_ACCESS_ALWAYS_ON
> >       select HAS_MEM_PAGING
> > -     select HAS_MEM_SHARING
> >       select HAS_NS16550
> >       select HAS_PASSTHROUGH
> >       select HAS_PCI
> > @@ -198,6 +197,13 @@ config PV_SHIM_EXCLUSIVE
> >         firmware, and will not function correctly in other scenarios.
> >
> >         If unsure, say N.
> > +
> > +config MEM_SHARING
> > +    bool
> > +    default n
> > +    depends on HVM
> > +    prompt "Xen memory sharing support" if EXPERT = "y"
> > +
>
> As per all the context above, please use proper (tab) indentation.
> Also please omit the pointless "default n". And then the "bool"
> and "prompt ..." can (and hence should) be combined into a
> single line.

Sure.

>
> I also wonder whether this shouldn't be in common/Kconfig, but
> then again it can be moved there if Arm would ever gain
> mem-sharing support.

It is currently only supported for x86 HVM guests. There are no plans
for adding ARM support. If and when that happens, it could be moved to
common. I don't expect that will ever happen.

>
> > @@ -98,4 +100,33 @@ void mem_sharing_init(void);
> >   */
> >  int relinquish_shared_pages(struct domain *d);
> >
> > +#else
> > +
> > +static inline unsigned int mem_sharing_get_nr_saved_mfns(void)
> > +{
> > +    return 0;
> > +}
> > +static inline unsigned int mem_sharing_get_nr_shared_mfns(void)
> > +{
> > +    return 0;
> > +}
>
> I follow you for these.
>
> > +static inline int mem_sharing_unshare_page(struct domain *d,
> > +                                           unsigned long gfn,
> > +                                           uint16_t flags)
> > +{
> > +    return 0;
> > +}
>
> But shouldn't this one (just as an example, there may be more
> below) return -EOPNOTSUPP?

It really doesn't matter. No caller ever reaches this function when
mem_sharing is compiled out since it's gated on the page type being
p2m_ram_shared. You can't set that page type when the memop/domctl to
set it is gone.

>
> And in general - if these inline functions are needed, shouldn't
> all of them (or at least some) gain ASSERT_UNREACHABLE(), as we
> do elsewhere?

Yes, since unshare_page is unreachable adding that assert would be
appropriate. It would probably be appropriate for the others too
(except get_nr_saved/shared_mfns).

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 4/4] x86/mem_sharing: compile mem_sharing subsystem only when kconfig is enabled

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 9:32 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> > --- a/xen/arch/x86/Kconfig
> > +++ b/xen/arch/x86/Kconfig
> > @@ -17,7 +17,6 @@ config X86
> >       select HAS_KEXEC
> >       select MEM_ACCESS_ALWAYS_ON
> >       select HAS_MEM_PAGING
> > -     select HAS_MEM_SHARING
> >       select HAS_NS16550
> >       select HAS_PASSTHROUGH
> >       select HAS_PCI
> > @@ -198,6 +197,13 @@ config PV_SHIM_EXCLUSIVE
> >         firmware, and will not function correctly in other scenarios.
> >
> >         If unsure, say N.
> > +
> > +config MEM_SHARING
> > +    bool
> > +    default n
> > +    depends on HVM
> > +    prompt "Xen memory sharing support" if EXPERT = "y"
> > +
>
> As per all the context above, please use proper (tab) indentation.
> Also please omit the pointless "default n". And then the "bool"
> and "prompt ..." can (and hence should) be combined into a
> single line.

Sure.

>
> I also wonder whether this shouldn't be in common/Kconfig, but
> then again it can be moved there if Arm would ever gain
> mem-sharing support.

It is currently only supported for x86 HVM guests. There are no plans
for adding ARM support. If and when that happens, it could be moved to
common. I don't expect that will ever happen.

>
> > @@ -98,4 +100,33 @@ void mem_sharing_init(void);
> >   */
> >  int relinquish_shared_pages(struct domain *d);
> >
> > +#else
> > +
> > +static inline unsigned int mem_sharing_get_nr_saved_mfns(void)
> > +{
> > +    return 0;
> > +}
> > +static inline unsigned int mem_sharing_get_nr_shared_mfns(void)
> > +{
> > +    return 0;
> > +}
>
> I follow you for these.
>
> > +static inline int mem_sharing_unshare_page(struct domain *d,
> > +                                           unsigned long gfn,
> > +                                           uint16_t flags)
> > +{
> > +    return 0;
> > +}
>
> But shouldn't this one (just as an example, there may be more
> below) return -EOPNOTSUPP?

It really doesn't matter. No caller ever reaches this function when
mem_sharing is compiled out since it's gated on the page type being
p2m_ram_shared. You can't set that page type when the memop/domctl to
set it is gone.

>
> And in general - if these inline functions are needed, shouldn't
> all of them (or at least some) gain ASSERT_UNREACHABLE(), as we
> do elsewhere?

Yes, since unshare_page is unreachable adding that assert would be
appropriate. It would probably be appropriate for the others too
(except get_nr_saved/shared_mfns).

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

On 4/26/19 6:21 PM, Tamas K Lengyel wrote:
> Calling _put_page_type while also holding the page_lock
> for that page can cause a deadlock.
> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Roger Pau Monne <roger.pau@citrix.com>
> ---
> v3: simplified patch by keeping the additional references already in-place
> ---
>  xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index dfc279d371..e2f74ac770 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct page_info *page)
>          return -EBUSY;
>      }
>  
> -    /* We can only change the type if count is one */
> -    /* Because we are locking pages individually, we need to drop
> -     * the lock here, while the page is typed. We cannot risk the 
> -     * race of page_unlock and then put_page_type. */
>      expected_type = (PGT_shared_page | PGT_validated | PGT_locked | 2);
>      if ( page->u.inuse.type_info != expected_type )
>      {
> @@ -660,12 +656,12 @@ static int page_make_private(struct domain *d, struct page_info *page)
>          return -EEXIST;
>      }
>  
> -    /* Drop the final typecount */
> -    put_page_and_type(page);
> -
>      /* Now that we've dropped the type, we can unlock */
>      mem_sharing_page_unlock(page);
>  
> +    /* Drop the final typecount */
> +    put_page_and_type(page);
> +
>      /* Change the owner */
>      ASSERT(page_get_owner(page) == dom_cow);
>      page_set_owner(page, d);
> @@ -900,6 +896,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>      p2m_type_t smfn_type, cmfn_type;
>      struct two_gfns tg;
>      struct rmap_iterator ri;
> +    unsigned long put_count = 0;
>  
>      get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
>                   cd, cgfn, &cmfn_type, NULL, &cmfn, 0, &tg);
> @@ -984,7 +981,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>           * Don't change the type of rmap for the client page. */
>          rmap_del(gfn, cpage, 0);
>          rmap_add(gfn, spage);
> -        put_page_and_type(cpage);
> +        put_count++;
>          d = get_domain_by_id(gfn->domain);
>          BUG_ON(!d);
>          BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>      mem_sharing_page_unlock(secondpg);
>      mem_sharing_page_unlock(firstpg);
>  
> +    BUG_ON(!put_count);
> +    while ( put_count-- )
> +        put_page_and_type(cpage);
> +
>      /* Free the client page */
>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>          put_page(cpage);
> @@ -1167,8 +1168,8 @@ int __mem_sharing_unshare_page(struct domain *d,
>      {
>          if ( !last_gfn )
>              mem_sharing_gfn_destroy(page, d, gfn_info);
> -        put_page_and_type(page);
>          mem_sharing_page_unlock(page);
> +        put_page_and_type(page);
>          if ( last_gfn )
>          {
>              if ( !get_page(page, dom_cow) )

...Probably should have mentioned that this needs to be applied after
your other patch. :-)

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

On 4/26/19 6:21 PM, Tamas K Lengyel wrote:
> Calling _put_page_type while also holding the page_lock
> for that page can cause a deadlock.
> 
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Roger Pau Monne <roger.pau@citrix.com>
> ---
> v3: simplified patch by keeping the additional references already in-place
> ---
>  xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index dfc279d371..e2f74ac770 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct page_info *page)
>          return -EBUSY;
>      }
>  
> -    /* We can only change the type if count is one */
> -    /* Because we are locking pages individually, we need to drop
> -     * the lock here, while the page is typed. We cannot risk the 
> -     * race of page_unlock and then put_page_type. */
>      expected_type = (PGT_shared_page | PGT_validated | PGT_locked | 2);
>      if ( page->u.inuse.type_info != expected_type )
>      {
> @@ -660,12 +656,12 @@ static int page_make_private(struct domain *d, struct page_info *page)
>          return -EEXIST;
>      }
>  
> -    /* Drop the final typecount */
> -    put_page_and_type(page);
> -
>      /* Now that we've dropped the type, we can unlock */
>      mem_sharing_page_unlock(page);
>  
> +    /* Drop the final typecount */
> +    put_page_and_type(page);
> +
>      /* Change the owner */
>      ASSERT(page_get_owner(page) == dom_cow);
>      page_set_owner(page, d);
> @@ -900,6 +896,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>      p2m_type_t smfn_type, cmfn_type;
>      struct two_gfns tg;
>      struct rmap_iterator ri;
> +    unsigned long put_count = 0;
>  
>      get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
>                   cd, cgfn, &cmfn_type, NULL, &cmfn, 0, &tg);
> @@ -984,7 +981,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>           * Don't change the type of rmap for the client page. */
>          rmap_del(gfn, cpage, 0);
>          rmap_add(gfn, spage);
> -        put_page_and_type(cpage);
> +        put_count++;
>          d = get_domain_by_id(gfn->domain);
>          BUG_ON(!d);
>          BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>      mem_sharing_page_unlock(secondpg);
>      mem_sharing_page_unlock(firstpg);
>  
> +    BUG_ON(!put_count);
> +    while ( put_count-- )
> +        put_page_and_type(cpage);
> +
>      /* Free the client page */
>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>          put_page(cpage);
> @@ -1167,8 +1168,8 @@ int __mem_sharing_unshare_page(struct domain *d,
>      {
>          if ( !last_gfn )
>              mem_sharing_gfn_destroy(page, d, gfn_info);
> -        put_page_and_type(page);
>          mem_sharing_page_unlock(page);
> +        put_page_and_type(page);
>          if ( last_gfn )
>          {
>              if ( !get_page(page, dom_cow) )

...Probably should have mentioned that this needs to be applied after
your other patch. :-)

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

On 4/29/19 3:32 PM, George Dunlap wrote:
> On 4/26/19 6:21 PM, Tamas K Lengyel wrote:
>> Calling _put_page_type while also holding the page_lock
>> for that page can cause a deadlock.
>>
>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Roger Pau Monne <roger.pau@citrix.com>
>> ---
>> v3: simplified patch by keeping the additional references already in-place
>> ---
>>  xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
>>  1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
>> index dfc279d371..e2f74ac770 100644
>> --- a/xen/arch/x86/mm/mem_sharing.c
>> +++ b/xen/arch/x86/mm/mem_sharing.c
>> @@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct page_info *page)
>>          return -EBUSY;
>>      }
>>  
>> -    /* We can only change the type if count is one */
>> -    /* Because we are locking pages individually, we need to drop
>> -     * the lock here, while the page is typed. We cannot risk the 
>> -     * race of page_unlock and then put_page_type. */
>>      expected_type = (PGT_shared_page | PGT_validated | PGT_locked | 2);
>>      if ( page->u.inuse.type_info != expected_type )
>>      {
>> @@ -660,12 +656,12 @@ static int page_make_private(struct domain *d, struct page_info *page)
>>          return -EEXIST;
>>      }
>>  
>> -    /* Drop the final typecount */
>> -    put_page_and_type(page);
>> -
>>      /* Now that we've dropped the type, we can unlock */
>>      mem_sharing_page_unlock(page);
>>  
>> +    /* Drop the final typecount */
>> +    put_page_and_type(page);
>> +
>>      /* Change the owner */
>>      ASSERT(page_get_owner(page) == dom_cow);
>>      page_set_owner(page, d);
>> @@ -900,6 +896,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>      p2m_type_t smfn_type, cmfn_type;
>>      struct two_gfns tg;
>>      struct rmap_iterator ri;
>> +    unsigned long put_count = 0;
>>  
>>      get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
>>                   cd, cgfn, &cmfn_type, NULL, &cmfn, 0, &tg);
>> @@ -984,7 +981,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>           * Don't change the type of rmap for the client page. */
>>          rmap_del(gfn, cpage, 0);
>>          rmap_add(gfn, spage);
>> -        put_page_and_type(cpage);
>> +        put_count++;
>>          d = get_domain_by_id(gfn->domain);
>>          BUG_ON(!d);
>>          BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
>> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>      mem_sharing_page_unlock(secondpg);
>>      mem_sharing_page_unlock(firstpg);
>>  
>> +    BUG_ON(!put_count);
>> +    while ( put_count-- )
>> +        put_page_and_type(cpage);
>> +
>>      /* Free the client page */
>>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>>          put_page(cpage);
>> @@ -1167,8 +1168,8 @@ int __mem_sharing_unshare_page(struct domain *d,
>>      {
>>          if ( !last_gfn )
>>              mem_sharing_gfn_destroy(page, d, gfn_info);
>> -        put_page_and_type(page);
>>          mem_sharing_page_unlock(page);
>> +        put_page_and_type(page);
>>          if ( last_gfn )
>>          {
>>              if ( !get_page(page, dom_cow) )
> 
> ...Probably should have mentioned that this needs to be applied after
> your other patch. :-)

Hmm -- actually, the base appears to be a non-publicly-available tree
(Andy's private x86-next).

I think series should:
1. Always be posted against a publicly-available tree, and
2. If that tree is not xenbits/xen.git staging, the URL and branch
should be provided.

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

On 4/29/19 3:32 PM, George Dunlap wrote:
> On 4/26/19 6:21 PM, Tamas K Lengyel wrote:
>> Calling _put_page_type while also holding the page_lock
>> for that page can cause a deadlock.
>>
>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Roger Pau Monne <roger.pau@citrix.com>
>> ---
>> v3: simplified patch by keeping the additional references already in-place
>> ---
>>  xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
>>  1 file changed, 10 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
>> index dfc279d371..e2f74ac770 100644
>> --- a/xen/arch/x86/mm/mem_sharing.c
>> +++ b/xen/arch/x86/mm/mem_sharing.c
>> @@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct page_info *page)
>>          return -EBUSY;
>>      }
>>  
>> -    /* We can only change the type if count is one */
>> -    /* Because we are locking pages individually, we need to drop
>> -     * the lock here, while the page is typed. We cannot risk the 
>> -     * race of page_unlock and then put_page_type. */
>>      expected_type = (PGT_shared_page | PGT_validated | PGT_locked | 2);
>>      if ( page->u.inuse.type_info != expected_type )
>>      {
>> @@ -660,12 +656,12 @@ static int page_make_private(struct domain *d, struct page_info *page)
>>          return -EEXIST;
>>      }
>>  
>> -    /* Drop the final typecount */
>> -    put_page_and_type(page);
>> -
>>      /* Now that we've dropped the type, we can unlock */
>>      mem_sharing_page_unlock(page);
>>  
>> +    /* Drop the final typecount */
>> +    put_page_and_type(page);
>> +
>>      /* Change the owner */
>>      ASSERT(page_get_owner(page) == dom_cow);
>>      page_set_owner(page, d);
>> @@ -900,6 +896,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>      p2m_type_t smfn_type, cmfn_type;
>>      struct two_gfns tg;
>>      struct rmap_iterator ri;
>> +    unsigned long put_count = 0;
>>  
>>      get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
>>                   cd, cgfn, &cmfn_type, NULL, &cmfn, 0, &tg);
>> @@ -984,7 +981,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>           * Don't change the type of rmap for the client page. */
>>          rmap_del(gfn, cpage, 0);
>>          rmap_add(gfn, spage);
>> -        put_page_and_type(cpage);
>> +        put_count++;
>>          d = get_domain_by_id(gfn->domain);
>>          BUG_ON(!d);
>>          BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
>> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>      mem_sharing_page_unlock(secondpg);
>>      mem_sharing_page_unlock(firstpg);
>>  
>> +    BUG_ON(!put_count);
>> +    while ( put_count-- )
>> +        put_page_and_type(cpage);
>> +
>>      /* Free the client page */
>>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>>          put_page(cpage);
>> @@ -1167,8 +1168,8 @@ int __mem_sharing_unshare_page(struct domain *d,
>>      {
>>          if ( !last_gfn )
>>              mem_sharing_gfn_destroy(page, d, gfn_info);
>> -        put_page_and_type(page);
>>          mem_sharing_page_unlock(page);
>> +        put_page_and_type(page);
>>          if ( last_gfn )
>>          {
>>              if ( !get_page(page, dom_cow) )
> 
> ...Probably should have mentioned that this needs to be applied after
> your other patch. :-)

Hmm -- actually, the base appears to be a non-publicly-available tree
(Andy's private x86-next).

I think series should:
1. Always be posted against a publicly-available tree, and
2. If that tree is not xenbits/xen.git staging, the URL and branch
should be provided.

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Andrew Cooper]

On 29/04/2019 15:46, George Dunlap wrote:
> On 4/29/19 3:32 PM, George Dunlap wrote:
>> On 4/26/19 6:21 PM, Tamas K Lengyel wrote:
>>> Calling _put_page_type while also holding the page_lock
>>> for that page can cause a deadlock.
>>>
>>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>> Cc: Roger Pau Monne <roger.pau@citrix.com>
>>> ---
>>> v3: simplified patch by keeping the additional references already in-place
>>> ---
>>>  xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
>>>  1 file changed, 10 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
>>> index dfc279d371..e2f74ac770 100644
>>> --- a/xen/arch/x86/mm/mem_sharing.c
>>> +++ b/xen/arch/x86/mm/mem_sharing.c
>>> @@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct page_info *page)
>>>          return -EBUSY;
>>>      }
>>>  
>>> -    /* We can only change the type if count is one */
>>> -    /* Because we are locking pages individually, we need to drop
>>> -     * the lock here, while the page is typed. We cannot risk the 
>>> -     * race of page_unlock and then put_page_type. */
>>>      expected_type = (PGT_shared_page | PGT_validated | PGT_locked | 2);
>>>      if ( page->u.inuse.type_info != expected_type )
>>>      {
>>> @@ -660,12 +656,12 @@ static int page_make_private(struct domain *d, struct page_info *page)
>>>          return -EEXIST;
>>>      }
>>>  
>>> -    /* Drop the final typecount */
>>> -    put_page_and_type(page);
>>> -
>>>      /* Now that we've dropped the type, we can unlock */
>>>      mem_sharing_page_unlock(page);
>>>  
>>> +    /* Drop the final typecount */
>>> +    put_page_and_type(page);
>>> +
>>>      /* Change the owner */
>>>      ASSERT(page_get_owner(page) == dom_cow);
>>>      page_set_owner(page, d);
>>> @@ -900,6 +896,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>      p2m_type_t smfn_type, cmfn_type;
>>>      struct two_gfns tg;
>>>      struct rmap_iterator ri;
>>> +    unsigned long put_count = 0;
>>>  
>>>      get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
>>>                   cd, cgfn, &cmfn_type, NULL, &cmfn, 0, &tg);
>>> @@ -984,7 +981,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>           * Don't change the type of rmap for the client page. */
>>>          rmap_del(gfn, cpage, 0);
>>>          rmap_add(gfn, spage);
>>> -        put_page_and_type(cpage);
>>> +        put_count++;
>>>          d = get_domain_by_id(gfn->domain);
>>>          BUG_ON(!d);
>>>          BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
>>> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>      mem_sharing_page_unlock(secondpg);
>>>      mem_sharing_page_unlock(firstpg);
>>>  
>>> +    BUG_ON(!put_count);
>>> +    while ( put_count-- )
>>> +        put_page_and_type(cpage);
>>> +
>>>      /* Free the client page */
>>>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>>>          put_page(cpage);
>>> @@ -1167,8 +1168,8 @@ int __mem_sharing_unshare_page(struct domain *d,
>>>      {
>>>          if ( !last_gfn )
>>>              mem_sharing_gfn_destroy(page, d, gfn_info);
>>> -        put_page_and_type(page);
>>>          mem_sharing_page_unlock(page);
>>> +        put_page_and_type(page);
>>>          if ( last_gfn )
>>>          {
>>>              if ( !get_page(page, dom_cow) )
>> ...Probably should have mentioned that this needs to be applied after
>> your other patch. :-)
> Hmm -- actually, the base appears to be a non-publicly-available tree
> (Andy's private x86-next).

Its perfectly public

http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/x86-next

and has been in effect during commit freezes for the past several years
worth of releases.

It is fine to post a tree based on something other than xen.git/staging
so long as the cover letter identifies where the series is based.  A lot
of development already occurs in this way on xen-devel.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Andrew Cooper]

On 29/04/2019 15:46, George Dunlap wrote:
> On 4/29/19 3:32 PM, George Dunlap wrote:
>> On 4/26/19 6:21 PM, Tamas K Lengyel wrote:
>>> Calling _put_page_type while also holding the page_lock
>>> for that page can cause a deadlock.
>>>
>>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
>>> Cc: Jan Beulich <jbeulich@suse.com>
>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>> Cc: Roger Pau Monne <roger.pau@citrix.com>
>>> ---
>>> v3: simplified patch by keeping the additional references already in-place
>>> ---
>>>  xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
>>>  1 file changed, 10 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
>>> index dfc279d371..e2f74ac770 100644
>>> --- a/xen/arch/x86/mm/mem_sharing.c
>>> +++ b/xen/arch/x86/mm/mem_sharing.c
>>> @@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct page_info *page)
>>>          return -EBUSY;
>>>      }
>>>  
>>> -    /* We can only change the type if count is one */
>>> -    /* Because we are locking pages individually, we need to drop
>>> -     * the lock here, while the page is typed. We cannot risk the 
>>> -     * race of page_unlock and then put_page_type. */
>>>      expected_type = (PGT_shared_page | PGT_validated | PGT_locked | 2);
>>>      if ( page->u.inuse.type_info != expected_type )
>>>      {
>>> @@ -660,12 +656,12 @@ static int page_make_private(struct domain *d, struct page_info *page)
>>>          return -EEXIST;
>>>      }
>>>  
>>> -    /* Drop the final typecount */
>>> -    put_page_and_type(page);
>>> -
>>>      /* Now that we've dropped the type, we can unlock */
>>>      mem_sharing_page_unlock(page);
>>>  
>>> +    /* Drop the final typecount */
>>> +    put_page_and_type(page);
>>> +
>>>      /* Change the owner */
>>>      ASSERT(page_get_owner(page) == dom_cow);
>>>      page_set_owner(page, d);
>>> @@ -900,6 +896,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>      p2m_type_t smfn_type, cmfn_type;
>>>      struct two_gfns tg;
>>>      struct rmap_iterator ri;
>>> +    unsigned long put_count = 0;
>>>  
>>>      get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
>>>                   cd, cgfn, &cmfn_type, NULL, &cmfn, 0, &tg);
>>> @@ -984,7 +981,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>           * Don't change the type of rmap for the client page. */
>>>          rmap_del(gfn, cpage, 0);
>>>          rmap_add(gfn, spage);
>>> -        put_page_and_type(cpage);
>>> +        put_count++;
>>>          d = get_domain_by_id(gfn->domain);
>>>          BUG_ON(!d);
>>>          BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
>>> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>      mem_sharing_page_unlock(secondpg);
>>>      mem_sharing_page_unlock(firstpg);
>>>  
>>> +    BUG_ON(!put_count);
>>> +    while ( put_count-- )
>>> +        put_page_and_type(cpage);
>>> +
>>>      /* Free the client page */
>>>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>>>          put_page(cpage);
>>> @@ -1167,8 +1168,8 @@ int __mem_sharing_unshare_page(struct domain *d,
>>>      {
>>>          if ( !last_gfn )
>>>              mem_sharing_gfn_destroy(page, d, gfn_info);
>>> -        put_page_and_type(page);
>>>          mem_sharing_page_unlock(page);
>>> +        put_page_and_type(page);
>>>          if ( last_gfn )
>>>          {
>>>              if ( !get_page(page, dom_cow) )
>> ...Probably should have mentioned that this needs to be applied after
>> your other patch. :-)
> Hmm -- actually, the base appears to be a non-publicly-available tree
> (Andy's private x86-next).

Its perfectly public

http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/x86-next

and has been in effect during commit freezes for the past several years
worth of releases.

It is fine to post a tree based on something other than xen.git/staging
so long as the cover letter identifies where the series is based.  A lot
of development already occurs in this way on xen-devel.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

On 4/29/19 3:49 PM, Andrew Cooper wrote:
> On 29/04/2019 15:46, George Dunlap wrote:
>> On 4/29/19 3:32 PM, George Dunlap wrote:
>>> On 4/26/19 6:21 PM, Tamas K Lengyel wrote:
>>>> Calling _put_page_type while also holding the page_lock
>>>> for that page can cause a deadlock.
>>>>
>>>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
>>>> Cc: Jan Beulich <jbeulich@suse.com>
>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>>> Cc: Roger Pau Monne <roger.pau@citrix.com>
>>>> ---
>>>> v3: simplified patch by keeping the additional references already in-place
>>>> ---
>>>>  xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
>>>>  1 file changed, 10 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
>>>> index dfc279d371..e2f74ac770 100644
>>>> --- a/xen/arch/x86/mm/mem_sharing.c
>>>> +++ b/xen/arch/x86/mm/mem_sharing.c
>>>> @@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct page_info *page)
>>>>          return -EBUSY;
>>>>      }
>>>>  
>>>> -    /* We can only change the type if count is one */
>>>> -    /* Because we are locking pages individually, we need to drop
>>>> -     * the lock here, while the page is typed. We cannot risk the 
>>>> -     * race of page_unlock and then put_page_type. */
>>>>      expected_type = (PGT_shared_page | PGT_validated | PGT_locked | 2);
>>>>      if ( page->u.inuse.type_info != expected_type )
>>>>      {
>>>> @@ -660,12 +656,12 @@ static int page_make_private(struct domain *d, struct page_info *page)
>>>>          return -EEXIST;
>>>>      }
>>>>  
>>>> -    /* Drop the final typecount */
>>>> -    put_page_and_type(page);
>>>> -
>>>>      /* Now that we've dropped the type, we can unlock */
>>>>      mem_sharing_page_unlock(page);
>>>>  
>>>> +    /* Drop the final typecount */
>>>> +    put_page_and_type(page);
>>>> +
>>>>      /* Change the owner */
>>>>      ASSERT(page_get_owner(page) == dom_cow);
>>>>      page_set_owner(page, d);
>>>> @@ -900,6 +896,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>>      p2m_type_t smfn_type, cmfn_type;
>>>>      struct two_gfns tg;
>>>>      struct rmap_iterator ri;
>>>> +    unsigned long put_count = 0;
>>>>  
>>>>      get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
>>>>                   cd, cgfn, &cmfn_type, NULL, &cmfn, 0, &tg);
>>>> @@ -984,7 +981,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>>           * Don't change the type of rmap for the client page. */
>>>>          rmap_del(gfn, cpage, 0);
>>>>          rmap_add(gfn, spage);
>>>> -        put_page_and_type(cpage);
>>>> +        put_count++;
>>>>          d = get_domain_by_id(gfn->domain);
>>>>          BUG_ON(!d);
>>>>          BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
>>>> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>>      mem_sharing_page_unlock(secondpg);
>>>>      mem_sharing_page_unlock(firstpg);
>>>>  
>>>> +    BUG_ON(!put_count);
>>>> +    while ( put_count-- )
>>>> +        put_page_and_type(cpage);
>>>> +
>>>>      /* Free the client page */
>>>>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>>>>          put_page(cpage);
>>>> @@ -1167,8 +1168,8 @@ int __mem_sharing_unshare_page(struct domain *d,
>>>>      {
>>>>          if ( !last_gfn )
>>>>              mem_sharing_gfn_destroy(page, d, gfn_info);
>>>> -        put_page_and_type(page);
>>>>          mem_sharing_page_unlock(page);
>>>> +        put_page_and_type(page);
>>>>          if ( last_gfn )
>>>>          {
>>>>              if ( !get_page(page, dom_cow) )
>>> ...Probably should have mentioned that this needs to be applied after
>>> your other patch. :-)
>> Hmm -- actually, the base appears to be a non-publicly-available tree
>> (Andy's private x86-next).
> 
> Its perfectly public
> 
> http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/x86-next

OK, didn't realize this.

> 
> and has been in effect during commit freezes for the past several years
> worth of releases.
> 
> It is fine to post a tree based on something other than xen.git/staging
> so long as the cover letter identifies where the series is based.  A lot
> of development already occurs in this way on xen-devel.

I agree it's fine when a URL is provided.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

On 4/29/19 3:49 PM, Andrew Cooper wrote:
> On 29/04/2019 15:46, George Dunlap wrote:
>> On 4/29/19 3:32 PM, George Dunlap wrote:
>>> On 4/26/19 6:21 PM, Tamas K Lengyel wrote:
>>>> Calling _put_page_type while also holding the page_lock
>>>> for that page can cause a deadlock.
>>>>
>>>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
>>>> Cc: Jan Beulich <jbeulich@suse.com>
>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>>> Cc: Roger Pau Monne <roger.pau@citrix.com>
>>>> ---
>>>> v3: simplified patch by keeping the additional references already in-place
>>>> ---
>>>>  xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
>>>>  1 file changed, 10 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
>>>> index dfc279d371..e2f74ac770 100644
>>>> --- a/xen/arch/x86/mm/mem_sharing.c
>>>> +++ b/xen/arch/x86/mm/mem_sharing.c
>>>> @@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct page_info *page)
>>>>          return -EBUSY;
>>>>      }
>>>>  
>>>> -    /* We can only change the type if count is one */
>>>> -    /* Because we are locking pages individually, we need to drop
>>>> -     * the lock here, while the page is typed. We cannot risk the 
>>>> -     * race of page_unlock and then put_page_type. */
>>>>      expected_type = (PGT_shared_page | PGT_validated | PGT_locked | 2);
>>>>      if ( page->u.inuse.type_info != expected_type )
>>>>      {
>>>> @@ -660,12 +656,12 @@ static int page_make_private(struct domain *d, struct page_info *page)
>>>>          return -EEXIST;
>>>>      }
>>>>  
>>>> -    /* Drop the final typecount */
>>>> -    put_page_and_type(page);
>>>> -
>>>>      /* Now that we've dropped the type, we can unlock */
>>>>      mem_sharing_page_unlock(page);
>>>>  
>>>> +    /* Drop the final typecount */
>>>> +    put_page_and_type(page);
>>>> +
>>>>      /* Change the owner */
>>>>      ASSERT(page_get_owner(page) == dom_cow);
>>>>      page_set_owner(page, d);
>>>> @@ -900,6 +896,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>>      p2m_type_t smfn_type, cmfn_type;
>>>>      struct two_gfns tg;
>>>>      struct rmap_iterator ri;
>>>> +    unsigned long put_count = 0;
>>>>  
>>>>      get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
>>>>                   cd, cgfn, &cmfn_type, NULL, &cmfn, 0, &tg);
>>>> @@ -984,7 +981,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>>           * Don't change the type of rmap for the client page. */
>>>>          rmap_del(gfn, cpage, 0);
>>>>          rmap_add(gfn, spage);
>>>> -        put_page_and_type(cpage);
>>>> +        put_count++;
>>>>          d = get_domain_by_id(gfn->domain);
>>>>          BUG_ON(!d);
>>>>          BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
>>>> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>>      mem_sharing_page_unlock(secondpg);
>>>>      mem_sharing_page_unlock(firstpg);
>>>>  
>>>> +    BUG_ON(!put_count);
>>>> +    while ( put_count-- )
>>>> +        put_page_and_type(cpage);
>>>> +
>>>>      /* Free the client page */
>>>>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>>>>          put_page(cpage);
>>>> @@ -1167,8 +1168,8 @@ int __mem_sharing_unshare_page(struct domain *d,
>>>>      {
>>>>          if ( !last_gfn )
>>>>              mem_sharing_gfn_destroy(page, d, gfn_info);
>>>> -        put_page_and_type(page);
>>>>          mem_sharing_page_unlock(page);
>>>> +        put_page_and_type(page);
>>>>          if ( last_gfn )
>>>>          {
>>>>              if ( !get_page(page, dom_cow) )
>>> ...Probably should have mentioned that this needs to be applied after
>>> your other patch. :-)
>> Hmm -- actually, the base appears to be a non-publicly-available tree
>> (Andy's private x86-next).
> 
> Its perfectly public
> 
> http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/x86-next

OK, didn't realize this.

> 
> and has been in effect during commit freezes for the past several years
> worth of releases.
> 
> It is fine to post a tree based on something other than xen.git/staging
> so long as the cover letter identifies where the series is based.  A lot
> of development already occurs in this way on xen-devel.

I agree it's fine when a URL is provided.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 8:54 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 4/29/19 3:49 PM, Andrew Cooper wrote:
> > On 29/04/2019 15:46, George Dunlap wrote:
> >> On 4/29/19 3:32 PM, George Dunlap wrote:
> >>> On 4/26/19 6:21 PM, Tamas K Lengyel wrote:
> >>>> Calling _put_page_type while also holding the page_lock
> >>>> for that page can cause a deadlock.
> >>>>
> >>>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> >>>> Cc: Jan Beulich <jbeulich@suse.com>
> >>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> >>>> Cc: Wei Liu <wei.liu2@citrix.com>
> >>>> Cc: Roger Pau Monne <roger.pau@citrix.com>
> >>>> ---
> >>>> v3: simplified patch by keeping the additional references already in-place
> >>>> ---
> >>>>  xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
> >>>>  1 file changed, 10 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> >>>> index dfc279d371..e2f74ac770 100644
> >>>> --- a/xen/arch/x86/mm/mem_sharing.c
> >>>> +++ b/xen/arch/x86/mm/mem_sharing.c
> >>>> @@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct page_info *page)
> >>>>          return -EBUSY;
> >>>>      }
> >>>>
> >>>> -    /* We can only change the type if count is one */
> >>>> -    /* Because we are locking pages individually, we need to drop
> >>>> -     * the lock here, while the page is typed. We cannot risk the
> >>>> -     * race of page_unlock and then put_page_type. */
> >>>>      expected_type = (PGT_shared_page | PGT_validated | PGT_locked | 2);
> >>>>      if ( page->u.inuse.type_info != expected_type )
> >>>>      {
> >>>> @@ -660,12 +656,12 @@ static int page_make_private(struct domain *d, struct page_info *page)
> >>>>          return -EEXIST;
> >>>>      }
> >>>>
> >>>> -    /* Drop the final typecount */
> >>>> -    put_page_and_type(page);
> >>>> -
> >>>>      /* Now that we've dropped the type, we can unlock */
> >>>>      mem_sharing_page_unlock(page);
> >>>>
> >>>> +    /* Drop the final typecount */
> >>>> +    put_page_and_type(page);
> >>>> +
> >>>>      /* Change the owner */
> >>>>      ASSERT(page_get_owner(page) == dom_cow);
> >>>>      page_set_owner(page, d);
> >>>> @@ -900,6 +896,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
> >>>>      p2m_type_t smfn_type, cmfn_type;
> >>>>      struct two_gfns tg;
> >>>>      struct rmap_iterator ri;
> >>>> +    unsigned long put_count = 0;
> >>>>
> >>>>      get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
> >>>>                   cd, cgfn, &cmfn_type, NULL, &cmfn, 0, &tg);
> >>>> @@ -984,7 +981,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
> >>>>           * Don't change the type of rmap for the client page. */
> >>>>          rmap_del(gfn, cpage, 0);
> >>>>          rmap_add(gfn, spage);
> >>>> -        put_page_and_type(cpage);
> >>>> +        put_count++;
> >>>>          d = get_domain_by_id(gfn->domain);
> >>>>          BUG_ON(!d);
> >>>>          BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
> >>>> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
> >>>>      mem_sharing_page_unlock(secondpg);
> >>>>      mem_sharing_page_unlock(firstpg);
> >>>>
> >>>> +    BUG_ON(!put_count);
> >>>> +    while ( put_count-- )
> >>>> +        put_page_and_type(cpage);
> >>>> +
> >>>>      /* Free the client page */
> >>>>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
> >>>>          put_page(cpage);
> >>>> @@ -1167,8 +1168,8 @@ int __mem_sharing_unshare_page(struct domain *d,
> >>>>      {
> >>>>          if ( !last_gfn )
> >>>>              mem_sharing_gfn_destroy(page, d, gfn_info);
> >>>> -        put_page_and_type(page);
> >>>>          mem_sharing_page_unlock(page);
> >>>> +        put_page_and_type(page);
> >>>>          if ( last_gfn )
> >>>>          {
> >>>>              if ( !get_page(page, dom_cow) )
> >>> ...Probably should have mentioned that this needs to be applied after
> >>> your other patch. :-)
> >> Hmm -- actually, the base appears to be a non-publicly-available tree
> >> (Andy's private x86-next).
> >
> > Its perfectly public
> >
> > http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/x86-next
>
> OK, didn't realize this.
>
> >
> > and has been in effect during commit freezes for the past several years
> > worth of releases.
> >
> > It is fine to post a tree based on something other than xen.git/staging
> > so long as the cover letter identifies where the series is based.  A lot
> > of development already occurs in this way on xen-devel.
>
> I agree it's fine when a URL is provided.
>

Yeap, sorry, should have pointed out I rebased on x86-next so I don't
keep sending patches that are already pulled in.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 8:54 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 4/29/19 3:49 PM, Andrew Cooper wrote:
> > On 29/04/2019 15:46, George Dunlap wrote:
> >> On 4/29/19 3:32 PM, George Dunlap wrote:
> >>> On 4/26/19 6:21 PM, Tamas K Lengyel wrote:
> >>>> Calling _put_page_type while also holding the page_lock
> >>>> for that page can cause a deadlock.
> >>>>
> >>>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> >>>> Cc: Jan Beulich <jbeulich@suse.com>
> >>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> >>>> Cc: Wei Liu <wei.liu2@citrix.com>
> >>>> Cc: Roger Pau Monne <roger.pau@citrix.com>
> >>>> ---
> >>>> v3: simplified patch by keeping the additional references already in-place
> >>>> ---
> >>>>  xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
> >>>>  1 file changed, 10 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> >>>> index dfc279d371..e2f74ac770 100644
> >>>> --- a/xen/arch/x86/mm/mem_sharing.c
> >>>> +++ b/xen/arch/x86/mm/mem_sharing.c
> >>>> @@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct page_info *page)
> >>>>          return -EBUSY;
> >>>>      }
> >>>>
> >>>> -    /* We can only change the type if count is one */
> >>>> -    /* Because we are locking pages individually, we need to drop
> >>>> -     * the lock here, while the page is typed. We cannot risk the
> >>>> -     * race of page_unlock and then put_page_type. */
> >>>>      expected_type = (PGT_shared_page | PGT_validated | PGT_locked | 2);
> >>>>      if ( page->u.inuse.type_info != expected_type )
> >>>>      {
> >>>> @@ -660,12 +656,12 @@ static int page_make_private(struct domain *d, struct page_info *page)
> >>>>          return -EEXIST;
> >>>>      }
> >>>>
> >>>> -    /* Drop the final typecount */
> >>>> -    put_page_and_type(page);
> >>>> -
> >>>>      /* Now that we've dropped the type, we can unlock */
> >>>>      mem_sharing_page_unlock(page);
> >>>>
> >>>> +    /* Drop the final typecount */
> >>>> +    put_page_and_type(page);
> >>>> +
> >>>>      /* Change the owner */
> >>>>      ASSERT(page_get_owner(page) == dom_cow);
> >>>>      page_set_owner(page, d);
> >>>> @@ -900,6 +896,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
> >>>>      p2m_type_t smfn_type, cmfn_type;
> >>>>      struct two_gfns tg;
> >>>>      struct rmap_iterator ri;
> >>>> +    unsigned long put_count = 0;
> >>>>
> >>>>      get_two_gfns(sd, sgfn, &smfn_type, NULL, &smfn,
> >>>>                   cd, cgfn, &cmfn_type, NULL, &cmfn, 0, &tg);
> >>>> @@ -984,7 +981,7 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
> >>>>           * Don't change the type of rmap for the client page. */
> >>>>          rmap_del(gfn, cpage, 0);
> >>>>          rmap_add(gfn, spage);
> >>>> -        put_page_and_type(cpage);
> >>>> +        put_count++;
> >>>>          d = get_domain_by_id(gfn->domain);
> >>>>          BUG_ON(!d);
> >>>>          BUG_ON(set_shared_p2m_entry(d, gfn->gfn, smfn));
> >>>> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
> >>>>      mem_sharing_page_unlock(secondpg);
> >>>>      mem_sharing_page_unlock(firstpg);
> >>>>
> >>>> +    BUG_ON(!put_count);
> >>>> +    while ( put_count-- )
> >>>> +        put_page_and_type(cpage);
> >>>> +
> >>>>      /* Free the client page */
> >>>>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
> >>>>          put_page(cpage);
> >>>> @@ -1167,8 +1168,8 @@ int __mem_sharing_unshare_page(struct domain *d,
> >>>>      {
> >>>>          if ( !last_gfn )
> >>>>              mem_sharing_gfn_destroy(page, d, gfn_info);
> >>>> -        put_page_and_type(page);
> >>>>          mem_sharing_page_unlock(page);
> >>>> +        put_page_and_type(page);
> >>>>          if ( last_gfn )
> >>>>          {
> >>>>              if ( !get_page(page, dom_cow) )
> >>> ...Probably should have mentioned that this needs to be applied after
> >>> your other patch. :-)
> >> Hmm -- actually, the base appears to be a non-publicly-available tree
> >> (Andy's private x86-next).
> >
> > Its perfectly public
> >
> > http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/x86-next
>
> OK, didn't realize this.
>
> >
> > and has been in effect during commit freezes for the past several years
> > worth of releases.
> >
> > It is fine to post a tree based on something other than xen.git/staging
> > so long as the cover letter identifies where the series is based.  A lot
> > of development already occurs in this way on xen-devel.
>
> I agree it's fine when a URL is provided.
>

Yeap, sorry, should have pointed out I rebased on x86-next so I don't
keep sending patches that are already pulled in.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Jan Beulich]

>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>      mem_sharing_page_unlock(secondpg);
>      mem_sharing_page_unlock(firstpg);
>  
> +    BUG_ON(!put_count);
> +    while ( put_count-- )
> +        put_page_and_type(cpage);
> +
>      /* Free the client page */
>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>          put_page(cpage);

If this was to happen before all the put_page_and_type() calls,
wouldn't it render unnecessary the extra get_page()/put_page()
around this code region?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Jan Beulich]

>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>      mem_sharing_page_unlock(secondpg);
>      mem_sharing_page_unlock(firstpg);
>  
> +    BUG_ON(!put_count);
> +    while ( put_count-- )
> +        put_page_and_type(cpage);
> +
>      /* Free the client page */
>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>          put_page(cpage);

If this was to happen before all the put_page_and_type() calls,
wouldn't it render unnecessary the extra get_page()/put_page()
around this code region?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 9:01 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> > @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
> >      mem_sharing_page_unlock(secondpg);
> >      mem_sharing_page_unlock(firstpg);
> >
> > +    BUG_ON(!put_count);
> > +    while ( put_count-- )
> > +        put_page_and_type(cpage);
> > +
> >      /* Free the client page */
> >      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
> >          put_page(cpage);
>
> If this was to happen before all the put_page_and_type() calls,
> wouldn't it render unnecessary the extra get_page()/put_page()
> around this code region?

It would - I already sent a version of the patch in that form but
there was unease expressed by George going that route because of the
complexity of the code and in his view it's the safe bet to keep the
extra references. I think the overhead of grabbing the extra
references is negligible enough that I'm not going to argue over it.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 9:01 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> > @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
> >      mem_sharing_page_unlock(secondpg);
> >      mem_sharing_page_unlock(firstpg);
> >
> > +    BUG_ON(!put_count);
> > +    while ( put_count-- )
> > +        put_page_and_type(cpage);
> > +
> >      /* Free the client page */
> >      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
> >          put_page(cpage);
>
> If this was to happen before all the put_page_and_type() calls,
> wouldn't it render unnecessary the extra get_page()/put_page()
> around this code region?

It would - I already sent a version of the patch in that form but
there was unease expressed by George going that route because of the
complexity of the code and in his view it's the safe bet to keep the
extra references. I think the overhead of grabbing the extra
references is negligible enough that I'm not going to argue over it.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

On 4/29/19 4:41 PM, Tamas K Lengyel wrote:
> On Mon, Apr 29, 2019 at 9:01 AM Jan Beulich <JBeulich@suse.com> wrote:
>>
>>>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
>>> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>      mem_sharing_page_unlock(secondpg);
>>>      mem_sharing_page_unlock(firstpg);
>>>
>>> +    BUG_ON(!put_count);
>>> +    while ( put_count-- )
>>> +        put_page_and_type(cpage);
>>> +
>>>      /* Free the client page */
>>>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>>>          put_page(cpage);
>>
>> If this was to happen before all the put_page_and_type() calls,
>> wouldn't it render unnecessary the extra get_page()/put_page()
>> around this code region?
> 
> It would - I already sent a version of the patch in that form but
> there was unease expressed by George going that route because of the
> complexity of the code and in his view it's the safe bet to keep the
> extra references. I think the overhead of grabbing the extra
> references is negligible enough that I'm not going to argue over it.

Er, that's not what I said. :-)

I gave four possible options, *one* of which was to change the ASSERT()
to a BUG_ON(), and *one* of which was to keep the (probably redundant)
get_page() and put_page() calls.  You appear to have done both. :-)

I haven't re-grokked the code here, but assuming I was correct 2 weeks
ago, if you have the BUG_ON() there, you can get rid of the extra
references.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

On 4/29/19 4:41 PM, Tamas K Lengyel wrote:
> On Mon, Apr 29, 2019 at 9:01 AM Jan Beulich <JBeulich@suse.com> wrote:
>>
>>>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
>>> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
>>>      mem_sharing_page_unlock(secondpg);
>>>      mem_sharing_page_unlock(firstpg);
>>>
>>> +    BUG_ON(!put_count);
>>> +    while ( put_count-- )
>>> +        put_page_and_type(cpage);
>>> +
>>>      /* Free the client page */
>>>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
>>>          put_page(cpage);
>>
>> If this was to happen before all the put_page_and_type() calls,
>> wouldn't it render unnecessary the extra get_page()/put_page()
>> around this code region?
> 
> It would - I already sent a version of the patch in that form but
> there was unease expressed by George going that route because of the
> complexity of the code and in his view it's the safe bet to keep the
> extra references. I think the overhead of grabbing the extra
> references is negligible enough that I'm not going to argue over it.

Er, that's not what I said. :-)

I gave four possible options, *one* of which was to change the ASSERT()
to a BUG_ON(), and *one* of which was to keep the (probably redundant)
get_page() and put_page() calls.  You appear to have done both. :-)

I haven't re-grokked the code here, but assuming I was correct 2 weeks
ago, if you have the BUG_ON() there, you can get rid of the extra
references.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 9:52 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 4/29/19 4:41 PM, Tamas K Lengyel wrote:
> > On Mon, Apr 29, 2019 at 9:01 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>
> >>>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> >>> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
> >>>      mem_sharing_page_unlock(secondpg);
> >>>      mem_sharing_page_unlock(firstpg);
> >>>
> >>> +    BUG_ON(!put_count);
> >>> +    while ( put_count-- )
> >>> +        put_page_and_type(cpage);
> >>> +
> >>>      /* Free the client page */
> >>>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
> >>>          put_page(cpage);
> >>
> >> If this was to happen before all the put_page_and_type() calls,
> >> wouldn't it render unnecessary the extra get_page()/put_page()
> >> around this code region?
> >
> > It would - I already sent a version of the patch in that form but
> > there was unease expressed by George going that route because of the
> > complexity of the code and in his view it's the safe bet to keep the
> > extra references. I think the overhead of grabbing the extra
> > references is negligible enough that I'm not going to argue over it.
>
> Er, that's not what I said. :-)
>
> I gave four possible options, *one* of which was to change the ASSERT()
> to a BUG_ON(), and *one* of which was to keep the (probably redundant)
> get_page() and put_page() calls.  You appear to have done both. :-)

Ah indeed - I've combined those recommendations :)

>
> I haven't re-grokked the code here, but assuming I was correct 2 weeks
> ago, if you have the BUG_ON() there, you can get rid of the extra
> references.
>

Sure, but again, the overhead of having them in-place is negligible so
might as well just keep it.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 9:52 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 4/29/19 4:41 PM, Tamas K Lengyel wrote:
> > On Mon, Apr 29, 2019 at 9:01 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>
> >>>>> On 26.04.19 at 19:21, <tamas@tklengyel.com> wrote:
> >>> @@ -999,6 +996,10 @@ static int share_pages(struct domain *sd, gfn_t sgfn, shr_handle_t sh,
> >>>      mem_sharing_page_unlock(secondpg);
> >>>      mem_sharing_page_unlock(firstpg);
> >>>
> >>> +    BUG_ON(!put_count);
> >>> +    while ( put_count-- )
> >>> +        put_page_and_type(cpage);
> >>> +
> >>>      /* Free the client page */
> >>>      if(test_and_clear_bit(_PGC_allocated, &cpage->count_info))
> >>>          put_page(cpage);
> >>
> >> If this was to happen before all the put_page_and_type() calls,
> >> wouldn't it render unnecessary the extra get_page()/put_page()
> >> around this code region?
> >
> > It would - I already sent a version of the patch in that form but
> > there was unease expressed by George going that route because of the
> > complexity of the code and in his view it's the safe bet to keep the
> > extra references. I think the overhead of grabbing the extra
> > references is negligible enough that I'm not going to argue over it.
>
> Er, that's not what I said. :-)
>
> I gave four possible options, *one* of which was to change the ASSERT()
> to a BUG_ON(), and *one* of which was to keep the (probably redundant)
> get_page() and put_page() calls.  You appear to have done both. :-)

Ah indeed - I've combined those recommendations :)

>
> I haven't re-grokked the code here, but assuming I was correct 2 weeks
> ago, if you have the BUG_ON() there, you can get rid of the extra
> references.
>

Sure, but again, the overhead of having them in-place is negligible so
might as well just keep it.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Jan Beulich]

>>> On 29.04.19 at 18:05, <tamas@tklengyel.com> wrote:
> On Mon, Apr 29, 2019 at 9:52 AM George Dunlap <george.dunlap@citrix.com> wrote:
>> I haven't re-grokked the code here, but assuming I was correct 2 weeks
>> ago, if you have the BUG_ON() there, you can get rid of the extra
>> references.
> 
> Sure, but again, the overhead of having them in-place is negligible so
> might as well just keep it.

The overhead is only one aspect here. People looking at the code
may also be mislead into trying to figure out why the heck this
extra reference gets obtained. Plus sub-optimal code tends to get
cloned ...

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Jan Beulich]

>>> On 29.04.19 at 18:05, <tamas@tklengyel.com> wrote:
> On Mon, Apr 29, 2019 at 9:52 AM George Dunlap <george.dunlap@citrix.com> wrote:
>> I haven't re-grokked the code here, but assuming I was correct 2 weeks
>> ago, if you have the BUG_ON() there, you can get rid of the extra
>> references.
> 
> Sure, but again, the overhead of having them in-place is negligible so
> might as well just keep it.

The overhead is only one aspect here. People looking at the code
may also be mislead into trying to figure out why the heck this
extra reference gets obtained. Plus sub-optimal code tends to get
cloned ...

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

On 4/29/19 5:14 PM, Jan Beulich wrote:
>>>> On 29.04.19 at 18:05, <tamas@tklengyel.com> wrote:
>> On Mon, Apr 29, 2019 at 9:52 AM George Dunlap <george.dunlap@citrix.com> wrote:
>>> I haven't re-grokked the code here, but assuming I was correct 2 weeks
>>> ago, if you have the BUG_ON() there, you can get rid of the extra
>>> references.
>>
>> Sure, but again, the overhead of having them in-place is negligible so
>> might as well just keep it.
> 
> The overhead is only one aspect here. People looking at the code
> may also be mislead into trying to figure out why the heck this
> extra reference gets obtained. Plus sub-optimal code tends to get
> cloned ...

I think it's better not to have duplicate measures as well, but I don't
want to argue too much over it.

At very least, there should be a comment saying that the get_page()
shouldn't be necessary.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

On 4/29/19 5:14 PM, Jan Beulich wrote:
>>>> On 29.04.19 at 18:05, <tamas@tklengyel.com> wrote:
>> On Mon, Apr 29, 2019 at 9:52 AM George Dunlap <george.dunlap@citrix.com> wrote:
>>> I haven't re-grokked the code here, but assuming I was correct 2 weeks
>>> ago, if you have the BUG_ON() there, you can get rid of the extra
>>> references.
>>
>> Sure, but again, the overhead of having them in-place is negligible so
>> might as well just keep it.
> 
> The overhead is only one aspect here. People looking at the code
> may also be mislead into trying to figure out why the heck this
> extra reference gets obtained. Plus sub-optimal code tends to get
> cloned ...

I think it's better not to have duplicate measures as well, but I don't
want to argue too much over it.

At very least, there should be a comment saying that the get_page()
shouldn't be necessary.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 10:14 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 29.04.19 at 18:05, <tamas@tklengyel.com> wrote:
> > On Mon, Apr 29, 2019 at 9:52 AM George Dunlap <george.dunlap@citrix.com> wrote:
> >> I haven't re-grokked the code here, but assuming I was correct 2 weeks
> >> ago, if you have the BUG_ON() there, you can get rid of the extra
> >> references.
> >
> > Sure, but again, the overhead of having them in-place is negligible so
> > might as well just keep it.
>
> The overhead is only one aspect here. People looking at the code
> may also be mislead into trying to figure out why the heck this
> extra reference gets obtained. Plus sub-optimal code tends to get
> cloned ...

Yea, I'm with you.. Alright, in that case Andrew pulled in that
previous patch into x86-next for no good reason as that whole thing is
going to get dropped now. Andrew - if you can just drop that patch
from x86-next I'll rebase on staging and resend without that patch.

Thanks,
Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 10:14 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 29.04.19 at 18:05, <tamas@tklengyel.com> wrote:
> > On Mon, Apr 29, 2019 at 9:52 AM George Dunlap <george.dunlap@citrix.com> wrote:
> >> I haven't re-grokked the code here, but assuming I was correct 2 weeks
> >> ago, if you have the BUG_ON() there, you can get rid of the extra
> >> references.
> >
> > Sure, but again, the overhead of having them in-place is negligible so
> > might as well just keep it.
>
> The overhead is only one aspect here. People looking at the code
> may also be mislead into trying to figure out why the heck this
> extra reference gets obtained. Plus sub-optimal code tends to get
> cloned ...

Yea, I'm with you.. Alright, in that case Andrew pulled in that
previous patch into x86-next for no good reason as that whole thing is
going to get dropped now. Andrew - if you can just drop that patch
from x86-next I'll rebase on staging and resend without that patch.

Thanks,
Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

On 4/29/19 5:26 PM, Tamas K Lengyel wrote:
> On Mon, Apr 29, 2019 at 10:14 AM Jan Beulich <JBeulich@suse.com> wrote:
>>
>>>>> On 29.04.19 at 18:05, <tamas@tklengyel.com> wrote:
>>> On Mon, Apr 29, 2019 at 9:52 AM George Dunlap <george.dunlap@citrix.com> wrote:
>>>> I haven't re-grokked the code here, but assuming I was correct 2 weeks
>>>> ago, if you have the BUG_ON() there, you can get rid of the extra
>>>> references.
>>>
>>> Sure, but again, the overhead of having them in-place is negligible so
>>> might as well just keep it.
>>
>> The overhead is only one aspect here. People looking at the code
>> may also be mislead into trying to figure out why the heck this
>> extra reference gets obtained. Plus sub-optimal code tends to get
>> cloned ...
> 
> Yea, I'm with you.. Alright, in that case Andrew pulled in that
> previous patch into x86-next for no good reason as that whole thing is
> going to get dropped now. Andrew - if you can just drop that patch
> from x86-next I'll rebase on staging and resend without that patch.

I assume he wants that branch to be fast-forwarding; if so, he can't
really pull it out.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

On 4/29/19 5:26 PM, Tamas K Lengyel wrote:
> On Mon, Apr 29, 2019 at 10:14 AM Jan Beulich <JBeulich@suse.com> wrote:
>>
>>>>> On 29.04.19 at 18:05, <tamas@tklengyel.com> wrote:
>>> On Mon, Apr 29, 2019 at 9:52 AM George Dunlap <george.dunlap@citrix.com> wrote:
>>>> I haven't re-grokked the code here, but assuming I was correct 2 weeks
>>>> ago, if you have the BUG_ON() there, you can get rid of the extra
>>>> references.
>>>
>>> Sure, but again, the overhead of having them in-place is negligible so
>>> might as well just keep it.
>>
>> The overhead is only one aspect here. People looking at the code
>> may also be mislead into trying to figure out why the heck this
>> extra reference gets obtained. Plus sub-optimal code tends to get
>> cloned ...
> 
> Yea, I'm with you.. Alright, in that case Andrew pulled in that
> previous patch into x86-next for no good reason as that whole thing is
> going to get dropped now. Andrew - if you can just drop that patch
> from x86-next I'll rebase on staging and resend without that patch.

I assume he wants that branch to be fast-forwarding; if so, he can't
really pull it out.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 10:29 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 4/29/19 5:26 PM, Tamas K Lengyel wrote:
> > On Mon, Apr 29, 2019 at 10:14 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>
> >>>>> On 29.04.19 at 18:05, <tamas@tklengyel.com> wrote:
> >>> On Mon, Apr 29, 2019 at 9:52 AM George Dunlap <george.dunlap@citrix.com> wrote:
> >>>> I haven't re-grokked the code here, but assuming I was correct 2 weeks
> >>>> ago, if you have the BUG_ON() there, you can get rid of the extra
> >>>> references.
> >>>
> >>> Sure, but again, the overhead of having them in-place is negligible so
> >>> might as well just keep it.
> >>
> >> The overhead is only one aspect here. People looking at the code
> >> may also be mislead into trying to figure out why the heck this
> >> extra reference gets obtained. Plus sub-optimal code tends to get
> >> cloned ...
> >
> > Yea, I'm with you.. Alright, in that case Andrew pulled in that
> > previous patch into x86-next for no good reason as that whole thing is
> > going to get dropped now. Andrew - if you can just drop that patch
> > from x86-next I'll rebase on staging and resend without that patch.
>
> I assume he wants that branch to be fast-forwarding; if so, he can't
> really pull it out.
>

Yea, figured.. Oh well, not really a big deal.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 10:29 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 4/29/19 5:26 PM, Tamas K Lengyel wrote:
> > On Mon, Apr 29, 2019 at 10:14 AM Jan Beulich <JBeulich@suse.com> wrote:
> >>
> >>>>> On 29.04.19 at 18:05, <tamas@tklengyel.com> wrote:
> >>> On Mon, Apr 29, 2019 at 9:52 AM George Dunlap <george.dunlap@citrix.com> wrote:
> >>>> I haven't re-grokked the code here, but assuming I was correct 2 weeks
> >>>> ago, if you have the BUG_ON() there, you can get rid of the extra
> >>>> references.
> >>>
> >>> Sure, but again, the overhead of having them in-place is negligible so
> >>> might as well just keep it.
> >>
> >> The overhead is only one aspect here. People looking at the code
> >> may also be mislead into trying to figure out why the heck this
> >> extra reference gets obtained. Plus sub-optimal code tends to get
> >> cloned ...
> >
> > Yea, I'm with you.. Alright, in that case Andrew pulled in that
> > previous patch into x86-next for no good reason as that whole thing is
> > going to get dropped now. Andrew - if you can just drop that patch
> > from x86-next I'll rebase on staging and resend without that patch.
>
> I assume he wants that branch to be fast-forwarding; if so, he can't
> really pull it out.
>

Yea, figured.. Oh well, not really a big deal.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

On 4/26/19 6:21 PM, Tamas K Lengyel wrote:
> Calling _put_page_type while also holding the page_lock
> for that page can cause a deadlock.

I can't immediately see what the mem_sharing_page_[un]lock() functions
are meant to be protecting against.  Is there a comment anywhere
describing what they're used for or how they work?

Because...

> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Roger Pau Monne <roger.pau@citrix.com>
> ---
> v3: simplified patch by keeping the additional references already in-place
> ---
>  xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index dfc279d371..e2f74ac770 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct page_info *page)
>          return -EBUSY;
>      }
>  
> -    /* We can only change the type if count is one */
> -    /* Because we are locking pages individually, we need to drop
> -     * the lock here, while the page is typed. We cannot risk the 
> -     * race of page_unlock and then put_page_type. */

...the comment you're removing explicitly says that what you're doing is
"risk"-y.; but you don't explain at all why the comment is wrong.

Ultimately you're the maintainer, and this is non-security-supported, so
if you insist it's safe, I won't oppose it; but it seems like having
some clarity about what is or is not risky and why would be helpful.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[George Dunlap]

On 4/26/19 6:21 PM, Tamas K Lengyel wrote:
> Calling _put_page_type while also holding the page_lock
> for that page can cause a deadlock.

I can't immediately see what the mem_sharing_page_[un]lock() functions
are meant to be protecting against.  Is there a comment anywhere
describing what they're used for or how they work?

Because...

> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Roger Pau Monne <roger.pau@citrix.com>
> ---
> v3: simplified patch by keeping the additional references already in-place
> ---
>  xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index dfc279d371..e2f74ac770 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct page_info *page)
>          return -EBUSY;
>      }
>  
> -    /* We can only change the type if count is one */
> -    /* Because we are locking pages individually, we need to drop
> -     * the lock here, while the page is typed. We cannot risk the 
> -     * race of page_unlock and then put_page_type. */

...the comment you're removing explicitly says that what you're doing is
"risk"-y.; but you don't explain at all why the comment is wrong.

Ultimately you're the maintainer, and this is non-security-supported, so
if you insist it's safe, I won't oppose it; but it seems like having
some clarity about what is or is not risky and why would be helpful.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 9:44 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 4/26/19 6:21 PM, Tamas K Lengyel wrote:
> > Calling _put_page_type while also holding the page_lock
> > for that page can cause a deadlock.
>
> I can't immediately see what the mem_sharing_page_[un]lock() functions
> are meant to be protecting against.  Is there a comment anywhere
> describing what they're used for or how they work?

There are none. The whole subsystem and its of page_lock/unlock use is
undocumented. The lock is used to protect the page_sharing_info
structure from simultaneous updates (when the page is shared/unshared)
and also to make freeing that structure safe.

>
> Because...
>
> > Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: George Dunlap <george.dunlap@eu.citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: Roger Pau Monne <roger.pau@citrix.com>
> > ---
> > v3: simplified patch by keeping the additional references already in-place
> > ---
> >  xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> > index dfc279d371..e2f74ac770 100644
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct page_info *page)
> >          return -EBUSY;
> >      }
> >
> > -    /* We can only change the type if count is one */
> > -    /* Because we are locking pages individually, we need to drop
> > -     * the lock here, while the page is typed. We cannot risk the
> > -     * race of page_unlock and then put_page_type. */
>
> ...the comment you're removing explicitly says that what you're doing is
> "risk"-y.; but you don't explain at all why the comment is wrong.

AFAICT that comment was correct when it was added but since then it is
out-of-date. There is now an explicit switch case added to
_put_page_type that seem to handle this situation (case PGT_locked |
1) that was not there when this comment was introduced. That's what I
was able to decipher using git archeology. IMHO the whole
page_lock/put_type and reference counting is undecipherable (at least
I'm no closer to understanding it after staring at this for 2 weeks
now) and I wish there was a way to just getting rid of the whole thing
but unfortunately there doesn't seem to be one due to not being able
to easily grow page_info.

>
> Ultimately you're the maintainer, and this is non-security-supported, so
> if you insist it's safe, I won't oppose it; but it seems like having
> some clarity about what is or is not risky and why would be helpful.

I'm just trying to revive the subsystem as the way things are right
now it's broken. Doing this reorder makes the deadlock go away.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 1/4] x86/mem_sharing: reorder when pages are unlocked and released

[Tamas K Lengyel]

On Mon, Apr 29, 2019 at 9:44 AM George Dunlap <george.dunlap@citrix.com> wrote:
>
> On 4/26/19 6:21 PM, Tamas K Lengyel wrote:
> > Calling _put_page_type while also holding the page_lock
> > for that page can cause a deadlock.
>
> I can't immediately see what the mem_sharing_page_[un]lock() functions
> are meant to be protecting against.  Is there a comment anywhere
> describing what they're used for or how they work?

There are none. The whole subsystem and its of page_lock/unlock use is
undocumented. The lock is used to protect the page_sharing_info
structure from simultaneous updates (when the page is shared/unshared)
and also to make freeing that structure safe.

>
> Because...
>
> > Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: George Dunlap <george.dunlap@eu.citrix.com>
> > Cc: Wei Liu <wei.liu2@citrix.com>
> > Cc: Roger Pau Monne <roger.pau@citrix.com>
> > ---
> > v3: simplified patch by keeping the additional references already in-place
> > ---
> >  xen/arch/x86/mm/mem_sharing.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> >
> > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> > index dfc279d371..e2f74ac770 100644
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -648,10 +648,6 @@ static int page_make_private(struct domain *d, struct page_info *page)
> >          return -EBUSY;
> >      }
> >
> > -    /* We can only change the type if count is one */
> > -    /* Because we are locking pages individually, we need to drop
> > -     * the lock here, while the page is typed. We cannot risk the
> > -     * race of page_unlock and then put_page_type. */
>
> ...the comment you're removing explicitly says that what you're doing is
> "risk"-y.; but you don't explain at all why the comment is wrong.

AFAICT that comment was correct when it was added but since then it is
out-of-date. There is now an explicit switch case added to
_put_page_type that seem to handle this situation (case PGT_locked |
1) that was not there when this comment was introduced. That's what I
was able to decipher using git archeology. IMHO the whole
page_lock/put_type and reference counting is undecipherable (at least
I'm no closer to understanding it after staring at this for 2 weeks
now) and I wish there was a way to just getting rid of the whole thing
but unfortunately there doesn't seem to be one due to not being able
to easily grow page_info.

>
> Ultimately you're the maintainer, and this is non-security-supported, so
> if you insist it's safe, I won't oppose it; but it seems like having
> some clarity about what is or is not risky and why would be helpful.

I'm just trying to revive the subsystem as the way things are right
now it's broken. Doing this reorder makes the deadlock go away.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel