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

[Igor Mammedov]

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


> 
> Thanks,
> drew
> 
> > 
> > -- 
> > Eduardo
> >