[Qemu-devel] [RFC 0/2] cpu-add compatibility for query-hotpluggable-cpus implementations

[Qemu-devel] [RFC 0/2] cpu-add compatibility for query-hotpluggable-cpus implementations

[David Gibson]

I'm not entirely sure if this is a good idea, and if it is whether
this is a good approach to it.  But I'd like to discuss it and see if
anyone has better ideas.

As you may know we've hit a bunch of complications with cpu_index
which will impose some limitations with what we can do with the new
query-hotpluggable-cpus interface, and we've run out of time to
address these in qemu-2.7.

At the same time we're hitting complications with the fact that the
new qemu interface requires a new libvirt interface to use properly,
and that has follow on effects further up the stack.

Together this means a bunch more delays to having usable CPU hotplug
on Power for downstream users, which is unfortunate.

This is an attempt to get something limited working in a shorter time
frame, by implementing the old cpu-add interface in terms of the new
interface.  Obviously this can't fully exploit the new interface's
capabilities, but you can do basic in-order cpu hotplug without removal.

To make this work, I need to broaden the semantics of cpu-add: it will
a single entry from query-hotpluggable-cpus, which means it may add
multiple vcpus, which the x86 implementation did not previously do.
I'm not sure if the intended semantics of cpu-add were ever defined
well enough to say if this is "wrong" or not.

Because of this, I suspect libvirt will still need some work, but I'm
hoping it might be less that the full new API implementation.

David Gibson (2):
  spapr: Reverse order of hotpluggable cpus list
  qmp: Implement cpu-add in terms of query-hotpluggable-cpus when
    available

 hw/ppc/spapr.c |  2 +-
 qmp.c          | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 1 deletion(-)

-- 
2.7.4

[Qemu-devel] [RFC 1/2] spapr: Reverse order of hotpluggable cpus list

[David Gibson]

The spapr implementation of query-hotpluggable-cpus builds the list of
hotpluggable cores from the end (most removed from the list head)
because that's the easiest way with a singly linked list.  Because it
also traverses the possible CPU cores starting from low indexes the
resulting list has the cpu cores listed in reverse order by core-id.

That's not generally harmful, but it means the output from "info
hotpluggable-cpus" is a little harder to read than it could be.

Therefore, traverse the cpus in reverse order so that the final list
ends up in increasing-core-id order.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7f33a1b..6426b45 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2371,7 +2371,7 @@ static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine)
     int spapr_max_cores = max_cpus / smp_threads;
     int smt = kvmppc_smt_threads();
 
-    for (i = 0; i < spapr_max_cores; i++) {
+    for (i = spapr_max_cores - 1; i >= 0; i--) {
         HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
         HotpluggableCPU *cpu_item = g_new0(typeof(*cpu_item), 1);
         CpuInstanceProperties *cpu_props = g_new0(typeof(*cpu_props), 1);
-- 
2.7.4

Re: [Qemu-devel] [RFC 1/2] spapr: Reverse order of hotpluggable cpus list

[Peter Krempa]

On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote:
> The spapr implementation of query-hotpluggable-cpus builds the list of
> hotpluggable cores from the end (most removed from the list head)
> because that's the easiest way with a singly linked list.  Because it
> also traverses the possible CPU cores starting from low indexes the
> resulting list has the cpu cores listed in reverse order by core-id.
> 
> That's not generally harmful, but it means the output from "info
> hotpluggable-cpus" is a little harder to read than it could be.
> 
> Therefore, traverse the cpus in reverse order so that the final list
> ends up in increasing-core-id order.

To make this interface usable with in-order hotplug the ordering of the
entries should be codified in the schema documentation. (see my response
on the cover letter for justification).

Re: [Qemu-devel] [RFC 1/2] spapr: Reverse order of hotpluggable cpus list

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1202 bytes --]

On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote:
> On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote:
> > The spapr implementation of query-hotpluggable-cpus builds the list of
> > hotpluggable cores from the end (most removed from the list head)
> > because that's the easiest way with a singly linked list.  Because it
> > also traverses the possible CPU cores starting from low indexes the
> > resulting list has the cpu cores listed in reverse order by core-id.
> > 
> > That's not generally harmful, but it means the output from "info
> > hotpluggable-cpus" is a little harder to read than it could be.
> > 
> > Therefore, traverse the cpus in reverse order so that the final list
> > ends up in increasing-core-id order.
> 
> To make this interface usable with in-order hotplug the ordering of the
> entries should be codified in the schema documentation. (see my response
> on the cover letter for justification).

I'm not really sure what you mean by this.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC 1/2] spapr: Reverse order of hotpluggable cpus list

[Peter Krempa]

On Wed, Jul 20, 2016 at 11:00:05 +1000, David Gibson wrote:
> On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote:
> > On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote:
> > > The spapr implementation of query-hotpluggable-cpus builds the list of
> > > hotpluggable cores from the end (most removed from the list head)
> > > because that's the easiest way with a singly linked list.  Because it
> > > also traverses the possible CPU cores starting from low indexes the
> > > resulting list has the cpu cores listed in reverse order by core-id.
> > > 
> > > That's not generally harmful, but it means the output from "info
> > > hotpluggable-cpus" is a little harder to read than it could be.
> > > 
> > > Therefore, traverse the cpus in reverse order so that the final list
> > > ends up in increasing-core-id order.
> > 
> > To make this interface usable with in-order hotplug the ordering of the
> > entries should be codified in the schema documentation. (see my response
> > on the cover letter for justification).
> 
> I'm not really sure what you mean by this.

Currently the order of entries in reply of query-hotpluggable-cpus is
arbitrary as this patch hints.

If qemu won't support arbitrary order hotplug but libvirt should be able
to use the new interface, then the order of replies of
query-hotpluggable-cpus need to corelate (in a documented fashion) with
the order we need to plug the cpus in. By not documenting any order
libvirt can just guess it (by reimplementing the algorithm in qemu).

I've pointed this out in a sub-thread of the cover-letter.

Peter

Re: [Qemu-devel] [RFC 1/2] spapr: Reverse order of hotpluggable cpus list

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2346 bytes --]

On Wed, Jul 20, 2016 at 09:01:11AM +0200, Peter Krempa wrote:
> On Wed, Jul 20, 2016 at 11:00:05 +1000, David Gibson wrote:
> > On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote:
> > > On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote:
> > > > The spapr implementation of query-hotpluggable-cpus builds the list of
> > > > hotpluggable cores from the end (most removed from the list head)
> > > > because that's the easiest way with a singly linked list.  Because it
> > > > also traverses the possible CPU cores starting from low indexes the
> > > > resulting list has the cpu cores listed in reverse order by core-id.
> > > > 
> > > > That's not generally harmful, but it means the output from "info
> > > > hotpluggable-cpus" is a little harder to read than it could be.
> > > > 
> > > > Therefore, traverse the cpus in reverse order so that the final list
> > > > ends up in increasing-core-id order.
> > > 
> > > To make this interface usable with in-order hotplug the ordering of the
> > > entries should be codified in the schema documentation. (see my response
> > > on the cover letter for justification).
> > 
> > I'm not really sure what you mean by this.
> 
> Currently the order of entries in reply of query-hotpluggable-cpus is
> arbitrary as this patch hints.
> 
> If qemu won't support arbitrary order hotplug but libvirt should be able
> to use the new interface, then the order of replies of
> query-hotpluggable-cpus need to corelate (in a documented fashion) with
> the order we need to plug the cpus in. By not documenting any order
> libvirt can just guess it (by reimplementing the algorithm in qemu).

> I've pointed this out in a sub-thread of the cover-letter.

Right, that's kind of the point of this patch - it makes the order
returned by query-hotpluggable-cpus match the required order for "in
order" hotplug.  That may not be necessary so - it looks like between
Igor and myself we might have an eleventh hour way of enabling
any-order hotplug.

That said, I'm still inclined to push this patch at some point for the
sake of easier to read debugging output.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC 1/2] spapr: Reverse order of hotpluggable cpus list

[Igor Mammedov]

On Thu, 21 Jul 2016 23:07:06 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jul 20, 2016 at 09:01:11AM +0200, Peter Krempa wrote:
> > On Wed, Jul 20, 2016 at 11:00:05 +1000, David Gibson wrote:  
> > > On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote:  
> > > > On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote:  
> > > > > The spapr implementation of query-hotpluggable-cpus builds the list of
> > > > > hotpluggable cores from the end (most removed from the list head)
> > > > > because that's the easiest way with a singly linked list.  Because it
> > > > > also traverses the possible CPU cores starting from low indexes the
> > > > > resulting list has the cpu cores listed in reverse order by core-id.
> > > > > 
> > > > > That's not generally harmful, but it means the output from "info
> > > > > hotpluggable-cpus" is a little harder to read than it could be.
> > > > > 
> > > > > Therefore, traverse the cpus in reverse order so that the final list
> > > > > ends up in increasing-core-id order.  
> > > > 
> > > > To make this interface usable with in-order hotplug the ordering of the
> > > > entries should be codified in the schema documentation. (see my response
> > > > on the cover letter for justification).  
> > > 
> > > I'm not really sure what you mean by this.  
> > 
> > Currently the order of entries in reply of query-hotpluggable-cpus is
> > arbitrary as this patch hints.
> > 
> > If qemu won't support arbitrary order hotplug but libvirt should be able
> > to use the new interface, then the order of replies of
> > query-hotpluggable-cpus need to corelate (in a documented fashion) with
> > the order we need to plug the cpus in. By not documenting any order
> > libvirt can just guess it (by reimplementing the algorithm in qemu).  
> 
> > I've pointed this out in a sub-thread of the cover-letter.  
> 
> Right, that's kind of the point of this patch - it makes the order
> returned by query-hotpluggable-cpus match the required order for "in
> order" hotplug.  That may not be necessary so - it looks like between
> Igor and myself we might have an eleventh hour way of enabling
> any-order hotplug.
> 
> That said, I'm still inclined to push this patch at some point for the
> sake of easier to read debugging output.
it's qmp command output intended for machine parsing, so don't really need it

Re: [Qemu-devel] [RFC 1/2] spapr: Reverse order of hotpluggable cpus list

[Greg Kurz]

On Thu, 21 Jul 2016 16:30:06 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Thu, 21 Jul 2016 23:07:06 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Jul 20, 2016 at 09:01:11AM +0200, Peter Krempa wrote:  
> > > On Wed, Jul 20, 2016 at 11:00:05 +1000, David Gibson wrote:    
> > > > On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote:    
> > > > > On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote:    
> > > > > > The spapr implementation of query-hotpluggable-cpus builds the list of
> > > > > > hotpluggable cores from the end (most removed from the list head)
> > > > > > because that's the easiest way with a singly linked list.  Because it
> > > > > > also traverses the possible CPU cores starting from low indexes the
> > > > > > resulting list has the cpu cores listed in reverse order by core-id.
> > > > > > 
> > > > > > That's not generally harmful, but it means the output from "info
> > > > > > hotpluggable-cpus" is a little harder to read than it could be.
> > > > > > 
> > > > > > Therefore, traverse the cpus in reverse order so that the final list
> > > > > > ends up in increasing-core-id order.    
> > > > > 
> > > > > To make this interface usable with in-order hotplug the ordering of the
> > > > > entries should be codified in the schema documentation. (see my response
> > > > > on the cover letter for justification).    
> > > > 
> > > > I'm not really sure what you mean by this.    
> > > 
> > > Currently the order of entries in reply of query-hotpluggable-cpus is
> > > arbitrary as this patch hints.
> > > 
> > > If qemu won't support arbitrary order hotplug but libvirt should be able
> > > to use the new interface, then the order of replies of
> > > query-hotpluggable-cpus need to corelate (in a documented fashion) with
> > > the order we need to plug the cpus in. By not documenting any order
> > > libvirt can just guess it (by reimplementing the algorithm in qemu).    
> >   
> > > I've pointed this out in a sub-thread of the cover-letter.    
> > 
> > Right, that's kind of the point of this patch - it makes the order
> > returned by query-hotpluggable-cpus match the required order for "in
> > order" hotplug.  That may not be necessary so - it looks like between
> > Igor and myself we might have an eleventh hour way of enabling
> > any-order hotplug.
> > 
> > That said, I'm still inclined to push this patch at some point for the
> > sake of easier to read debugging output.  
> it's qmp command output intended for machine parsing, so don't really need it
> 

It is also hmp output as indicated in the changelog with "info hotpluggable-cpus"

Cheers.

--
Greg

Re: [Qemu-devel] [RFC 1/2] spapr: Reverse order of hotpluggable cpus list

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2767 bytes --]

On Wed, Jul 20, 2016 at 09:01:11AM +0200, Peter Krempa wrote:
> On Wed, Jul 20, 2016 at 11:00:05 +1000, David Gibson wrote:
> > On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote:
> > > On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote:
> > > > The spapr implementation of query-hotpluggable-cpus builds the list of
> > > > hotpluggable cores from the end (most removed from the list head)
> > > > because that's the easiest way with a singly linked list.  Because it
> > > > also traverses the possible CPU cores starting from low indexes the
> > > > resulting list has the cpu cores listed in reverse order by core-id.
> > > > 
> > > > That's not generally harmful, but it means the output from "info
> > > > hotpluggable-cpus" is a little harder to read than it could be.
> > > > 
> > > > Therefore, traverse the cpus in reverse order so that the final list
> > > > ends up in increasing-core-id order.
> > > 
> > > To make this interface usable with in-order hotplug the ordering of the
> > > entries should be codified in the schema documentation. (see my response
> > > on the cover letter for justification).
> > 
> > I'm not really sure what you mean by this.
> 
> Currently the order of entries in reply of query-hotpluggable-cpus is
> arbitrary as this patch hints.
> 
> If qemu won't support arbitrary order hotplug but libvirt should be able
> to use the new interface, then the order of replies of
> query-hotpluggable-cpus need to corelate (in a documented fashion) with
> the order we need to plug the cpus in. By not documenting any order
> libvirt can just guess it (by reimplementing the algorithm in qemu).
> 
> I've pointed this out in a sub-thread of the cover-letter.

So, based on Peter's comments (and others) I've concluded that the
cpu-add implementation in terms of the new interface isn't really
useful.

However, I think this re-ordering is still a good idea, because:
  * It makes info hotpluggable-cpus easier to read in the HMP

  * Although we certainly need a better approach to handling hotplug +
    NUMA, correcting the order should allow the existing NUMA
    interface to work with only as much guesswork in libvirt as we
    already have.

  * We haven't released a qemu with query-hotpluggable-cpus yet, so we
    shouldn't need version conditions in order to use the order.

For that reason I've tentatively merged this patch into ppc-for-2.7.
It would be good to get an R-b or A-b from someone before I send a
pull request (which I'm hoping to do tomorrow).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC 1/2] spapr: Reverse order of hotpluggable cpus list

[Igor Mammedov]

On Mon, 25 Jul 2016 16:09:04 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jul 20, 2016 at 09:01:11AM +0200, Peter Krempa wrote:
> > On Wed, Jul 20, 2016 at 11:00:05 +1000, David Gibson wrote:  
> > > On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote:  
> > > > On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote:  
> > > > > The spapr implementation of query-hotpluggable-cpus builds the list of
> > > > > hotpluggable cores from the end (most removed from the list head)
> > > > > because that's the easiest way with a singly linked list.  Because it
> > > > > also traverses the possible CPU cores starting from low indexes the
> > > > > resulting list has the cpu cores listed in reverse order by core-id.
> > > > > 
> > > > > That's not generally harmful, but it means the output from "info
> > > > > hotpluggable-cpus" is a little harder to read than it could be.
> > > > > 
> > > > > Therefore, traverse the cpus in reverse order so that the final list
> > > > > ends up in increasing-core-id order.  
> > > > 
> > > > To make this interface usable with in-order hotplug the ordering of the
> > > > entries should be codified in the schema documentation. (see my response
> > > > on the cover letter for justification).  
> > > 
> > > I'm not really sure what you mean by this.  
> > 
> > Currently the order of entries in reply of query-hotpluggable-cpus is
> > arbitrary as this patch hints.
> > 
> > If qemu won't support arbitrary order hotplug but libvirt should be able
> > to use the new interface, then the order of replies of
> > query-hotpluggable-cpus need to corelate (in a documented fashion) with
> > the order we need to plug the cpus in. By not documenting any order
> > libvirt can just guess it (by reimplementing the algorithm in qemu).
> > 
> > I've pointed this out in a sub-thread of the cover-letter.  
> 
> So, based on Peter's comments (and others) I've concluded that the
> cpu-add implementation in terms of the new interface isn't really
> useful.
> 
> However, I think this re-ordering is still a good idea, because:
>   * It makes info hotpluggable-cpus easier to read in the HMP
> 
>   * Although we certainly need a better approach to handling hotplug +
>     NUMA, correcting the order should allow the existing NUMA
>     interface to work with only as much guesswork in libvirt as we
>     already have.
> 
>   * We haven't released a qemu with query-hotpluggable-cpus yet, so we
>     shouldn't need version conditions in order to use the order.
I've talked with Peter and meanwhile (till we have sane NUMA interface)
libvirt will be guessing ids based on query-hotpluggable-cpus output
sorted by socket/core/thread-id, so forward/reverse order won't really
matter to it.

Forward sorting is fine wrt 'info hotpluggable-cpus', however it might
be better to do sorting in HMP callback so that each target won't have
to do it nor regress command output in future or introduce another
ordering.

Considering that ordering affects only HMP won't require any compat
glue even if it's not fixed in 2.7 release.

> 
> For that reason I've tentatively merged this patch into ppc-for-2.7.
> It would be good to get an R-b or A-b from someone before I send a
> pull request (which I'm hoping to do tomorrow).
> 

Re: [Qemu-devel] [RFC 1/2] spapr: Reverse order of hotpluggable cpus list

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3788 bytes --]

On Mon, Jul 25, 2016 at 10:14:18AM +0200, Igor Mammedov wrote:
> On Mon, 25 Jul 2016 16:09:04 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Jul 20, 2016 at 09:01:11AM +0200, Peter Krempa wrote:
> > > On Wed, Jul 20, 2016 at 11:00:05 +1000, David Gibson wrote:  
> > > > On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote:  
> > > > > On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote:  
> > > > > > The spapr implementation of query-hotpluggable-cpus builds the list of
> > > > > > hotpluggable cores from the end (most removed from the list head)
> > > > > > because that's the easiest way with a singly linked list.  Because it
> > > > > > also traverses the possible CPU cores starting from low indexes the
> > > > > > resulting list has the cpu cores listed in reverse order by core-id.
> > > > > > 
> > > > > > That's not generally harmful, but it means the output from "info
> > > > > > hotpluggable-cpus" is a little harder to read than it could be.
> > > > > > 
> > > > > > Therefore, traverse the cpus in reverse order so that the final list
> > > > > > ends up in increasing-core-id order.  
> > > > > 
> > > > > To make this interface usable with in-order hotplug the ordering of the
> > > > > entries should be codified in the schema documentation. (see my response
> > > > > on the cover letter for justification).  
> > > > 
> > > > I'm not really sure what you mean by this.  
> > > 
> > > Currently the order of entries in reply of query-hotpluggable-cpus is
> > > arbitrary as this patch hints.
> > > 
> > > If qemu won't support arbitrary order hotplug but libvirt should be able
> > > to use the new interface, then the order of replies of
> > > query-hotpluggable-cpus need to corelate (in a documented fashion) with
> > > the order we need to plug the cpus in. By not documenting any order
> > > libvirt can just guess it (by reimplementing the algorithm in qemu).
> > > 
> > > I've pointed this out in a sub-thread of the cover-letter.  
> > 
> > So, based on Peter's comments (and others) I've concluded that the
> > cpu-add implementation in terms of the new interface isn't really
> > useful.
> > 
> > However, I think this re-ordering is still a good idea, because:
> >   * It makes info hotpluggable-cpus easier to read in the HMP
> > 
> >   * Although we certainly need a better approach to handling hotplug +
> >     NUMA, correcting the order should allow the existing NUMA
> >     interface to work with only as much guesswork in libvirt as we
> >     already have.
> > 
> >   * We haven't released a qemu with query-hotpluggable-cpus yet, so we
> >     shouldn't need version conditions in order to use the order.
> I've talked with Peter and meanwhile (till we have sane NUMA interface)
> libvirt will be guessing ids based on query-hotpluggable-cpus output
> sorted by socket/core/thread-id, so forward/reverse order won't really
> matter to it.
> 
> Forward sorting is fine wrt 'info hotpluggable-cpus', however it might
> be better to do sorting in HMP callback so that each target won't have
> to do it nor regress command output in future or introduce another
> ordering.
> 
> Considering that ordering affects only HMP won't require any compat
> glue even if it's not fixed in 2.7 release.

Hm, ok, I'll drop this patch.

> 
> > 
> > For that reason I've tentatively merged this patch into ppc-for-2.7.
> > It would be good to get an R-b or A-b from someone before I send a
> > pull request (which I'm hoping to do tomorrow).
> > 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [RFC 2/2] qmp: Implement cpu-add in terms of query-hotpluggable-cpus when available

[David Gibson]

We've recently added a new device_add based cpu hotplug
implementation, with the spapr machine type being the first user.  In
order to overcome the limitations of the old cpu_add interface, it
works very differently.  That's going to require a new interface in
libvirt to properly use the new interface in qemu, which will in turn
require work further up the stack to make use of it.

While that's certainly necessary in the long run, it would be nice if
we could have at least something working with the old cpu_add
interface.  This patch is an attempt to do so.

This patch implements cpu-add on machines which don't support it
directly but which do support the query-hotpluggable-cpus interface.
Essentially,
	cpu-add <n>
will be treated as a device_add of the n-th entry in the list provided
by query-hotpluggable-cpus.

This means that on spapr cpu-add will add a whole core, not a single
vcpu as on x86. That's arguably cheating on the cpu-add semantics, but
AFAICT those were never terribly well defined anyway.
---
 qmp.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/qmp.c b/qmp.c
index b6d531e..cde02c4 100644
--- a/qmp.c
+++ b/qmp.c
@@ -17,6 +17,7 @@
 #include "qemu-version.h"
 #include "qemu/cutils.h"
 #include "monitor/monitor.h"
+#include "monitor/qdev.h"
 #include "sysemu/sysemu.h"
 #include "qmp-commands.h"
 #include "sysemu/char.h"
@@ -29,6 +30,7 @@
 #include "sysemu/block-backend.h"
 #include "qom/qom-qobject.h"
 #include "qapi/qmp/qerror.h"
+#include "qapi/qmp/qint.h"
 #include "qapi/qmp/qobject.h"
 #include "qapi/qmp-input-visitor.h"
 #include "hw/boards.h"
@@ -133,6 +135,65 @@ void qmp_cpu(int64_t index, Error **errp)
     /* Just do nothing */
 }
 
+static void qmp_compat_cpu_add(int64_t id, Error **errp)
+{
+    Error *err = NULL;
+    MachineClass *mc = MACHINE_GET_CLASS(current_machine);
+    HotpluggableCPUList *l = mc->query_hotpluggable_cpus(current_machine);
+    HotpluggableCPUList *list_item;
+    HotpluggableCPU *cpu_item;
+    QDict *opts = NULL;
+    int64_t i;
+
+    if (!l) {
+        goto out;
+    }
+
+    for (list_item = l, i = 0;
+         list_item;
+         list_item = list_item->next, i++) {
+        if (i == id) {
+            break;
+        }
+    }
+
+    if (!list_item) {
+        error_setg(errp, "CPU id must be in the range 0..%" PRId64,
+                   i - 1);
+        goto out;
+    }
+
+    cpu_item = list_item->value;
+
+    opts = qdict_new();
+    qdict_put(opts, "driver", qstring_from_str(cpu_item->type));
+    if (cpu_item->props->has_node_id) {
+        qdict_put(opts, "node-id", qint_from_int(cpu_item->props->node_id));
+    }
+    if (cpu_item->props->has_socket_id) {
+        qdict_put(opts, "socket-id", qint_from_int(cpu_item->props->socket_id));
+    }
+    if (cpu_item->props->has_core_id) {
+        qdict_put(opts, "core-id", qint_from_int(cpu_item->props->core_id));
+    }
+    if (cpu_item->props->has_thread_id) {
+        qdict_put(opts, "thread-id", qint_from_int(cpu_item->props->thread_id));
+    }
+
+    qmp_device_add(opts, NULL, &err);
+    if (err) {
+        error_propagate(errp, err);
+    }
+
+out:
+    if (opts) {
+        QDECREF(opts);
+    }
+    if (l) {
+        qapi_free_HotpluggableCPUList(l);
+    }
+}
+
 void qmp_cpu_add(int64_t id, Error **errp)
 {
     MachineClass *mc;
@@ -140,6 +201,8 @@ void qmp_cpu_add(int64_t id, Error **errp)
     mc = MACHINE_GET_CLASS(current_machine);
     if (mc->hot_add_cpu) {
         mc->hot_add_cpu(id, errp);
+    } else if (mc->query_hotpluggable_cpus) {
+        qmp_compat_cpu_add(id, errp);
     } else {
         error_setg(errp, "Not supported");
     }
-- 
2.7.4

Re: [Qemu-devel] [RFC 2/2] qmp: Implement cpu-add in terms of query-hotpluggable-cpus when available

[Peter Krempa]

On Mon, Jul 18, 2016 at 19:19:20 +1000, David Gibson wrote:
> We've recently added a new device_add based cpu hotplug
> implementation, with the spapr machine type being the first user.  In
> order to overcome the limitations of the old cpu_add interface, it
> works very differently.  That's going to require a new interface in
> libvirt to properly use the new interface in qemu, which will in turn
> require work further up the stack to make use of it.
> 
> While that's certainly necessary in the long run, it would be nice if
> we could have at least something working with the old cpu_add
> interface.  This patch is an attempt to do so.
> 
> This patch implements cpu-add on machines which don't support it
> directly but which do support the query-hotpluggable-cpus interface.
> Essentially,
> 	cpu-add <n>
> will be treated as a device_add of the n-th entry in the list provided
> by query-hotpluggable-cpus.

Does this have the same implications when libvirt will need to re-create
a matching qemu process for migration target? (Will we need to specify
-device cpu,... for every CPU 'entity' added this way on the command
line?)

When using cpu_add libvirt recreates the process by just increasing the
number of active cpus specified via '-smp'.

In case where libvirt would need to add the -device entries this would
create just problems with compatibility. Libvirt needs to add XML
specification of the cpus in order to allow tracking which cpus need to
be added on the commandline.

> This means that on spapr cpu-add will add a whole core, not a single
> vcpu as on x86. That's arguably cheating on the cpu-add semantics, but
> AFAICT those were never terribly well defined anyway.

Peter

Re: [Qemu-devel] [RFC 2/2] qmp: Implement cpu-add in terms of query-hotpluggable-cpus when available

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2401 bytes --]

On Mon, Jul 18, 2016 at 04:01:18PM +0200, Peter Krempa wrote:
> On Mon, Jul 18, 2016 at 19:19:20 +1000, David Gibson wrote:
> > We've recently added a new device_add based cpu hotplug
> > implementation, with the spapr machine type being the first user.  In
> > order to overcome the limitations of the old cpu_add interface, it
> > works very differently.  That's going to require a new interface in
> > libvirt to properly use the new interface in qemu, which will in turn
> > require work further up the stack to make use of it.
> > 
> > While that's certainly necessary in the long run, it would be nice if
> > we could have at least something working with the old cpu_add
> > interface.  This patch is an attempt to do so.
> > 
> > This patch implements cpu-add on machines which don't support it
> > directly but which do support the query-hotpluggable-cpus interface.
> > Essentially,
> > 	cpu-add <n>
> > will be treated as a device_add of the n-th entry in the list provided
> > by query-hotpluggable-cpus.
> 
> Does this have the same implications when libvirt will need to re-create
> a matching qemu process for migration target? (Will we need to specify
> -device cpu,... for every CPU 'entity' added this way on the command
> line?)
> 
> When using cpu_add libvirt recreates the process by just increasing the
> number of active cpus specified via '-smp'.

As long as the cpus are added "in order" (which is necessary to avoid
other problems anyway), then increasing the value to -smp is
sufficient.

...but working out how much to increase -smp may be an issue.  With
Power -smp will need to be increased by #threads for each cpu-add
instead of just 1 as it is now on x86.

> In case where libvirt would need to add the -device entries this would
> create just problems with compatibility. Libvirt needs to add XML
> specification of the cpus in order to allow tracking which cpus need to
> be added on the commandline.
> 
> > This means that on spapr cpu-add will add a whole core, not a single
> > vcpu as on x86. That's arguably cheating on the cpu-add semantics, but
> > AFAICT those were never terribly well defined anyway.
> 
> Peter
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC 2/2] qmp: Implement cpu-add in terms of query-hotpluggable-cpus when available

[Bharata B Rao]

On Mon, Jul 18, 2016 at 04:01:18PM +0200, Peter Krempa wrote:
> On Mon, Jul 18, 2016 at 19:19:20 +1000, David Gibson wrote:
> > We've recently added a new device_add based cpu hotplug
> > implementation, with the spapr machine type being the first user.  In
> > order to overcome the limitations of the old cpu_add interface, it
> > works very differently.  That's going to require a new interface in
> > libvirt to properly use the new interface in qemu, which will in turn
> > require work further up the stack to make use of it.
> > 
> > While that's certainly necessary in the long run, it would be nice if
> > we could have at least something working with the old cpu_add
> > interface.  This patch is an attempt to do so.
> > 
> > This patch implements cpu-add on machines which don't support it
> > directly but which do support the query-hotpluggable-cpus interface.
> > Essentially,
> > 	cpu-add <n>
> > will be treated as a device_add of the n-th entry in the list provided
> > by query-hotpluggable-cpus.
> 
> Does this have the same implications when libvirt will need to re-create
> a matching qemu process for migration target? (Will we need to specify
> -device cpu,... for every CPU 'entity' added this way on the command
> line?)
> 
> When using cpu_add libvirt recreates the process by just increasing the
> number of active cpus specified via '-smp'.
> 
> In case where libvirt would need to add the -device entries this would
> create just problems with compatibility. Libvirt needs to add XML
> specification of the cpus in order to allow tracking which cpus need to
> be added on the commandline.

Is this compatability handling any different from how libvirt already
handles memory specified by -m and by -device pc-dimm ? Isn't cpus specified
by -smp and by -device kind of comparable to the memory case ?

Regards,
Bharata.

Re: [Qemu-devel] [RFC 2/2] qmp: Implement cpu-add in terms of query-hotpluggable-cpus when available

[Igor Mammedov]

On Tue, 19 Jul 2016 09:32:08 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Mon, Jul 18, 2016 at 04:01:18PM +0200, Peter Krempa wrote:
> > On Mon, Jul 18, 2016 at 19:19:20 +1000, David Gibson wrote:  
> > > We've recently added a new device_add based cpu hotplug
> > > implementation, with the spapr machine type being the first user.  In
> > > order to overcome the limitations of the old cpu_add interface, it
> > > works very differently.  That's going to require a new interface in
> > > libvirt to properly use the new interface in qemu, which will in turn
> > > require work further up the stack to make use of it.
> > > 
> > > While that's certainly necessary in the long run, it would be nice if
> > > we could have at least something working with the old cpu_add
> > > interface.  This patch is an attempt to do so.
> > > 
> > > This patch implements cpu-add on machines which don't support it
> > > directly but which do support the query-hotpluggable-cpus interface.
> > > Essentially,
> > > 	cpu-add <n>
> > > will be treated as a device_add of the n-th entry in the list provided
> > > by query-hotpluggable-cpus.  
> > 
> > Does this have the same implications when libvirt will need to re-create
> > a matching qemu process for migration target? (Will we need to specify
> > -device cpu,... for every CPU 'entity' added this way on the command
> > line?)
> > 
> > When using cpu_add libvirt recreates the process by just increasing the
> > number of active cpus specified via '-smp'.
> > 
> > In case where libvirt would need to add the -device entries this would
> > create just problems with compatibility. Libvirt needs to add XML
> > specification of the cpus in order to allow tracking which cpus need to
> > be added on the commandline.  
> 
> Is this compatability handling any different from how libvirt already
> handles memory specified by -m and by -device pc-dimm ? Isn't cpus specified
> by -smp and by -device kind of comparable to the memory case ?
it is not,
when one hotplugs pc-dimm, then on target side -m keeps the same values as
on source, the only change to target CLI is added -device for hotplugged memory.
Behavior is basically the same as with any other device that could be hotplugged
and migrated.

In cpu-add case, semantics are different. CLI option -smp X on source becomes
-smp (x+#hotplugge_cpus) since there were not means to create CPUs using -device
and code that is creating initial CPUs would create additional CPUs that were added
on source. Hence inability to add any random CPU (only the next free) so that target
could reproduce setup.

After enabling -device/device_add for cpus we can ditch cpu-add in favor of
usual -device semantics where -smp X on target stays the same as on source
and we manipulate only with -device for hotplugged cpus.


I'm not sure that enabling cpu-add for spapr would bring simplify life of
libvirt.

> 
> Regards,
> Bharata.
> 
> 

Re: [Qemu-devel] [RFC 2/2] qmp: Implement cpu-add in terms of query-hotpluggable-cpus when available

[Peter Krempa]

On Tue, Jul 19, 2016 at 09:51:00 +0200, Igor Mammedov wrote:
> On Tue, 19 Jul 2016 09:32:08 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > On Mon, Jul 18, 2016 at 04:01:18PM +0200, Peter Krempa wrote:
> > > On Mon, Jul 18, 2016 at 19:19:20 +1000, David Gibson wrote:  

[...]

> > Is this compatability handling any different from how libvirt already
> > handles memory specified by -m and by -device pc-dimm ? Isn't cpus specified
> > by -smp and by -device kind of comparable to the memory case ?
> it is not,
> when one hotplugs pc-dimm, then on target side -m keeps the same values as
> on source, the only change to target CLI is added -device for hotplugged memory.
> Behavior is basically the same as with any other device that could be hotplugged
> and migrated.
> 
> In cpu-add case, semantics are different. CLI option -smp X on source becomes
> -smp (x+#hotplugge_cpus) since there were not means to create CPUs using -device
> and code that is creating initial CPUs would create additional CPUs that were added
> on source. Hence inability to add any random CPU (only the next free) so that target
> could reproduce setup.
> 
> After enabling -device/device_add for cpus we can ditch cpu-add in favor of
> usual -device semantics where -smp X on target stays the same as on source
> and we manipulate only with -device for hotplugged cpus.
> 
> 
> I'm not sure that enabling cpu-add for spapr would bring simplify life of
> libvirt.

Agreed. As the device_add interface looks very alike this one it makes
little sense to add this interface (given that qemu will be able to
provide enough information to allow selecting the correct entry of
query-hotpluggable-cpus to add in sequence).

Re: [Qemu-devel] [RFC 0/2] cpu-add compatibility for query-hotpluggable-cpus implementations

[Peter Krempa]

On Mon, Jul 18, 2016 at 19:19:18 +1000, David Gibson wrote:
> I'm not entirely sure if this is a good idea, and if it is whether
> this is a good approach to it.  But I'd like to discuss it and see if
> anyone has better ideas.
> 
> As you may know we've hit a bunch of complications with cpu_index
> which will impose some limitations with what we can do with the new
> query-hotpluggable-cpus interface, and we've run out of time to
> address these in qemu-2.7.
>
> At the same time we're hitting complications with the fact that the
> new qemu interface requires a new libvirt interface to use properly,
> and that has follow on effects further up the stack.

The libvirt interface is basically now depending on adding a working
implementation for qemu or a different hypervisor. APIs without
implementation are not accepted upstream.

It looks like there are the following problems which make the above
hard:

First of the problem is the missing link between the NUMA topology
(currently confirured via 'cpu id' which is not linked in any way to the
query-hotpluggable-cpus entries). This basically means that I'll have to
re-implement the qemu numbering scheme and hope that it doesn't change
until a better approach is added.

Secondly from my understanding of the current state it's impossible to
select an arbitrary cpu to hotplug but they need to happen 'in order' of
the cpu id pointed out above (which is not accessible). The grand plan
is to allow adding the cpus in any order. This makes the feature look
like a proof of concept rather than something useful.

The two problems above make this feature hard to implement and hard to
sell to libvirt's upstream.

> Together this means a bunch more delays to having usable CPU hotplug
> on Power for downstream users, which is unfortunate.

I'm not in favor of adding upstream hacks for sake of downstream
deadlines.

> This is an attempt to get something limited working in a shorter time
> frame, by implementing the old cpu-add interface in terms of the new
> interface.  Obviously this can't fully exploit the new interface's
> capabilities, but you can do basic in-order cpu hotplug without removal.

As a side note, cpu-add technically allows out of order usage. Libvirt
didn't use it that way though.

> To make this work, I need to broaden the semantics of cpu-add: it will
> a single entry from query-hotpluggable-cpus, which means it may add
> multiple vcpus, which the x86 implementation did not previously do.

See my response to 2/2. If this requires to add -device for the
hotplugged entries when migrating it basically doesn't help at all.

> I'm not sure if the intended semantics of cpu-add were ever defined
> well enough to say if this is "wrong" or not.

For x86 I'll also need to experiment with the combined use of cpu-add
and device_add interfaces. I plan to add a implementation which
basically uses the old API in libvirt but calls the new APIs in qemu if
they were used previously. (We still need to fall back to the old API
for migration compatibility)

> Because of this, I suspect libvirt will still need some work, but I'm
> hoping it might be less that the full new API implementation.

Mostly as adding a single entry via the interface will result in
multiple entries in query-cpus. Also libvirt's interface takes the
target number of vcpus as argument so any increment that is not
divisible by the thread count needs to be rejected.

Peter

Re: [Qemu-devel] [RFC 0/2] cpu-add compatibility for query-hotpluggable-cpus implementations

[Igor Mammedov]

On Mon, 18 Jul 2016 17:06:18 +0200
Peter Krempa <pkrempa@redhat.com> wrote:

> On Mon, Jul 18, 2016 at 19:19:18 +1000, David Gibson wrote:
> > I'm not entirely sure if this is a good idea, and if it is whether
> > this is a good approach to it.  But I'd like to discuss it and see if
> > anyone has better ideas.
> > 
> > As you may know we've hit a bunch of complications with cpu_index
> > which will impose some limitations with what we can do with the new
> > query-hotpluggable-cpus interface, and we've run out of time to
> > address these in qemu-2.7.
> >
> > At the same time we're hitting complications with the fact that the
> > new qemu interface requires a new libvirt interface to use properly,
> > and that has follow on effects further up the stack.  
> 
> The libvirt interface is basically now depending on adding a working
> implementation for qemu or a different hypervisor. APIs without
> implementation are not accepted upstream.
> 
> It looks like there are the following problems which make the above
> hard:
> 
> First of the problem is the missing link between the NUMA topology
> (currently confirured via 'cpu id' which is not linked in any way to the
> query-hotpluggable-cpus entries). This basically means that I'll have to
> re-implement the qemu numbering scheme and hope that it doesn't change
> until a better approach is added.
with current 'in order' plug/unplug limitation behavior is the same as
for cpu-add (wrt x86) so device_add could be used as direct replacement
of cpu-add in NUMA case.

Numa node to CPU in query-hotpluggable-cpus a missing part
but once numa mapping for hotplugged CPUs (which is broken now) is fixed
(fix https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00595.html)
I'll be ready to extend x86.query-hotpluggable-cpus with numa mapping
that -numa cpus=1,2,3... happened to configure.
(note: that device_add cpu,node=X that doesn't match whatever has been
configured with -numa cpus=... will rise error, as numa configuration
is static and fixed at VM creation time, meaning that "node" option
in query-hotpluggable-cpus is optional and only to inform users to
which node cpu belongs)

> Secondly from my understanding of the current state it's impossible to
> select an arbitrary cpu to hotplug but they need to happen 'in order' of
> the cpu id pointed out above (which is not accessible). The grand plan
> is to allow adding the cpus in any order. This makes the feature look
> like a proof of concept rather than something useful.
having out-of-order plug/unplug would be nice but that wasn't
the grand plan. Main reason is to replace cpu-add with 'device_add cpu' and
on top of that provide support for 'device_del cpu' instead of adding cpu-del
command.
And as result of migration to device_add to avoid changing -smp to match
present cpus count on target and reuse the same interface as other devices.

We can still pick 'out of order' device_add cpu using migration_id patch
and revert in-order limit patch. It would work for x86,
but I think there were issues with SPAPR, that's why I'm in favor of
in-order limit approach.

> The two problems above make this feature hard to implement and hard to
> sell to libvirt's upstream.
> 
> > Together this means a bunch more delays to having usable CPU hotplug
> > on Power for downstream users, which is unfortunate.  
> 
> I'm not in favor of adding upstream hacks for sake of downstream
> deadlines.
> 
> > This is an attempt to get something limited working in a shorter time
> > frame, by implementing the old cpu-add interface in terms of the new
> > interface.  Obviously this can't fully exploit the new interface's
> > capabilities, but you can do basic in-order cpu hotplug without removal.  
> 
> As a side note, cpu-add technically allows out of order usage. Libvirt
> didn't use it that way though.
out-of-order cpu-add breaks migration that's why it's not been used.

> > To make this work, I need to broaden the semantics of cpu-add: it will
> > a single entry from query-hotpluggable-cpus, which means it may add
> > multiple vcpus, which the x86 implementation did not previously do.  
> 
> See my response to 2/2. If this requires to add -device for the
> hotplugged entries when migrating it basically doesn't help at all.
> 
> > I'm not sure if the intended semantics of cpu-add were ever defined
> > well enough to say if this is "wrong" or not.  
> 
> For x86 I'll also need to experiment with the combined use of cpu-add
> and device_add interfaces.
It should work, though I'd not recommend to use them together as cpu-add
will be obsoleted eventually.

>I plan to add a implementation which
> basically uses the old API in libvirt but calls the new APIs in qemu if
> they were used previously. 
(skip)

>(We still need to fall back to the old API for migration compatibility)
Why?

> 
> > Because of this, I suspect libvirt will still need some work, but I'm
> > hoping it might be less that the full new API implementation.  
> 
> Mostly as adding a single entry via the interface will result in
> multiple entries in query-cpus. Also libvirt's interface takes the
> target number of vcpus as argument so any increment that is not
> divisible by the thread count needs to be rejected.
> 
> Peter
> 

Re: [Qemu-devel] [RFC 0/2] cpu-add compatibility for query-hotpluggable-cpus implementations

[Bharata B Rao]

On Mon, Jul 18, 2016 at 06:20:35PM +0200, Igor Mammedov wrote:
> On Mon, 18 Jul 2016 17:06:18 +0200
> Peter Krempa <pkrempa@redhat.com> wrote:
> 
> > On Mon, Jul 18, 2016 at 19:19:18 +1000, David Gibson wrote:
> > > I'm not entirely sure if this is a good idea, and if it is whether
> > > this is a good approach to it.  But I'd like to discuss it and see if
> > > anyone has better ideas.
> > > 
> > > As you may know we've hit a bunch of complications with cpu_index
> > > which will impose some limitations with what we can do with the new
> > > query-hotpluggable-cpus interface, and we've run out of time to
> > > address these in qemu-2.7.
> > >
> > > At the same time we're hitting complications with the fact that the
> > > new qemu interface requires a new libvirt interface to use properly,
> > > and that has follow on effects further up the stack.  
> > 
> > The libvirt interface is basically now depending on adding a working
> > implementation for qemu or a different hypervisor. APIs without
> > implementation are not accepted upstream.
> > 
> > It looks like there are the following problems which make the above
> > hard:
> > 
> > First of the problem is the missing link between the NUMA topology
> > (currently confirured via 'cpu id' which is not linked in any way to the
> > query-hotpluggable-cpus entries). This basically means that I'll have to
> > re-implement the qemu numbering scheme and hope that it doesn't change
> > until a better approach is added.
> with current 'in order' plug/unplug limitation behavior is the same as
> for cpu-add (wrt x86) so device_add could be used as direct replacement
> of cpu-add in NUMA case.
> 
> Numa node to CPU in query-hotpluggable-cpus a missing part
> but once numa mapping for hotplugged CPUs (which is broken now) is fixed
> (fix https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00595.html)
> I'll be ready to extend x86.query-hotpluggable-cpus with numa mapping
> that -numa cpus=1,2,3... happened to configure.
> (note: that device_add cpu,node=X that doesn't match whatever has been
> configured with -numa cpus=... will rise error, as numa configuration
> is static and fixed at VM creation time, meaning that "node" option
> in query-hotpluggable-cpus is optional and only to inform users to
> which node cpu belongs)
> 
> > Secondly from my understanding of the current state it's impossible to
> > select an arbitrary cpu to hotplug but they need to happen 'in order' of
> > the cpu id pointed out above (which is not accessible). The grand plan
> > is to allow adding the cpus in any order. This makes the feature look
> > like a proof of concept rather than something useful.

> having out-of-order plug/unplug would be nice but that wasn't
> the grand plan. Main reason is to replace cpu-add with 'device_add cpu' and
> on top of that provide support for 'device_del cpu' instead of adding cpu-del
> command.
> And as result of migration to device_add to avoid changing -smp to match
> present cpus count on target and reuse the same interface as other devices.
> 
> We can still pick 'out of order' device_add cpu using migration_id patch
> and revert in-order limit patch. It would work for x86,
> but I think there were issues with SPAPR, that's why I'm in favor of
> in-order limit approach.

Not that the migration_id patch doesn't work for sPAPR, but it was felt
that having too many IDs (cpu_dt_id, arch_id, migration_id) is not
good/idea/preferable and could cause confusion.

I am not clear as to why limiting the out-of-order hotplug is a show
stopper for libvirt actually. Isn't that how it is for cpu-add currently ?

Regards,
Bharata.

Re: [Qemu-devel] [RFC 0/2] cpu-add compatibility for query-hotpluggable-cpus implementations

[Igor Mammedov]

On Tue, 19 Jul 2016 09:58:59 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Mon, Jul 18, 2016 at 06:20:35PM +0200, Igor Mammedov wrote:
> > On Mon, 18 Jul 2016 17:06:18 +0200
> > Peter Krempa <pkrempa@redhat.com> wrote:
> >   
> > > On Mon, Jul 18, 2016 at 19:19:18 +1000, David Gibson wrote:  
> > > > I'm not entirely sure if this is a good idea, and if it is whether
> > > > this is a good approach to it.  But I'd like to discuss it and see if
> > > > anyone has better ideas.
> > > > 
> > > > As you may know we've hit a bunch of complications with cpu_index
> > > > which will impose some limitations with what we can do with the new
> > > > query-hotpluggable-cpus interface, and we've run out of time to
> > > > address these in qemu-2.7.
> > > >
> > > > At the same time we're hitting complications with the fact that the
> > > > new qemu interface requires a new libvirt interface to use properly,
> > > > and that has follow on effects further up the stack.    
> > > 
> > > The libvirt interface is basically now depending on adding a working
> > > implementation for qemu or a different hypervisor. APIs without
> > > implementation are not accepted upstream.
> > > 
> > > It looks like there are the following problems which make the above
> > > hard:
> > > 
> > > First of the problem is the missing link between the NUMA topology
> > > (currently confirured via 'cpu id' which is not linked in any way to the
> > > query-hotpluggable-cpus entries). This basically means that I'll have to
> > > re-implement the qemu numbering scheme and hope that it doesn't change
> > > until a better approach is added.  
> > with current 'in order' plug/unplug limitation behavior is the same as
> > for cpu-add (wrt x86) so device_add could be used as direct replacement
> > of cpu-add in NUMA case.
> > 
> > Numa node to CPU in query-hotpluggable-cpus a missing part
> > but once numa mapping for hotplugged CPUs (which is broken now) is fixed
> > (fix https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00595.html)
> > I'll be ready to extend x86.query-hotpluggable-cpus with numa mapping
> > that -numa cpus=1,2,3... happened to configure.
> > (note: that device_add cpu,node=X that doesn't match whatever has been
> > configured with -numa cpus=... will rise error, as numa configuration
> > is static and fixed at VM creation time, meaning that "node" option
> > in query-hotpluggable-cpus is optional and only to inform users to
> > which node cpu belongs)
> >   
> > > Secondly from my understanding of the current state it's impossible to
> > > select an arbitrary cpu to hotplug but they need to happen 'in order' of
> > > the cpu id pointed out above (which is not accessible). The grand plan
> > > is to allow adding the cpus in any order. This makes the feature look
> > > like a proof of concept rather than something useful.  
> 
> > having out-of-order plug/unplug would be nice but that wasn't
> > the grand plan. Main reason is to replace cpu-add with 'device_add cpu' and
> > on top of that provide support for 'device_del cpu' instead of adding cpu-del
> > command.
> > And as result of migration to device_add to avoid changing -smp to match
> > present cpus count on target and reuse the same interface as other devices.
> > 
> > We can still pick 'out of order' device_add cpu using migration_id patch
> > and revert in-order limit patch. It would work for x86,
> > but I think there were issues with SPAPR, that's why I'm in favor of
> > in-order limit approach.  
> 
> Not that the migration_id patch doesn't work for sPAPR, but it was felt
> that having too many IDs (cpu_dt_id, arch_id, migration_id) is not
> good/idea/preferable and could cause confusion.
migration_id is internal thing and doesn't concern libvirt at all,
so it will be only QEMU thing that we can deal with later either by
eliminating cpu_index and leaving migration_id only or merging them
into one id after cpu_index refactoring.

> I am not clear as to why limiting the out-of-order hotplug is a show
> stopper for libvirt actually. Isn't that how it is for cpu-add currently ?
It's not show stopper but as Eric pointed out there is a caveat.
If we ship limited device_add then we would need to extend external
interface to report that out-of-order creation is available.

Looking from that point of view it's better to go migration_id route
keeping external API simple if spapr is able to handle out-of-order
cpu creation and migration.


> 
> Regards,
> Bharata.
> 

Re: [Qemu-devel] [RFC 0/2] cpu-add compatibility for query-hotpluggable-cpus implementations

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4255 bytes --]

On Tue, Jul 19, 2016 at 09:58:59AM +0530, Bharata B Rao wrote:
> On Mon, Jul 18, 2016 at 06:20:35PM +0200, Igor Mammedov wrote:
> > On Mon, 18 Jul 2016 17:06:18 +0200
> > Peter Krempa <pkrempa@redhat.com> wrote:
> > 
> > > On Mon, Jul 18, 2016 at 19:19:18 +1000, David Gibson wrote:
> > > > I'm not entirely sure if this is a good idea, and if it is whether
> > > > this is a good approach to it.  But I'd like to discuss it and see if
> > > > anyone has better ideas.
> > > > 
> > > > As you may know we've hit a bunch of complications with cpu_index
> > > > which will impose some limitations with what we can do with the new
> > > > query-hotpluggable-cpus interface, and we've run out of time to
> > > > address these in qemu-2.7.
> > > >
> > > > At the same time we're hitting complications with the fact that the
> > > > new qemu interface requires a new libvirt interface to use properly,
> > > > and that has follow on effects further up the stack.  
> > > 
> > > The libvirt interface is basically now depending on adding a working
> > > implementation for qemu or a different hypervisor. APIs without
> > > implementation are not accepted upstream.
> > > 
> > > It looks like there are the following problems which make the above
> > > hard:
> > > 
> > > First of the problem is the missing link between the NUMA topology
> > > (currently confirured via 'cpu id' which is not linked in any way to the
> > > query-hotpluggable-cpus entries). This basically means that I'll have to
> > > re-implement the qemu numbering scheme and hope that it doesn't change
> > > until a better approach is added.
> > with current 'in order' plug/unplug limitation behavior is the same as
> > for cpu-add (wrt x86) so device_add could be used as direct replacement
> > of cpu-add in NUMA case.
> > 
> > Numa node to CPU in query-hotpluggable-cpus a missing part
> > but once numa mapping for hotplugged CPUs (which is broken now) is fixed
> > (fix https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00595.html)
> > I'll be ready to extend x86.query-hotpluggable-cpus with numa mapping
> > that -numa cpus=1,2,3... happened to configure.
> > (note: that device_add cpu,node=X that doesn't match whatever has been
> > configured with -numa cpus=... will rise error, as numa configuration
> > is static and fixed at VM creation time, meaning that "node" option
> > in query-hotpluggable-cpus is optional and only to inform users to
> > which node cpu belongs)
> > 
> > > Secondly from my understanding of the current state it's impossible to
> > > select an arbitrary cpu to hotplug but they need to happen 'in order' of
> > > the cpu id pointed out above (which is not accessible). The grand plan
> > > is to allow adding the cpus in any order. This makes the feature look
> > > like a proof of concept rather than something useful.
> 
> > having out-of-order plug/unplug would be nice but that wasn't
> > the grand plan. Main reason is to replace cpu-add with 'device_add cpu' and
> > on top of that provide support for 'device_del cpu' instead of adding cpu-del
> > command.
> > And as result of migration to device_add to avoid changing -smp to match
> > present cpus count on target and reuse the same interface as other devices.
> > 
> > We can still pick 'out of order' device_add cpu using migration_id patch
> > and revert in-order limit patch. It would work for x86,
> > but I think there were issues with SPAPR, that's why I'm in favor of
> > in-order limit approach.
> 
> Not that the migration_id patch doesn't work for sPAPR, but it was felt
> that having too many IDs (cpu_dt_id, arch_id, migration_id) is not
> good/idea/preferable and could cause confusion.

I was also concerned that adding another id would be yet another layer
of things we needed to maintain compatibility with in future.

> I am not clear as to why limiting the out-of-order hotplug is a show
> stopper for libvirt actually. Isn't that how it is for cpu-add currently ?
> 
> Regards,
> Bharata.
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC 0/2] cpu-add compatibility for query-hotpluggable-cpus implementations

[Peter Krempa]

On Mon, Jul 18, 2016 at 18:20:35 +0200, Igor Mammedov wrote:
> On Mon, 18 Jul 2016 17:06:18 +0200
> Peter Krempa <pkrempa@redhat.com> wrote:
> > On Mon, Jul 18, 2016 at 19:19:18 +1000, David Gibson wrote:

[...]

> > First of the problem is the missing link between the NUMA topology
> > (currently confirured via 'cpu id' which is not linked in any way to the
> > query-hotpluggable-cpus entries). This basically means that I'll have to
> > re-implement the qemu numbering scheme and hope that it doesn't change
> > until a better approach is added.
> with current 'in order' plug/unplug limitation behavior is the same as
> for cpu-add (wrt x86) so device_add could be used as direct replacement
> of cpu-add in NUMA case.
> 
> Numa node to CPU in query-hotpluggable-cpus a missing part
> but once numa mapping for hotplugged CPUs (which is broken now) is fixed
> (fix https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00595.html)
> I'll be ready to extend x86.query-hotpluggable-cpus with numa mapping
> that -numa cpus=1,2,3... happened to configure.

So this is one instance where we need to know the relation between the
cpu 'number' and the entry in query-hotpluggable-cpus. (see below ...)

I'm aware though that for the NUMA mapping there is a plan to do it
differently in an upcomming release so that may eventually be solved

> (note: that device_add cpu,node=X that doesn't match whatever has been
> configured with -numa cpus=... will rise error, as numa configuration
> is static and fixed at VM creation time, meaning that "node" option
> in query-hotpluggable-cpus is optional and only to inform users to
> which node cpu belongs)

That is okay in this regard, I'm not planing on modifying the
configuration once we start it. We although need to allow keeping an
arbitrary configuration as it is possible now.

> > Secondly from my understanding of the current state it's impossible to
> > select an arbitrary cpu to hotplug but they need to happen 'in order' of
> > the cpu id pointed out above (which is not accessible). The grand plan
> > is to allow adding the cpus in any order. This makes the feature look
> > like a proof of concept rather than something useful.
> having out-of-order plug/unplug would be nice but that wasn't

Not only nice but it's really necessary for NUMA enabled guests so that
we can plug cpus into a given node. Otherwise NUMA guests can't take any
advantage of this since you can't control where to add vcpus.

> the grand plan. Main reason is to replace cpu-add with 'device_add cpu' and
> on top of that provide support for 'device_del cpu' instead of adding cpu-del
> command.

Unfortunately combination of the following:

- necessity to plug the vcpus in a certain order

- query-hotpluggable-cpus not reporting any ordering information

- order of entries in query-hotpluggable-cpus is arbitrary
  The documentation doesn't codify any ordering of the entries. This
  series also contains a patch that changes the order, thus the order
  information is unreliable.

make the interface unusable as-is.

With the interface there isn't a certain way how we could select the
correct entry to plug. We can just guess (basically reimplement qemu's
algorithm for numbering the cpus).

By codifying the order of entries (in any order, but it shall not be
changed afterthat) or numbering the entries we can at least eliminate the
guessing it would be possible to actually use this. (This basically
means that either the order or a index will in the end encode the
information that I've requested earlier [1])

As for the NUMA node numbering we can still guess (reimplement qemu's
algorithm for the numbering) with the data scraped from the above
information (thus basically infer the 'index' of the cpus [1]). This can
be later changed to the new interface once it will be done.

The gist of the above is that by disallowing arbitrary order of hotplug
you basically need to tell libvirt the 'index' of the cpus either
directly or indirectly by inference from the order of entries in
query-hotpluggable-cpus and the 'vcpus-count' field.

> And as result of migration to device_add to avoid changing -smp to match
> present cpus count on target and reuse the same interface as other devices.
> 
> We can still pick 'out of order' device_add cpu using migration_id patch
> and revert in-order limit patch. It would work for x86,
> but I think there were issues with SPAPR, that's why I'm in favor of
> in-order limit approach.

[...]

> > > To make this work, I need to broaden the semantics of cpu-add: it will
> > > a single entry from query-hotpluggable-cpus, which means it may add
> > > multiple vcpus, which the x86 implementation did not previously do.  
> > 
> > See my response to 2/2. If this requires to add -device for the
> > hotplugged entries when migrating it basically doesn't help at all.
> > 
> > > I'm not sure if the intended semantics of cpu-add were ever defined
> > > well enough to say if this is "wrong" or not.  
> > 
> > For x86 I'll also need to experiment with the combined use of cpu-add
> > and device_add interfaces.
> It should work, though I'd not recommend to use them together as cpu-add
> will be obsoleted eventually.

That makes sense now that I know how it's supposed to work.

> >I plan to add a implementation which
> > basically uses the old API in libvirt but calls the new APIs in qemu if
> > they were used previously. 
> (skip)
> 
> >(We still need to fall back to the old API for migration compatibility)
> Why?

Now that I've read the other responses I see that we don't need to do it
any more.

Peter

[1]
https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg01710.html

Re: [Qemu-devel] [RFC 0/2] cpu-add compatibility for query-hotpluggable-cpus implementations

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4546 bytes --]

On Mon, Jul 18, 2016 at 05:06:18PM +0200, Peter Krempa wrote:
> On Mon, Jul 18, 2016 at 19:19:18 +1000, David Gibson wrote:
> > I'm not entirely sure if this is a good idea, and if it is whether
> > this is a good approach to it.  But I'd like to discuss it and see if
> > anyone has better ideas.
> > 
> > As you may know we've hit a bunch of complications with cpu_index
> > which will impose some limitations with what we can do with the new
> > query-hotpluggable-cpus interface, and we've run out of time to
> > address these in qemu-2.7.
> >
> > At the same time we're hitting complications with the fact that the
> > new qemu interface requires a new libvirt interface to use properly,
> > and that has follow on effects further up the stack.
> 
> The libvirt interface is basically now depending on adding a working
> implementation for qemu or a different hypervisor. APIs without
> implementation are not accepted upstream.
> 
> It looks like there are the following problems which make the above
> hard:
> 
> First of the problem is the missing link between the NUMA topology
> (currently confirured via 'cpu id' which is not linked in any way to the
> query-hotpluggable-cpus entries). This basically means that I'll have to
> re-implement the qemu numbering scheme and hope that it doesn't change
> until a better approach is added.

I have at least a start on how to fix this in mind, and it's the next
thing I'll work on.  However, it obviously won't be merged for qemu-2.7.

> Secondly from my understanding of the current state it's impossible to
> select an arbitrary cpu to hotplug but they need to happen 'in order' of
> the cpu id pointed out above (which is not accessible). The grand plan
> is to allow adding the cpus in any order. This makes the feature look
> like a proof of concept rather than something useful.

Alas, yes :(.  Again, I have a plan on this, but it's missed the 2.7
window.

> The two problems above make this feature hard to implement and hard to
> sell to libvirt's upstream.
> 
> > Together this means a bunch more delays to having usable CPU hotplug
> > on Power for downstream users, which is unfortunate.
> 
> I'm not in favor of adding upstream hacks for sake of downstream
> deadlines.

As a rule, I'm not either.  But if the hacks are small and isolated
enough, I think it can be reasonable.  Whether that's the case is what
I'm trying to assess here.

> > This is an attempt to get something limited working in a shorter time
> > frame, by implementing the old cpu-add interface in terms of the new
> > interface.  Obviously this can't fully exploit the new interface's
> > capabilities, but you can do basic in-order cpu hotplug without removal.
> 
> As a side note, cpu-add technically allows out of order usage. Libvirt
> didn't use it that way though.

Yes, I know.  I gather it will break migration though.  With this
patch out-of-order cpu-add will fail because of the test enforcing
in-order device_add.

> > To make this work, I need to broaden the semantics of cpu-add: it will
> > a single entry from query-hotpluggable-cpus, which means it may add
> > multiple vcpus, which the x86 implementation did not previously do.
> 
> See my response to 2/2. If this requires to add -device for the
> hotplugged entries when migrating it basically doesn't help at all.

It doesn't.  But it does require a more complex calculation of how to
increase -smp.

> > I'm not sure if the intended semantics of cpu-add were ever defined
> > well enough to say if this is "wrong" or not.
> 
> For x86 I'll also need to experiment with the combined use of cpu-add
> and device_add interfaces. I plan to add a implementation which
> basically uses the old API in libvirt but calls the new APIs in qemu if
> they were used previously. (We still need to fall back to the old API
> for migration compatibility)

> > Because of this, I suspect libvirt will still need some work, but I'm
> > hoping it might be less that the full new API implementation.
> 
> Mostly as adding a single entry via the interface will result in
> multiple entries in query-cpus. Also libvirt's interface takes the
> target number of vcpus as argument so any increment that is not
> divisible by the thread count needs to be rejected.

Yes.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]