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

[Igor Mammedov]

On Fri, 15 Jul 2016 18:33:53 -0300
Eduardo Habkost <address@hidden> wrote:

> On Fri, Jul 15, 2016 at 08:38:35PM +0200, Igor Mammedov wrote:
> > On Fri, 15 Jul 2016 14:43:53 -0300
> > Eduardo Habkost <address@hidden> wrote:  
> > > On Fri, Jul 15, 2016 at 06:30:41PM +0200, Andreas Färber wrote:  
> > > > 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.  
> > > 
> > > Good point, and this is hard to solve without changing the way
> > > device_add works. Setting extra properties, on the other hand,
> > > can be done easily by the hotplug handler if necessary (like we
> > > do with apic-id in PC).
> > > 
> > > Also, if the properties are not supposed to be set directly by
> > > the user, then the hotplug handler could refuse to hotplug the
> > > device if the user tried to fiddle with them. Then the "external
> > > interface" problem is solved.
> > > 
> > > Now, depending on how much information is needed, "extra
> > > properties" may be duplicating data that is already available in
> > > other objects (like nr-cores/nr-threads), or just a link property
> > > (e.g. a link to the Core object in the case of spapr). If we
> > > still don't have the right object topology implemented, then we
> > > may need to use individual properties like "nr-cores" and
> > > "nr-threads" (preferably as a temporary solution?).
> > > 
> > > In other words, maybe "nr-cores" and "nr-threads" properties will
> > > be useful in x86, but only if we reject device creation in case
> > > the user tries to set them manually, and if we do _not_ expose
> > > them on TYPE_CPU.  
> > Should be add a QOM API that could mark property as an internal
> > that would be beneficial in generic as we won't have to be scared
> > exposing internal stuff to users and be able to hide target specifics
> > behind properties?
> > 
> > it should be simple enough to do.  
> 
> If it's internal, do we have any reason to register a (writeable)
> property in the first place? Why not use a plain old
> "obj->field = value" C statement? Or, if a simple assignment
> isn't enough, why not a simple obj_set_field(value) C function?
So that arch neutral code won't have to pull obj type definition
and we would be able to reuse all machinery that uses properties
instead of inventing yet another API or ad-hoc function calls.