Re: [Qemu-devel] [PATCH RFC 3/4] pc: Create sockets and cores for CPUs

[Bharata B Rao]

On Wed, Mar 25, 2015 at 10:43 PM, Andreas Färber <address@hidden> wrote:
> Am 25.03.2015 um 17:55 schrieb Bharata B Rao:
>> On Mon, Mar 23, 2015 at 11:02 PM, Andreas Färber <address@hidden> wrote:
>>> Inline realized=true from pc_new_cpu() so that the realization can be
>>> deferred, as it would otherwise create a device[n] node.
>>>
>>> Signed-off-by: Andreas Färber <address@hidden>
>>> ---
>>>  hw/i386/pc.c | 66 
>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++--------
>>>  1 file changed, 58 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index 2c48277..492c262 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -54,11 +54,14 @@
>>>  #include "exec/memory.h"
>>>  #include "exec/address-spaces.h"
>>>  #include "sysemu/arch_init.h"
>>> +#include "sysemu/cpus.h"
>>>  #include "qemu/bitmap.h"
>>>  #include "qemu/config-file.h"
>>>  #include "hw/acpi/acpi.h"
>>>  #include "hw/acpi/cpu_hotplug.h"
>>>  #include "hw/cpu/icc_bus.h"
>>> +#include "hw/i386/cpu-socket.h"
>>> +#include "hw/i386/cpu-core.h"
>>>  #include "hw/boards.h"
>>>  #include "hw/pci/pci_host.h"
>>>  #include "acpi-build.h"
>>> @@ -990,6 +993,17 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int 
>>> level)
>>>      }
>>>  }
>>>
>>> +static inline size_t pc_cpu_core_size(void)
>>> +{
>>> +    return sizeof(X86CPUCore);
>>> +}
>>> +
>>> +static inline X86CPUCore *pc_cpu_socket_get_core(X86CPUSocket *socket,
>>> +                                                 unsigned int index)
>>> +{
>>> +    return &socket->core[index];
>>> +}
>>> +
>>>  static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
>>>                            DeviceState *icc_bridge, Error **errp)
>>>  {
>>> @@ -1009,7 +1023,6 @@ static X86CPU *pc_new_cpu(const char *cpu_model, 
>>> int64_t apic_id,
>>>      qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, 
>>> "icc"));
>>>
>>>      object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
>>> -    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>>>
>>>  out:
>>>      if (local_err) {
>>> @@ -1060,15 +1073,19 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
>>>          error_propagate(errp, local_err);
>>>          return;
>>>      }
>>> +    object_property_set_bool(OBJECT(cpu), true, "realized", errp);
>>>      object_unref(OBJECT(cpu));
>>>  }
>>>
>>>  void pc_cpus_init(const char *cpu_model, DeviceState *icc_bridge)
>>>  {
>>> -    int i;
>>> +    int i, j, k;
>>> +    X86CPUSocket *socket;
>>> +    X86CPUCore *core;
>>>      X86CPU *cpu = NULL;
>>>      Error *error = NULL;
>>>      unsigned long apic_id_limit;
>>> +    int sockets, cpu_index = 0;
>>>
>>>      /* init CPUs */
>>>      if (cpu_model == NULL) {
>>> @@ -1087,14 +1104,41 @@ void pc_cpus_init(const char *cpu_model, 
>>> DeviceState *icc_bridge)
>>>          exit(1);
>>>      }
>>>
>>> -    for (i = 0; i < smp_cpus; i++) {
>>> -        cpu = pc_new_cpu(cpu_model, x86_cpu_apic_id_from_index(i),
>>> -                         icc_bridge, &error);
>>> +    sockets = smp_cpus / smp_cores / smp_threads;
>>> +    for (i = 0; i < sockets; i++) {
>>> +        socket = g_malloc0(sizeof(*socket) + smp_cores * 
>>> pc_cpu_core_size());
>>> +        object_initialize(socket, sizeof(*socket), TYPE_X86_CPU_SOCKET);
>>> +        OBJECT(socket)->free = g_free;
>>> +
>>> +        for (j = 0; j < smp_cores; j++) {
>>> +            core = pc_cpu_socket_get_core(socket, j);
>>> +            object_initialize(core, sizeof(*core), TYPE_X86_CPU_CORE);
>>> +            object_property_add_child(OBJECT(socket), "core[*]",
>>> +                                      OBJECT(core), &error);
>>> +            if (error) {
>>> +                goto error;
>>> +            }
>>> +
>>> +            for (k = 0; k < smp_threads; k++) {
>>> +                cpu = pc_new_cpu(cpu_model,
>>> +                                 x86_cpu_apic_id_from_index(cpu_index),
>>> +                                 icc_bridge, &error);
>>> +                if (error) {
>>> +                    goto error;
>>> +                }
>>> +                object_property_add_child(OBJECT(core), "thread[*]",
>>> +                                          OBJECT(cpu), &error);
>>> +                object_unref(OBJECT(cpu));
>>> +                if (error) {
>>> +                    goto error;
>>> +                }
>>> +                cpu_index++;
>>> +            }
>>> +        }
>>> +        object_property_set_bool(OBJECT(socket), true, "realized", &error);
>>
>> So you do in-place initialization as part of an "atomic" socket object.
>
> (indivisible might've been a better term on my part)
>
>>
>> I am not sure why cores and threads should be allocated as part of
>> socket object and initialized like above ? Do you see any problem with
>> just instantiating the socket object and then let the instance_init
>> routines of socket and cores to initialize the cores and threads like
>> how I am doing for sPAPR ?
>>
>> +    sockets = smp_cpus / smp_cores / smp_threads;
>> +    for (i = 0; i < sockets; i++) {
>> +        socket = object_new(TYPE_POWERPC_CPU_SOCKET);
>> +        object_property_set_bool(socket, true, "realized", &error_abort);
>>      }
>>
>> Ref: http://lists.gnu.org/archive/html/qemu-ppc/2015-03/msg00492.html
>
> Yes, instance_init cannot fail. By making the allocation separate, we
> assure that at this stage we have sufficient memory to perform the
> initializations. This is easiest when we know how many child objects we
> have, then we can use proper fields or arrays to reserve the memory
> (e.g., ARM MPCore, PCI host bridges). Here, to cope with dynamic
> smp_cores, I am using inline helpers to do the size/offset calculations.

Ok, So the difference between what you are doing and the approach I
have taken is not that much.

You allocate a big socket object (which includes memory for cores and
threads objects) and then initialize them in a loop and finally
realize the socket object which iteratively realizes cores and
threads.

Instead of open g_malloc0, I resort to object_new for the socket
object. The instance_init of the socket object will create core
objects (via object_new again) and the instance_init of thread object
will create the CPU threads (using object_new). At the end of this I
do a realize on socket object which iteratively realizes cores and
threads similar to your implementation.

So unless I am missing the subtlety here, it is about one big malloc
vs multiple small mallocs. So the chance for failure exists in both
cases.

>
> Further, setting realized=true from instance_init is a no-go. It's a
> two-step initialization, with realization being the second step and
> potentially failing. The alternative I pointed you to was creating
> objects in a property setter, like Alexey was asked for for xics.

Please look at my v2 patchset, I am not setting realized=true from
instance _init.

Regards,
Bharata.