From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54044) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bPTbS-0003sx-Bt for qemu-devel@nongnu.org; Tue, 19 Jul 2016 07:54:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bPTbP-0005ES-FD for qemu-devel@nongnu.org; Tue, 19 Jul 2016 07:54:21 -0400 Date: Tue, 19 Jul 2016 08:54:06 -0300 From: Eduardo Habkost Message-ID: <20160719115406.GA3337@thinpad.lan.raisama.net> References: <20160615005620.GU4882@voom.fritz.box> <20160714200743.GC31865@thinpad.lan.raisama.net> <20160715063530.2ebl4v4vpdtrpx5a@hawk.localdomain> <20160715111138.114d8b5f@nial.brq.redhat.com> <20160715161009.GB3727@thinpad.lan.raisama.net> <20160715174353.GD3727@thinpad.lan.raisama.net> <20160715203835.38565299@igors-macbook-pro.local> <20160715213353.GA3618@thinpad.lan.raisama.net> <20160716153004.3a3arbrz2thfk3zj@hawk.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20160716153004.3a3arbrz2thfk3zj@hawk.localdomain> Content-Transfer-Encoding: quoted-printable Subject: 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) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andrew Jones Cc: Igor Mammedov , peter.maydell@linaro.org, qemu-devel@nongnu.org, agraf@suse.de, qemu-arm@nongnu.org, qemu-ppc@nongnu.org, Bharata B Rao , Thomas Huth , pbonzini@redhat.com, dgibson@redhat.com, Andreas =?iso-8859-1?Q?F=E4rber?= , David Gibson On Sat, Jul 16, 2016 at 05:30:04PM +0200, Andrew Jones wrote: > On Fri, Jul 15, 2016 at 06:33:53PM -0300, Eduardo Habkost 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 wrote: > > > > On Fri, Jul 15, 2016 at 06:30:41PM +0200, Andreas F=E4rber 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 wrote: > > > > > >>> On Thu, Jul 14, 2016 at 05:07:43PM -0300, Eduardo Habkost w= rote: > > > > > >>>> > > > > > >>>> 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 wro= te: =20 > > > > > >>>>> On Tue, Jun 14, 2016 at 08:19:49AM +0200, Andrew Jones > > > > > >>>>> wrote: =20 > > > > > >>>>>> On Tue, Jun 14, 2016 at 12:12:16PM +1000, David Gibson > > > > > >>>>>> wrote: =20 > > > > > >>>>>>> On Sun, Jun 12, 2016 at 03:48:10PM +0200, Andrew Jones > > > > > >>>>>>> wrote: =20 > > > > > > [...] > > > > > >>>>>>>>>> +static Property cpu_common_properties[] =3D { > > > > > >>>>>>>>>> + DEFINE_PROP_INT32("nr-cores", CPUState, nr_core= s, > > > > > >>>>>>>>>> 1), > > > > > >>>>>>>>>> + DEFINE_PROP_INT32("nr-threads", CPUState, > > > > > >>>>>>>>>> nr_threads, 1), > > > > > >>>>>>>>>> + DEFINE_PROP_END_OF_LIST() > > > > > >>>>>>>>>> +}; =20 > > > > > >>>>>>>>> > > > > > >>>>>>>>> Are you aware of the current CPU hotplug discussion t= hat > > > > > >>>>>>>>> is going on? =20 > > > > > >>>>>>>> > > > > > >>>>>>>> I'm aware of it going on, but haven't been following i= t. > > > > > >>>>>>>> =20 > > > > > >>>>>>>>> I'm not very involved there, but I think some of thes= e > > > > > >>>>>>>>> reworks also move "nr_threads" into the CPU state > > > > > >>>>>>>>> already, e.g. see: =20 > > > > > >>>>>>>> > > > > > >>>>>>>> nr_threads (and nr_cores) are already state in CPUStat= e. > > > > > >>>>>>>> This patch just exposes that state via properties. > > > > > >>>>>>>> =20 > > > > > >>>>>>>>> > > > > > >>>>>>>>> https://github.com/dgibson/qemu/commit/9d07719784ecbe= ebea71 > > > > > >>>>>>>>> > > > > > >>>>>>>>> ... so you might want to check these patches first to= see > > > > > >>>>>>>>> whether you can base your rework on them? =20 > > > > > >>>>>>>> > > > > > >>>>>>>> Every cpu, and thus every machine, uses CPUState for i= ts > > > > > >>>>>>>> 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 i= t > > > > > >>>>>>>> from CPUState), but we'd still need nr_cores from the > > > > > >>>>>>>> abstract cpu package class (CPUState). =20 > > > > > >>>>>>> > > > > > >>>>>>> Hmm. Since the CPUState object represents just a singl= e > > > > > >>>>>>> thread, it seems weird to me that it would have nr_thre= ads > > > > > >>>>>>> and nr_cores information. =20 > > > > > >>>> > > > > > >>>> Agreed it is weird, and I think we should try to move it a= way > > > > > >>>> 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 wa= y > > > > > >>>> that would require changing the object hierarchy of all > > > > > >>>> architectures. > > > > > >>>> > > > > > >>>> =20 > > > > > >>>>>>> > > > > > >>>>>>> Exposing those as properties makes that much worse, bec= ause > > > > > >>>>>>> it's now ABI, rather than internal detail we can clean = up > > > > > >>>>>>> at some future time. =20 > > > > > >>>>>> > > > > > >>>>>> 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. =20 > > > > > >>>>> > > > > > >>>>> 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. =20 > > > > > >>>> > > > > > >>>> AFAIK, CPUState is still always thread state. Or has this > > > > > >>>> changed in some architectures, already? > > > > > >>>> =20 > > > > > >>>>> =20 > > > > > >>>>>> I guess there's no justification for having nr_cores in > > > > > >>>>>> there though. I agree adding the Core class is a good id= ea, > > > > > >>>>>> assuming it will get used by all machines, and CPUState = then > > > > > >>>>>> gets changed to a Thread class. The question then, thoug= h, > > > > > >>>>>> is do we also create a Socket class that contains nr_cor= es? =20 > > > > > >>>>> > > > > > >>>>> That was roughly our intention with the way the cross > > > > > >>>>> platform hotplug stuff is evolving. But the intention wa= s > > > > > >>>>> that the Socket objects would only need to be constructed= for > > > > > >>>>> machine types where it makes sense. So for example on th= e > > > > > >>>>> paravirt pseries platform, we'll only have Core objects, > > > > > >>>>> because the socket distinction isn't really meaningful. > > > > > >>>>> =20 > > > > > >>>>>> And how will a Thread method get that information when i= t > > > > > >>>>>> needs to emulate, e.g. CPUID, that requires it? It's a b= it > > > > > >>>>>> messy, so I'm open to all suggestions on it. =20 > > > > > >>>>> > > > > > >>>>> So, if the Thread needs this information, I'm not opposed= to > > > > > >>>>> it having it internally (presumably populated earlier fro= m > > > > > >>>>> the Core object). But I am opposed to it being a locked i= n > > > > > >>>>> part of the interface by having it as an exposed property= . =20 > > > > > >>>> > > > > > >>>> I agree we don't want to make this part of the external > > > > > >>>> interface. In this case, if we don't add the properties, h= ow > > > > > >>>> 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,threa= ds} > > > > > >>>> and smp_{cores,threads} get that info from MachineState > > > > > >>>> instead. =20 > > > > > >>> > > > > > >>> 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. =20 > > > > > >>> > > > > > >>> I guess that's just spapr so far, or at least spapr is the > > > > > >>> closest. Indeed this appears to be the cleanest approach, s= o > > > > > >>> 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 accept= ed > > > > > >> well, i.e. child shouldn't be aware nor access parent object= . > > > > > >=20 > > > > > > Can anybody explain why? > > > > > >=20 > > > > > > In this case, what's the best way for a parent to pass > > > > > > information to its children without adding new externally-vis= ible > > > > > > properties that the user is never supposed to set directly? > > > > > >=20 > > > > > > Should Thread objects have an additional link to the parent C= ore > > > > > > object, just to be able to get the information it needs? > > > > >=20 > > > > > I am not fully aware either and believe I ignored it in my x86 > > > > > socket patchset, part of which it was RFC. > > > > >=20 > > > > > The key thing to consider is that this breaks user instantiatio= n of > > > > > a device, so it needs to be disabled. > > > >=20 > > > > 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). > > > >=20 > > > > 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. > > > >=20 > > > > 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?). > > > >=20 > > > > 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 specifi= cs > > > behind properties? > > >=20 > > > it should be simple enough to do. > >=20 > > 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 =3D value" C statement? Or, if a simple assignment > > isn't enough, why not a simple obj_set_field(value) C function? >=20 > Being able to use qdev_prop_register_global was the motivation for > making nr-cores,nr-threads properties. If we can create something > like that for a "field", without too much code duplication, then > that'd work. If we end up duplicating much of the property code, > though, then I think extending the property code with a set-as-internal > feature, as Igor proposes, may be the better way to go. In the case of -smp, I don't think we should use global properties: we just need to set the right MachineState fields. But you have a point: compat_props (which are registered as global properties) are probably the main reason we have been adding QOM properties for internal stuff. But maybe that's a feature? If we want to make something different depending on the machine-type, it's better to make the difference visible to the user and management software. --=20 Eduardo