[PATCH for-4.5] xen: vnuma: expose vnode_to_pnode to guest

[PATCH for-4.5] xen: vnuma: expose vnode_to_pnode to guest

[Wei Liu]

This information is passed in in domctl hypercall but the guest
interface doesn't expose it to guest. PV NUMA-aware ballooning relies on
this piece of information to function properly.

Also fixed one typo while I was there.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Elena Ufimtseva <ufimtseva@gmail.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Wilk <konrad.wilk@oracle.com>
Cc: Dario Faggioli <dario.faggioli@citrix.com>
---
 xen/common/memory.c         |   13 +++++++++++--
 xen/include/public/memory.h |    4 ++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/xen/common/memory.c b/xen/common/memory.c
index cc36e39..fde03dc 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -1013,7 +1013,7 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         /*
          * Copied from guest values may differ from domain vnuma config.
          * Check here guest parameters make sure we dont overflow.
-         * Additionaly check padding.
+         * Additionally check padding.
          */
         if ( topology.nr_vnodes < dom_vnodes      ||
              topology.nr_vcpus < dom_vcpus        ||
@@ -1035,10 +1035,12 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         tmp.vdistance = xmalloc_array(unsigned int, dom_vnodes * dom_vnodes);
         tmp.vmemrange = xmalloc_array(vmemrange_t, dom_vranges);
         tmp.vcpu_to_vnode = xmalloc_array(unsigned int, dom_vcpus);
+        tmp.vnode_to_pnode = xmalloc_array(unsigned int, dom_vnodes);
 
         if ( tmp.vdistance == NULL ||
              tmp.vmemrange == NULL ||
-             tmp.vcpu_to_vnode == NULL )
+             tmp.vcpu_to_vnode == NULL ||
+             tmp.vnode_to_pnode == NULL )
         {
             rc = -ENOMEM;
             goto vnumainfo_out;
@@ -1069,6 +1071,8 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
                sizeof(*d->vnuma->vdistance) * dom_vnodes * dom_vnodes);
         memcpy(tmp.vcpu_to_vnode, d->vnuma->vcpu_to_vnode,
                sizeof(*d->vnuma->vcpu_to_vnode) * dom_vcpus);
+        memcpy(tmp.vnode_to_pnode, d->vnuma->vnode_to_pnode,
+               sizeof(*d->vnuma->vnode_to_pnode) * dom_vnodes);
 
         read_unlock(&d->vnuma_rwlock);
 
@@ -1086,6 +1090,10 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
                            dom_vcpus) != 0 )
             goto vnumainfo_out;
 
+        if ( copy_to_guest(topology.vnode_to_pnode.h, tmp.vnode_to_pnode,
+                           dom_vnodes) != 0 )
+            goto vnumainfo_out;
+
         topology.nr_vnodes = dom_vnodes;
         topology.nr_vcpus = dom_vcpus;
         topology.nr_vmemranges = dom_vranges;
@@ -1098,6 +1106,7 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         xfree(tmp.vdistance);
         xfree(tmp.vmemrange);
         xfree(tmp.vcpu_to_vnode);
+        xfree(tmp.vnode_to_pnode);
         break;
     }
 
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index f021958..9d8c27b 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -566,6 +566,10 @@ struct vnuma_topology_info {
         uint64_t pad;
     } vcpu_to_vnode;
     union {
+        XEN_GUEST_HANDLE(uint) h;
+        uint64_t pad;
+    } vnode_to_pnode;
+    union {
         XEN_GUEST_HANDLE(vmemrange_t) h;
         uint64_t pad;
     } vmemrange;
-- 
1.7.10.4

Re: [PATCH for-4.5] xen: vnuma: expose vnode_to_pnode to guest

[Wei Liu]

Targeting this patch for 4.5 because I don't want to release an
incomplete interface.

Not sure if this topic has been discussed. If it has and the decision
was to not expose the mapping, please ignore this patch.

Wei.

Re: [PATCH for-4.5] xen: vnuma: expose vnode_to_pnode to guest

[Jan Beulich]

>>> On 08.11.14 at 20:43, <wei.liu2@citrix.com> wrote:
> This information is passed in in domctl hypercall but the guest
> interface doesn't expose it to guest. PV NUMA-aware ballooning relies on
> this piece of information to function properly.

Considering that exposing this mapping is wrong from a conceptual
pov (as was discussed during the review of Elena's original series),
the desire to nevertheless expose it would need to be explained
much better than what you did above.

Jan

Re: [PATCH for-4.5] xen: vnuma: expose vnode_to_pnode to guest

[Wei Liu]

On Mon, Nov 10, 2014 at 09:21:24AM +0000, Jan Beulich wrote:
> >>> On 08.11.14 at 20:43, <wei.liu2@citrix.com> wrote:
> > This information is passed in in domctl hypercall but the guest
> > interface doesn't expose it to guest. PV NUMA-aware ballooning relies on
> > this piece of information to function properly.
> 
> Considering that exposing this mapping is wrong from a conceptual
> pov (as was discussed during the review of Elena's original series),
> the desire to nevertheless expose it would need to be explained
> much better than what you did above.
> 

My thought was that if a PV guest needs to do NUMA-aware ballooning, it
would be easier to have the mapping at hand to let the guest request
explicitly from what physical node it wants the page. It was based
on my vague memory of early version of Elena's series.

However, if this is conceptually wrong and has been discussed before,
(as I said in the other email) please just ignore this patch. I can try
to modify the hypervisor instead to make NUMA-aware ballooning happen
under the hood without guest knowing anything. That is, to make use of
the vmemrange structure to identify the vnode of a particular gpfn, then
with vnode_to_pnode map to identify the physical node of that gpfn, then
do NUMA-aware ballooning.

Wei.

> Jan

Re: [PATCH for-4.5] xen: vnuma: expose vnode_to_pnode to guest

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2790 bytes --]

On Mon, 2014-11-10 at 10:00 +0000, Wei Liu wrote:
> On Mon, Nov 10, 2014 at 09:21:24AM +0000, Jan Beulich wrote:
> > >>> On 08.11.14 at 20:43, <wei.liu2@citrix.com> wrote:
> > > This information is passed in in domctl hypercall but the guest
> > > interface doesn't expose it to guest. PV NUMA-aware ballooning relies on
> > > this piece of information to function properly.
> > 
> > Considering that exposing this mapping is wrong from a conceptual
> > pov (as was discussed during the review of Elena's original series),
> > the desire to nevertheless expose it would need to be explained
> > much better than what you did above.
> > 
> 
> My thought was that if a PV guest needs to do NUMA-aware ballooning, it
> would be easier to have the mapping at hand to let the guest request
> explicitly from what physical node it wants the page. It was based
> on my vague memory of early version of Elena's series.
> 
Some discussion on this happened while talking about some early work on
NUMA-aware ballooning. This is a message from that thread:

http://lists.xenproject.org/archives/html/xen-devel/2013-08/msg01986.html

> However, if this is conceptually wrong and has been discussed before,
> (as I said in the other email) please just ignore this patch. I can try
> to modify the hypervisor instead to make NUMA-aware ballooning happen
> under the hood without guest knowing anything. That is, to make use of
> the vmemrange structure to identify the vnode of a particular gpfn, then
> with vnode_to_pnode map to identify the physical node of that gpfn, then
> do NUMA-aware ballooning.
> 
I'm all for *not* exposing such information to the guest. However, a
quote from George, from that thread, with which I *totally* agree with,
is this one:

<<I would like to point out that to make this [NUMA aware ballooning]
  work for ballooning *up*, however, there will need to be a way for the
  guest to specify, "please allocate from vnode X", and have Xen
  translate the vnode into the appropriate pnode(s).>>

If this can be done without exposing the mapping, as Wei suggests, then
I agree we should go for it. If not, we'll have to introduce something
like this (along with proper documentation of how it should be used) at
some point.

I'm 100% ok to re-start that discussion here and now... however, how
stable should this interface be? Can't we deal with this when actually
implementing NUMA aware ballooning and add stuff at than point, if
necessary?

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

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

Re: [PATCH for-4.5] xen: vnuma: expose vnode_to_pnode to guest

[Jan Beulich]

>>> On 10.11.14 at 11:51, <dario.faggioli@citrix.com> wrote:
> I'm 100% ok to re-start that discussion here and now... however, how
> stable should this interface be? Can't we deal with this when actually
> implementing NUMA aware ballooning and add stuff at than point, if
> necessary?

Wei's desire to have a stable interface for 4.5 and later is quite
reasonable, as we can't change the interface once a release got
shipped with it. If we were to discover additional needs later, only
backward compatible changes to the existing interface would be
allowed (and e.g. adding another field to the interface structure
would be out of scope).

Jan

Re: [PATCH for-4.5] xen: vnuma: expose vnode_to_pnode to guest

[Wei Liu]

On Mon, Nov 10, 2014 at 11:51:28AM +0100, Dario Faggioli wrote:
> On Mon, 2014-11-10 at 10:00 +0000, Wei Liu wrote:
> > On Mon, Nov 10, 2014 at 09:21:24AM +0000, Jan Beulich wrote:
> > > >>> On 08.11.14 at 20:43, <wei.liu2@citrix.com> wrote:
> > > > This information is passed in in domctl hypercall but the guest
> > > > interface doesn't expose it to guest. PV NUMA-aware ballooning relies on
> > > > this piece of information to function properly.
> > > 
> > > Considering that exposing this mapping is wrong from a conceptual
> > > pov (as was discussed during the review of Elena's original series),
> > > the desire to nevertheless expose it would need to be explained
> > > much better than what you did above.
> > > 
> > 
> > My thought was that if a PV guest needs to do NUMA-aware ballooning, it
> > would be easier to have the mapping at hand to let the guest request
> > explicitly from what physical node it wants the page. It was based
> > on my vague memory of early version of Elena's series.
> > 
> Some discussion on this happened while talking about some early work on
> NUMA-aware ballooning. This is a message from that thread:
> 
> http://lists.xenproject.org/archives/html/xen-devel/2013-08/msg01986.html
> 

Phew! That's a year ago. No wonder I don't remember anything...

> > However, if this is conceptually wrong and has been discussed before,
> > (as I said in the other email) please just ignore this patch. I can try
> > to modify the hypervisor instead to make NUMA-aware ballooning happen
> > under the hood without guest knowing anything. That is, to make use of
> > the vmemrange structure to identify the vnode of a particular gpfn, then
> > with vnode_to_pnode map to identify the physical node of that gpfn, then
> > do NUMA-aware ballooning.
> > 
> I'm all for *not* exposing such information to the guest. However, a
> quote from George, from that thread, with which I *totally* agree with,
> is this one:
> 
> <<I would like to point out that to make this [NUMA aware ballooning]
>   work for ballooning *up*, however, there will need to be a way for the
>   guest to specify, "please allocate from vnode X", and have Xen
>   translate the vnode into the appropriate pnode(s).>>
> 
> If this can be done without exposing the mapping, as Wei suggests, then
> I agree we should go for it. If not, we'll have to introduce something
> like this (along with proper documentation of how it should be used) at
> some point.
> 
> I'm 100% ok to re-start that discussion here and now... however, how
> stable should this interface be? Can't we deal with this when actually
> implementing NUMA aware ballooning and add stuff at than point, if
> necessary?
> 

The risk would be any new guests with extended get_vnumainfo interface
won't be able to run on old hypervisor (4.5) without proper versioning.

So basically we have three choices:
1. Expose vnode_to_pnode in hypercall interface.
2. Expose the mapping in xenstore.
3. Don't expose anything, everything happens automagically without guest
   knowing anything.

I'm fine with any of those three. However, Jan suggested in that one
year old thread it would be wrong for the guest to know the mapping, so
I think he implicitly voted for the third option.

I only need to make sure that we don't miss option 1 and release
incomplete interface for 4.5. That's why I kick off this discussion.  If
we release the interface as it is now and find out we need to expose
mapping later, we would neither 1) do versioning 2) have yet another
interface to return mapping.

Wei.

> Regards,
> Dario
> 
> -- 
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
> 

Re: [PATCH for-4.5] xen: vnuma: expose vnode_to_pnode to guest

[David Vrabel]

On 10/11/14 11:09, Wei Liu wrote:
> 
> 3. Don't expose anything, everything happens automagically without guest
>    knowing anything.

This.  The vnode to pnode mapping can change on a save/restore.

David

Re: [PATCH for-4.5] xen: vnuma: expose vnode_to_pnode to guest

[Wei Liu]

On Mon, Nov 10, 2014 at 11:21:40AM +0000, David Vrabel wrote:
> On 10/11/14 11:09, Wei Liu wrote:
> > 
> > 3. Don't expose anything, everything happens automagically without guest
> >    knowing anything.
> 
> This.  The vnode to pnode mapping can change on a save/restore.

I don't think this is the most decisive reasoning of this issue. The
other two options can also deal with this -- just retrieve the
up-to-date versions after migration.

Wei.

> 
> David

Re: [PATCH for-4.5] xen: vnuma: expose vnode_to_pnode to guest

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2195 bytes --]

On Mon, 2014-11-10 at 11:09 +0000, Wei Liu wrote:
> On Mon, Nov 10, 2014 at 11:51:28AM +0100, Dario Faggioli wrote:
>
> > I'm 100% ok to re-start that discussion here and now... however, how
> > stable should this interface be? Can't we deal with this when actually
> > implementing NUMA aware ballooning and add stuff at than point, if
> > necessary?
> > 
> The risk would be any new guests with extended get_vnumainfo interface
> won't be able to run on old hypervisor (4.5) without proper versioning.
> 
Right.

> So basically we have three choices:
> 1. Expose vnode_to_pnode in hypercall interface.
> 2. Expose the mapping in xenstore.
> 3. Don't expose anything, everything happens automagically without guest
>    knowing anything.
> 
> I'm fine with any of those three. However, Jan suggested in that one
> year old thread it would be wrong for the guest to know the mapping, so
> I think he implicitly voted for the third option.
> 
Option 3 is the best IMO too. The guest, ideally, should know nothing
about how Xen mapped its virtual NUMA nodes onto the host RAM.

The question here is how effective that is. As of now, it's quite hard
to judge whether we'll be able to do everything automatically, I think.
I read your proposal, and it looks sensible, I'm just saying it's hard
to be conclusive at this stage.

> I only need to make sure that we don't miss option 1 and release
> incomplete interface for 4.5. That's why I kick off this discussion.  If
> we release the interface as it is now and find out we need to expose
> mapping later, we would neither 1) do versioning 2) have yet another
> interface to return mapping.
> 
Exactly. Personally, I'd keep the mapping out of the interface we
already have checked in. If it will reveal impossible to do things
completely automatically, I don't think it will be too terrible to add a
new specific hypercall.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

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

Re: [PATCH for-4.5] xen: vnuma: expose vnode_to_pnode to guest

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1353 bytes --]

On Mon, 2014-11-10 at 11:27 +0000, Wei Liu wrote:
> On Mon, Nov 10, 2014 at 11:21:40AM +0000, David Vrabel wrote:
> > On 10/11/14 11:09, Wei Liu wrote:
> > > 
> > > 3. Don't expose anything, everything happens automagically without guest
> > >    knowing anything.
> > 
> > This.  The vnode to pnode mapping can change on a save/restore.
> 
> I don't think this is the most decisive reasoning of this issue. The
> other two options can also deal with this -- just retrieve the
> up-to-date versions after migration.
> 
IMO, what's needed is an "on demand" "translation service". The guest
should not store the result of such translation and rely on it.

The opposite, actually: when ballooning up (i.e., the only legittimate
use case of this "service" I can think of right now), it should be
possible to let the hypervisor know from what physical NUMA node we want
a page, basing on which vnode such page belongs to.

This is probably another argument for not using option 1, and for going
for 3, if possible, or with something different, if not.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

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

Re: [PATCH for-4.5] xen: vnuma: expose vnode_to_pnode to guest

[Wei Liu]

On Mon, Nov 10, 2014 at 12:42:07PM +0100, Dario Faggioli wrote:
> On Mon, 2014-11-10 at 11:09 +0000, Wei Liu wrote:
> > On Mon, Nov 10, 2014 at 11:51:28AM +0100, Dario Faggioli wrote:
> >
> > > I'm 100% ok to re-start that discussion here and now... however, how
> > > stable should this interface be? Can't we deal with this when actually
> > > implementing NUMA aware ballooning and add stuff at than point, if
> > > necessary?
> > > 
> > The risk would be any new guests with extended get_vnumainfo interface
> > won't be able to run on old hypervisor (4.5) without proper versioning.
> > 
> Right.
> 
> > So basically we have three choices:
> > 1. Expose vnode_to_pnode in hypercall interface.
> > 2. Expose the mapping in xenstore.
> > 3. Don't expose anything, everything happens automagically without guest
> >    knowing anything.
> > 
> > I'm fine with any of those three. However, Jan suggested in that one
> > year old thread it would be wrong for the guest to know the mapping, so
> > I think he implicitly voted for the third option.
> > 
> Option 3 is the best IMO too. The guest, ideally, should know nothing
> about how Xen mapped its virtual NUMA nodes onto the host RAM.
> 

Agreed. I would go for this approach personally.

> The question here is how effective that is. As of now, it's quite hard
> to judge whether we'll be able to do everything automatically, I think.
> I read your proposal, and it looks sensible, I'm just saying it's hard
> to be conclusive at this stage.
> 

I just checked the code. Unfortunately increase_reservation cannot cope
with my proposal.

Currently, the increase_reservation hypercall only allocates pages with
no prior knowledge of what part of guest physical address space those
pages will be added to.

So for NUMA-aware ballooning, we might need a increase_reservation2
hypercall that does the trick I mentioned.

Wei.

Re: [PATCH for-4.5] xen: vnuma: expose vnode_to_pnode to guest

[Wei Liu]

To summarise this discussion so far.

Conceptually speaking, the guest should not need to know the mapping.
The translation from vnode to pnode should happen in hypervisor without
guest knowing.

Jan, Dario and David (and I) are all in favor of *not* exposing the
mapping (though starting from different angle). So we should go with
this approach. The hypercall interface is fine as it is now.

Wei.

Re: [PATCH for-4.5] xen: vnuma: expose vnode_to_pnode to guest

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 784 bytes --]

On Mon, 2014-11-10 at 13:43 +0000, Wei Liu wrote:
> To summarise this discussion so far.
> 
> Conceptually speaking, the guest should not need to know the mapping.
> The translation from vnode to pnode should happen in hypervisor without
> guest knowing.
> 
> Jan, Dario and David (and I) are all in favor of *not* exposing the
> mapping (though starting from different angle). So we should go with
> this approach. The hypercall interface is fine as it is now.
> 
+1

And thanks for the summary.

Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

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