[XEN] x86/domctl: Have XEN_DOMCTL_getpageframeinfo3 preemptible

Commit Message

Anthony PERARD Nov. 19, 2019, 12:08 p.m. UTC

This hypercall can take a long time to finish because it attempts to
grab the `hostp2m' lock up to 1024 times. The accumulated wait for the
lock can take several seconds.

This can easily happen with a guest with 32 vcpus and plenty of RAM,
during localhost migration.

Signed-off-by: Anthony PERARD <[e-mail]>
---

Notes:
    I don't know if it's a correct way to make the hypercall preemptible,
    the patch kind of modify the response, but libxc doesn't seems to care.
    
    Is it fine to modify the domctl_t that the domain (dom0) provides?
    If not, where could we store the progress made?

 xen/arch/x86/domctl.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Jan Beulich Nov. 19, 2019, 12:50 p.m. UTC | #1

On 19.11.2019 13:08, Anthony PERARD wrote:
> This hypercall can take a long time to finish because it attempts to
> grab the `hostp2m' lock up to 1024 times. The accumulated wait for the
> lock can take several seconds.

Which means several milliseconds on average per lock acquire. This
points (again) at a (the) bigger problem of p2m lock contention.
Therefore I'm afraid that, while the change here is an improvement,
it's only curing symptoms rather than the cause.

Seeing that p2m_get_page_from_gfn() uses a read lock (but sadly is
the only function doing so), one route could be to investigate
whether further paths could get away with just read locking. Fair
parts of e.g. the nested page fault handling don't really seem to
require a write lock, but there is this comment

    /*
     * Take a lock on the host p2m speculatively, to avoid potential
     * locking order problems later and to handle unshare etc.
     */

pointing out the possible issues with downgrading the lock to just
a read one.

Another route to consider would be to avoid taking the lock once
per iteration, and instead process several GFNs at a time within
a single lock holding sequence.

> Notes:
>     I don't know if it's a correct way to make the hypercall preemptible,
>     the patch kind of modify the response, but libxc doesn't seems to care.

I think that's acceptable for domctl-s, but would better be
accompanied by a comment adjustment/addition to public/domctl.h.

> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -425,6 +425,18 @@ long arch_do_domctl(
>                  ret = -EFAULT;
>                  break;
>              }
> +
> +            if ( hypercall_preempt_check() ) {

You don't want this on the last iteration. You also better don't
do this when there's no p2m lock involved to begin with, i.e.
for non-translated guests. This should then be accompanied by a
comment justifying the special casing.

Also the opening brace (one more below here) goes on its own line.

Jan

Patch

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 43e368d63bb9..5c0a7462e63b 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -425,6 +425,18 @@  long arch_do_domctl(
                 ret = -EFAULT;
                 break;
             }
+
+            if ( hypercall_preempt_check() ) {
+                domctl->u.getpageframeinfo3.num = num - i - 1;
+                domctl->u.getpageframeinfo3.array.p =
+                    guest_handle + ((i + 1) * width);
+                if ( __copy_to_guest(u_domctl, domctl, 1) ) {
+                    ret = -EFAULT;
+                    break;
+                }
+                return hypercall_create_continuation(__HYPERVISOR_domctl,
+                                                     "h", u_domctl);
+            }
         }
 
         break;