Re: [Qemu-devel] CPU hotplug, again

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 24 Feb 2016 22:28:22 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Feb 24, 2016 at 11:48:33AM +0100, Igor Mammedov wrote:
> > On Wed, 24 Feb 2016 13:01:06 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Tue, Feb 23, 2016 at 12:18:59PM +0100, Igor Mammedov wrote:  
> > > > On Tue, 23 Feb 2016 21:05:04 +1100
> > > > David Gibson <david@gibson.dropbear.id.au> wrote:
> > > >     
> > > > > On Tue, Feb 23, 2016 at 03:10:26PM +0530, Bharata B Rao wrote:    
> > > > > > On Tue, Feb 23, 2016 at 04:24:31PM +1100, David Gibson wrote:      
> > > > > > > Hi Andreas,
> > > > > > > 
> > > > > > > I've now found (with Thomas' help) your RFC series for socket/core
> > > > > > > based cpu hotplug on x86
> > > > > > > (https://github.com/afaerber/qemu-cpu/compare/qom-cpu-x86).  It seems
> > > > > > > sensible enough as far as it goes, but doesn't seem to address a bunch
> > > > > > > of the things that I was attempting to do with the cpu-package
> > > > > > > proposal - and which we absolutely need for cpu hotplug on Power.
> > > > > > > 
> > > > > > > 1) What interface do you envisage beyond cpu_add?
> > > > > > > 
> > > > > > > The patches I see just construct extra socket and core objects, but
> > > > > > > still control hotplug (for x86) through the cpu_add interface.  That
> > > > > > > interface is absolutely unusable on Power, since it operates on a
> > > > > > > per-thread basis, whereas the PAPR guest<->host interfaces can only
> > > > > > > communicate information at a per-core granularity.
> > > > > > > 
> > > > > > > 2) When hotplugging at core or socket granularity, where would the
> > > > > > >    code to construct the individual thread objects sit?
> > > > > > > 
> > > > > > > Your series has the construction done in both the machine init path
> > > > > > > and the hotplug path.  The latter works because hotplug occurs at
> > > > > > > thread granularity.  If we're hotplugging at core or socket
> > > > > > > granularity what would do the construct?  The core/socket object
> > > > > > > itself (in instance_init?  in realize?); the hotplug handler?
> > > > > > > something else?
> > > > > > > 
> > > > > > > 3) How does the management layer determine what is pluggable?
> > > > > > > 
> > > > > > > Both the number of pluggable slots, and what it will need to do to
> > > > > > > populate them.
> > > > > > > 
> > > > > > > 4) How do we enforce that toplogies illegal for the platform can't be
> > > > > > >    constructed?      
> > > > > > 
> > > > > > 5) QOM-links
> > > > > > 
> > > > > > Andreas, You have often talked about setting up links from machine object
> > > > > > to the CPU objects. Would the below code correctly capture that idea of
> > > > > > yours ?
> > > > > > 
> > > > > > #define SPAPR_MACHINE_CPU_CORE_PROP "core"
> > > > > > 
> > > > > > /* MachineClass.init for sPAPR */
> > > > > > static void ppc_spapr_init(MachineState *machine)
> > > > > > {
> > > > > >     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> > > > > >     int spapr_smp_cores = smp_cpus / smp_threads;
> > > > > >     int spapr_max_cores = max_cpus / smp_threads;
> > > > > > 
> > > > > >     ...
> > > > > >     for (i = 0; i < spapr_max_cores; i++) {
> > > > > >         Object *obj = object_new(TYPE_SPAPR_CPU_CORE);
> > > > > >         sPAPRCPUCore *core = SPAPR_CPU_CORE(obj);
> > > > > >         char name[32];
> > > > > > 
> > > > > >         snprintf(name, sizeof(name), "%s[%d]", SPAPR_MACHINE_CPU_CORE_PROP, i);
> > > > > > 
> > > > > >         /*
> > > > > >          * Create links from machine objects to all possible cores.
> > > > > >          */
> > > > > >         object_property_add_link(OBJECT(spapr), name, TYPE_SPAPR_CPU_CORE,
> > > > > >                                  (Object **)&spapr->core[i],
> > > > > >                                  NULL, NULL, &error_abort); 
> > > > > > 
> > > > > >         /*
> > > > > >          * Set the QOM link from machine object to core object for all
> > > > > >          * boot time CPUs specified with -smp. For rest of the hotpluggable
> > > > > >          * cores this is done from the core hotplug path.
> > > > > >          */
> > > > > >         if (i < spapr_smp_cores) {
> > > > > >             object_property_set_link(OBJECT(spapr), OBJECT(core),
> > > > > >                                      SPAPR_MACHINE_CPU_CORE_PROP, &error_abort);      
> > > > > 
> > > > > I hope we can at least have a helper function to both construct the
> > > > > core and create the links, if we can't handle the link creation in the
> > > > > core object itself.
> > > > > 
> > > > > Having to open-code it in each machine sounds like a recipe for subtle
> > > > > differences in presentation between platforms, which is exactly what
> > > > > we want to avoid.    
> > > > Creating links doesn't give us much, it's just adds means for mgmt
> > > > to check how many CPUs could be hotplugged  without keeping that
> > > > state in mgmt like it's now, so links are mostly useless if one
> > > > care where CPU is being plugged in.
> > > > The rest like enumerating exiting CPUs could be done by
> > > > traversing QOM tree, links would just simplify finding
> > > > CPUs putting them at fixed namespace.    
> > > 
> > > Simplifying finding CPUs is pretty much all we intended the links for.  
> > Do mgmt really needs it? For machine it's easy to find CPUs under
> > /machine/[peripheral|unattached] by enumerating entries over there.
> > For human, one would need to implement a dedicated HMP command that
> > would do the same, so it doesn't really matter where links are
> > located.  
> 
> If we require management to go searching the whole device tree for
> cpus, I'm concerned they'll just assume they're in the x86 location
> instead, and we'll have to fix it over and over for every platform
> that puts them somewhere different.
CPUs are inherited from Device so inherited behaviour is that they
are pretty much at fixed location /machine/[peripheral|unattached]
regardless of platform QOM tree wise, like every other device.


> > > Well, and then I was expecting a different set of links to simplify
> > > enumerating all the threads in a cpu package/core/socket/whatever.
> > >   
> >   
>