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)

[Eduardo Habkost]

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.

Anyway, I think all that can be done as a second step. First, we
need to replace existing usage of smp_{threads,cores} globals
with MachineState fields. Later, remaining qdev_get_machine()
calls can be replaced by a more QOM-friendly mechanism.

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

No problem. I wanted it to be a generic question about what are
QOM best practices. Thanks for your feedback.

-- 
Eduardo