On Fri, 15 Jul 2016 08:35:30 +0200
Andrew Jones <drjo...@redhat.com> 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 <drjo...@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.
> >
> > 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
> >
