Re: [Qemu-arm] QOM: best way for parents to pass information to children? (was Re: [Qemu-devel] [PATCH RFC 07/16] qom/cpu: make nr-cores, nr-threads real properties)

[Andreas Färber]

Am 15.07.2016 um 18:10 schrieb Eduardo Habkost:
> On Fri, Jul 15, 2016 at 11:11:38AM +0200, Igor Mammedov wrote:
>> On Fri, 15 Jul 2016 08:35:30 +0200
>> Andrew Jones <address@hidden> wrote:
>>> On Thu, Jul 14, 2016 at 05:07:43PM -0300, Eduardo Habkost wrote:
>>>>
>>>> First of all, sorry for the horrible delay in replying to this
>>>> thread.
>>>>
>>>> On Wed, Jun 15, 2016 at 10:56:20AM +1000, David Gibson wrote:  
>>>>> On Tue, Jun 14, 2016 at 08:19:49AM +0200, Andrew Jones wrote:  
>>>>>> On Tue, Jun 14, 2016 at 12:12:16PM +1000, David Gibson wrote:  
>>>>>>> On Sun, Jun 12, 2016 at 03:48:10PM +0200, Andrew Jones wrote:  
> [...]
>>>>>>>>>> +static Property cpu_common_properties[] = {
>>>>>>>>>> +    DEFINE_PROP_INT32("nr-cores", CPUState, nr_cores, 1),
>>>>>>>>>> +    DEFINE_PROP_INT32("nr-threads", CPUState, nr_threads, 1),
>>>>>>>>>> +    DEFINE_PROP_END_OF_LIST()
>>>>>>>>>> +};  
>>>>>>>>>
>>>>>>>>> Are you aware of the current CPU hotplug discussion that is going on? 
>>>>>>>>>  
>>>>>>>>
>>>>>>>> I'm aware of it going on, but haven't been following it.
>>>>>>>>   
>>>>>>>>> I'm not very involved there, but I think some of these reworks also 
>>>>>>>>> move
>>>>>>>>> "nr_threads" into the CPU state already, e.g. see:  
>>>>>>>>
>>>>>>>> nr_threads (and nr_cores) are already state in CPUState. This patch 
>>>>>>>> just
>>>>>>>> exposes that state via properties.
>>>>>>>>   
>>>>>>>>>
>>>>>>>>> https://github.com/dgibson/qemu/commit/9d07719784ecbeebea71
>>>>>>>>>
>>>>>>>>> ... so you might want to check these patches first to see whether you
>>>>>>>>> can base your rework on them?  
>>>>>>>>
>>>>>>>> Every cpu, and thus every machine, uses CPUState for its cpus. I'm
>>>>>>>> not sure every machine will want to use that new abstract core class
>>>>>>>> though. If they did, then we could indeed use nr_threads from there
>>>>>>>> instead (and remove it from CPUState), but we'd still need nr_cores
>>>>>>>> from the abstract cpu package class (CPUState).  
>>>>>>>
>>>>>>> Hmm.  Since the CPUState object represents just a single thread, it
>>>>>>> seems weird to me that it would have nr_threads and nr_cores
>>>>>>> information.  
>>>>
>>>> Agreed it is weird, and I think we should try to move it away
>>>> from CPUState, not make it part of the TYPE_CPU interface.
>>>> nr_threads belongs to the actual container of the Thread objects,
>>>> and nr_cores in the actual container of the Core objects.
>>>>
>>>> The problem is how to implement that in a non-intrusive way that
>>>> would require changing the object hierarchy of all architectures.
>>>>
>>>>   
>>>>>>>
>>>>>>> Exposing those as properties makes that much worse, because it's now
>>>>>>> ABI, rather than internal detail we can clean up at some future time.  
>>>>>>
>>>>>> CPUState is supposed to be "State of one CPU core or thread", which
>>>>>> justifies having nr_threads state, as it may be describing a core.  
>>>>>
>>>>> Um.. does it ever actually represent a (multithread) core in practice?
>>>>> It would need to have duplicated register state for every thread were
>>>>> that the case.  
>>>>
>>>> AFAIK, CPUState is still always thread state. Or has this changed
>>>> in some architectures, already?
>>>>   
>>>>>   
>>>>>> I guess there's no justification for having nr_cores in there though.
>>>>>> I agree adding the Core class is a good idea, assuming it will get used
>>>>>> by all machines, and CPUState then gets changed to a Thread class. The
>>>>>> question then, though, is do we also create a Socket class that contains
>>>>>> nr_cores?  
>>>>>
>>>>> That was roughly our intention with the way the cross platform hotplug
>>>>> stuff is evolving.  But the intention was that the Socket objects
>>>>> would only need to be constructed for machine types where it makes
>>>>> sense.  So for example on the paravirt pseries platform, we'll only
>>>>> have Core objects, because the socket distinction isn't really
>>>>> meaningful.
>>>>>   
>>>>>> And how will a Thread method get that information when it
>>>>>> needs to emulate, e.g. CPUID, that requires it? It's a bit messy, so
>>>>>> I'm open to all suggestions on it.  
>>>>>
>>>>> So, if the Thread needs this information, I'm not opposed to it having
>>>>> it internally (presumably populated earlier from the Core object).
>>>>> But I am opposed to it being a locked in part of the interface by
>>>>> having it as an exposed property.  
>>>>
>>>> I agree we don't want to make this part of the external
>>>> interface. In this case, if we don't add the properties, how
>>>> exactly is the Machine or Core code supposed to pass that
>>>> information to the Thread object?
>>>>
>>>> Maybe the intermediate steps could be:
>>>>
>>>> * Make the Thread code that uses CPUState::nr_{cores,threads} and
>>>>   smp_{cores,threads} get that info from MachineState instead.  
>>>
>>> I have some patches already headed down this road.
>>>
>>>> * On the architectures where we already have a reasonable
>>>>   Socket/Core/Thread hierarchy, let the Thread code simply ask
>>>>   for that information from its parent.  
>>>
>>> I guess that's just spapr so far, or at least spapr is the closest.
>>> Indeed this appears to be the cleanest approach, so architectures
>>> adding support for cpu topology should likely strive to implement it
>>> this way.
>> If I recall correctly, the only thing about accessing parent is that
>> in QOM design accessing parent from child wasn't accepted well,
>> i.e. child shouldn't be aware nor access parent object.
> 
> Can anybody explain why?
> 
> In this case, what's the best way for a parent to pass
> information to its children without adding new externally-visible
> properties that the user is never supposed to set directly?
> 
> Should Thread objects have an additional link to the parent Core
> object, just to be able to get the information it needs?

I am not fully aware either and believe I ignored it in my x86 socket
patchset, part of which it was RFC.

The key thing to consider is that this breaks user instantiation of a
device, so it needs to be disabled.

Note that I'm just replying to this particular question without the full
context.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)