Re: [PATCH v12 1/7] s390x/cpu topology: Creating CPU topology device

[Pierre Morel <pmorel@linux.ibm.com>]

On 12/7/22 12:38, Janis Schoetterl-Glausch wrote:
>   * On Wed, 2022-12-07 at 11:00 +0100, Pierre Morel wrote:
>>
>> On 12/6/22 22:06, Janis Schoetterl-Glausch wrote:
>>> On Tue, 2022-12-06 at 15:35 +0100, Pierre Morel wrote:
>>>>
>>>> On 12/6/22 14:35, Janis Schoetterl-Glausch wrote:
>>>>> On Tue, 2022-12-06 at 11:32 +0100, Pierre Morel wrote:
>>>>>>
>>>>>> On 12/6/22 10:31, Janis Schoetterl-Glausch wrote:
>>>>>>> On Tue, 2022-11-29 at 18:42 +0100, Pierre Morel wrote:
>>>>>>>> We will need a Topology device to transfer the topology
>>>>>>>> during migration and to implement machine reset.
>>>>>>>>
>>>>>>>> The device creation is fenced by s390_has_topology().
>>>>>>>>
>>>>>>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>>>>>>> ---
>>>>>>>>   include/hw/s390x/cpu-topology.h | 44 +++++++++++++++
>>>>>>>>   include/hw/s390x/s390-virtio-ccw.h | 1 +
>>>>>>>>   hw/s390x/cpu-topology.c | 87 ++++++++++++++++++++++++++++++
>>>>>>>>   hw/s390x/s390-virtio-ccw.c | 25 +++++++++
>>>>>>>>   hw/s390x/meson.build | 1 +
>>>>>>>>   5 files changed, 158 insertions(+)
>>>>>>>>   create mode 100644 include/hw/s390x/cpu-topology.h
>>>>>>>>   create mode 100644 hw/s390x/cpu-topology.c
>>>>>>>
> [...]
> 
>>>>>>>> + object_property_set_int(OBJECT(dev), "num-cores",
>>>>>>>> + machine->smp.cores * machine->smp.threads, errp);
>>>>>>>> + object_property_set_int(OBJECT(dev), "num-sockets",
>>>>>>>> + machine->smp.sockets, errp);
>>>>>>>> +
>>>>>>>> + sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), errp);
>>>>>>>
>>>>>>> I must admit that I haven't fully grokked qemu's memory management yet.
>>>>>>> Is the topology devices now owned by the sysbus?
>>>>>>
>>>>>> Yes it is so we see it on the qtree with its properties.
>>>>>>
>>>>>>
>>>>>>> If so, is it fine to have a pointer to it S390CcwMachineState?
>>>>>>
>>>>>> Why not?
>>>>>
>>>>> If it's owned by the sysbus and the object is not explicitly referenced
>>>>> for the pointer, it might be deallocated and then you'd have a dangling pointer.
>>>>
>>>> Why would it be deallocated ?
>>>
>>> That's beside the point, if you transfer ownership, you have no control over when
>>> the deallocation happens.
>>> It's going to be fine in practice, but I don't think you should rely on it.
>>> I think you could just do sysbus_realize instead of ..._and_unref,
>>> but like I said, I haven't fully understood qemu memory management.
>>> (It would also leak in a sense, but since the machine exists forever that should be fine)
>>
>> If I understand correctly:
>>
>> - qdev_new adds a reference count to the new created object, dev.
>>
>> - object_property_add_child adds a reference count to the child also
>> here the new created device dev so the ref count of dev is 2 .
>>
>> after the unref on dev, the ref count of dev get down to 1
>>
>> then it seems OK. Did I miss something?
> 
> The properties ref belongs to the property, if the property were removed,
> it would be unref'ed. There is no extra ref for the pointer in S390CcwMachineState.
> I'm coming from a clean code perspective, I don't think we'd run into this problem in practice.

OK, I understand, you are right.
My original code used object_resolve_path() to retrieve the object what 
made things cleaner I think.

For performance reason, Cedric proposed during the review of V10 to add 
the pointer to the machine state instead.

I must say that I am not very comfortable to argument on this.
@Cedric what do you think?


Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen