qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eduardo Habkost
Subject: Re: [Qemu-devel] [PATCH RFC 07/16] qom/cpu: make nr-cores, nr-threads real properties
Date: Thu, 14 Jul 2016 17:07:43 -0300
User-agent: Mutt/1.6.1 (2016-04-27)

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:
> > > > 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 <address@hidden>
> > > > > > ---
> > > > > >  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.

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.
* 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.

-- 
Eduardo



reply via email to

[Prev in Thread] Current Thread [Next in Thread]