Re: [Qemu-devel] [PATCH RFC 07/16] qom/cpu: make nr-cores, nr-threads real properties

From: David Gibson <david@gibson.dropbear.id.au>
To: Andrew Jones <drjones@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>,
	qemu-devel@nongnu.org, qemu-ppc@nongnu.org, qemu-arm@nongnu.org,
	peter.maydell@linaro.org, ehabkost@redhat.com,
	Bharata B Rao <bharata@linux.vnet.ibm.com>,
	agraf@suse.de, pbonzini@redhat.com, imammedo@redhat.com,
	dgibson@redhat.com
Subject: Re: [Qemu-devel] [PATCH RFC 07/16] qom/cpu: make nr-cores, nr-threads real properties
Date: Tue, 14 Jun 2016 12:12:16 +1000	[thread overview]
Message-ID: <20160614021216.GI4882@voom.fritz.box> (raw)
In-Reply-To: <20160612134810.posa6p4wyxhit4yt@hawk.localdomain>

[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]

On Sun, Jun 12, 2016 at 03:48:10PM +0200, Andrew Jones wrote:
> On Sat, Jun 11, 2016 at 08:54:35AM +0200, Thomas Huth wrote:
> > On 10.06.2016 19:40, Andrew Jones wrote:
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  qom/cpu.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/qom/cpu.c b/qom/cpu.c
> > > index 751e992de8823..024cda3eb98c8 100644
> > > --- a/qom/cpu.c
> > > +++ b/qom/cpu.c
> > > @@ -28,6 +28,7 @@
> > >  #include "exec/log.h"
> > >  #include "qemu/error-report.h"
> > >  #include "sysemu/sysemu.h"
> > > +#include "hw/qdev-properties.h"
> > >  
> > >  bool cpu_exists(int64_t id)
> > >  {
> > > @@ -342,6 +343,12 @@ static int64_t cpu_common_get_arch_id(CPUState *cpu)
> > >      return cpu->cpu_index;
> > >  }
> > >  
> > > +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.

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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]