Re: [Qemu-devel] [PATCH 22/22] add cpu-add qmp command and implement CPU hot-add for target-i386

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 11 Apr 2013 12:12:37 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Apr 09, 2013 at 11:05:26PM +0200, Igor Mammedov wrote:
> > On Tue, 9 Apr 2013 17:46:21 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Tue, Apr 09, 2013 at 10:19:11PM +0200, Igor Mammedov wrote:
> > > > On Fri, 5 Apr 2013 14:10:54 -0300
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > 
> > > > > On Fri, Apr 05, 2013 at 04:37:16PM +0200, Igor Mammedov wrote:
> > > > > [...]
> > > > > > diff --git a/qapi-schema.json b/qapi-schema.json
> > > > > > index db542f6..a760ed5 100644
> > > > > > --- a/qapi-schema.json
> > > > > > +++ b/qapi-schema.json
> > > > > > @@ -1387,6 +1387,17 @@
> > > > > >  { 'command': 'cpu', 'data': {'index': 'int'} }
> > > > > >  
> > > > > >  ##
> > > > > > +# @cpu-add
> > > > > > +#
> > > > > > +# Adds CPU with specified id
> > > > > > +#
> > > > > > +# @id: cpu id of CPU to be created
> > > > > 
> > > > > Can we have the semantics/constraints of "id" documented here? Is it an
> > > > > arbitrary ID chosen by the caller? Does it have to be the APIC ID? Does
> > > > it's generic function so documenting it as APIC ID is not appropriate.
> > > > 
> > > > I for sure should document it on cpu-hotplug wiki page though, for x86 use
> > > > case for starters. i.e. how to use QMP to get a list of available/free IDs.
> > > > and in which order to use them.
> > > > 
> > > > > it have to be the index of the CPU in the CPU list? How the IDs of
> > > > > existing CPUs set using "-smp" are allocated?
> > > > With current -smp implementation the same way as it was before,
> > > > and for migration to work hot-plugged CPU has to be the next unused APIC
> > > > ID in their sequence, so that target qemu could be started with "-smp n+1".
> > > 
> > > The problem is that it's hard to find out what's the APIC ID for each
> > > CPU mentioned in the command-line.
> > > 
> > > For example, if you use "-smp 18,cores=3,threads=3,maxcpus=36" thread ID
> > > will use 2 bits, core ID will use 2 bits, the APIC IDs on startup will
> > > be:
> > > 
> > > online on startup:
> > > package 0, core 0: 0 1 2
> > > package 0, core 1: 4 5 6
> > > package 0, core 2: 8 9 10
> > > package 1, core 0: 16 17 18
> > > package 1, core 1: 20 21 22
> > > package 1, core 2: 24 25 26
> > > 
> > > offline on startup:
> > > package 2, core 0: 32 33 34
> > > package 2, core 1: 36 37 38
> > > package 2, core 2: 40 41 42
> > > package 3, core 0: 48 49 50
> > > package 3, core 1: 52 53 54
> > > package 3, core 2: 56 57 58
> > > 
> > > 
> > > What should the caller do to find out the correct ID for each of the 36
> > > VCPUs? This should be clearly documented.
> > Patch 21/22 exposes all APIC IDs (including offline) as links via QOM
> > as /machine/icc-bridge/cpu[0..n]
> > And since in above sequence IDs are monotonously increasing it's enough to use
> > the next unused APIC ID to make -smp n+1 work. 
> 
> This can be one method to map CPU indexes to APIC IDs, yes. I find it
> hard to explain and hard to use, and it imposes constraints on the way
> the target-specific IDs are calculated (requiring them to be
> monotonically increasing). But it may be a reasonable solution by now.
Arbitrary CPU hotplug + migration, require ability to specify which CPU
to create on target. It's probably post CPU-unplug topic or might be
fixed with it.

> 
> I have one additional question about the icc-bridge paths: are the link
> paths on icc-bridge explicitly documented/required to be APIC IDs, or
> the caller should assume they are arbitrary IDs with no specific
> meaning?
since it's on icc-bridge, they are APIC IDs, but from user's POV I wouldn't
care and treat it as opaque.

If we do it in a generic way, then I'll say they are arbitrary IDs.

> 
> > > 
> > > 
> > > > 
> > > > But -smp along with -numa should be reworked to allow specifying guest visible
> > > > CPU IDs for arbitrary CPU hotplug to work.
> > > 
> > > I'm curious how you plan to make this work while keeping command-line
> > > compatibility. See the question I sent on my other message, about how to
> > > map the IDs used on -numa (that are "CPU indexes") to the IDs required
> > > by cpu-add.
> > It doesn't mean that we should stick to bad/insufficient interface forever. 
> > We could add new one that does it right and keep old one for a time being for
> > compatibility.
> 
> The problem is that I only see three possible kinds of CPU identifiers
> that could work in the command-line:
> 
>  * Arbitrary user-defined IDs
>  * CPU indexes (the current interface)
>  * Topology-based identifiers/paths (e.g.
>    "/machine/numa_node[0]/cpu_socket[1]/core[2]/thread[1]")
+1 to topology, others will allow to specify bad configuration

> 
> I don't think we can asily use APIC IDs on the command-line because the
> caller simply doesn't know what will be the APIC ID for each VCPU.
Agree, especially regarding -numa option, it should consume sockets instead.

user might still potentially use opaque ID in cmd line if he will add CPUs
with -device apic_id=xxx, but first he should probe qemu with the same topology
and read complete topology with IDs qemu provides.

> 
> But this is a problem we can discuss and solve later. By now, we are
> stuck with the legacy CPU-index-based interfaces.
> 
> > 
> > > 
> > > > 
> > > > when we done with QOMifying CPUs it might be possible to use -device for them
> > > > and keeping -smp for compat/shorcut purposes.
> > > > 
> > > > > 
> > > > > I am looking at the code right now to understand how this implementation
> > > > > works, but the documentation could contain or point to documentation on
> > > > > how the "id" parameter is used and interpreted.
> > > > I'll add pointer to wiki and describe there target-i386 use-case.
> > > 
> > > Thanks! Could you try to document it succintly inside qapi-schema.json
> > > as well? Maybe just a pointer to other documents would be useful.
> > It's very target specific, so separate document probably would make more sense.
> 
> The IDs are target-specific, but do we really need to make the interface
> specification/usage to be target-specific? We could have a
> target-independent interface to find out what are the available/valid
> IDs to use on cpu-add.
Answered to this in another email about -numa and showed how ID discovery
could be made in unified cross target way.

> 
> -- 
> Eduardo


-- 
Regards,
  Igor