Re: [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)

[Eduardo Habkost <ehabkost@redhat.com>]

On Mon, Jul 18, 2016 at 09:23:04AM +0200, Igor Mammedov wrote:
> On Fri, 15 Jul 2016 18:33:53 -0300
> Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com> 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 <drjones@redhat.com> 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

I don't get it. If arch neutral code uses it, it should be
available in an arch-neutral header.

> and we would be able to reuse all machinery that uses properties
> instead of inventing yet another API or ad-hoc function calls.

Why is adding a new C function or setting a struct field worse
than adding a new property name? I actually prefer the former,
because it makes code review easier and allows the compiler to
detect more mistakes.

-- 
Eduardo