Re: [Qemu-arm] [Qemu-devel] [PATCH RFC 05/16] hw/core/machine: add smp properites

[Andrew Jones]

On Thu, Jul 14, 2016 at 05:18:20PM -0300, Eduardo Habkost wrote:
> On Wed, Jun 15, 2016 at 09:11:13AM +0200, Andrew Jones wrote:
> > On Wed, Jun 15, 2016 at 10:37:59AM +1000, David Gibson wrote:
> > > On Tue, Jun 14, 2016 at 08:08:07AM +0200, Andrew Jones wrote:
> > > > On Tue, Jun 14, 2016 at 12:00:26PM +1000, David Gibson wrote:
> > > > > On Fri, Jun 10, 2016 at 07:40:16PM +0200, Andrew Jones wrote:
> > > > > > Signed-off-by: Andrew Jones <address@hidden>
> > > > > > ---
> > > > > >  hw/core/machine.c   | 81 
> > > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  include/hw/boards.h |  6 ++++
> > > > > >  2 files changed, 87 insertions(+)
> > > > > > 
> > > > > > diff --git a/hw/core/machine.c b/hw/core/machine.c
> > > > > > index 3dce9020e510a..2625044002e57 100644
> > > > > > --- a/hw/core/machine.c
> > > > > > +++ b/hw/core/machine.c
> > > > > > @@ -172,6 +172,53 @@ static void machine_set_dumpdtb(Object *obj, 
> > > > > > const char *value, Error **errp)
> > > > > >      ms->dumpdtb = g_strdup(value);
> > > > > >  }
> > > > > >  
> > > > > > +static void machine_get_smp(Object *obj, Visitor *v, const char 
> > > > > > *name,
> > > > > > +                            void *opaque, Error **errp)
> > > > > > +{
> > > > > > +    MachineState *ms = MACHINE(obj);
> > > > > > +    int64_t value;
> > > > > > +
> > > > > > +    if (strncmp(name, "sockets", 7) == 0) {
> > > > > > +        value = ms->sockets;
> > > > > > +    } else if (strncmp(name, "cores", 5) == 0) {
> > > > > > +        value = ms->cores;
> > > > > > +    } else if (strncmp(name, "threads", 7) == 0) {
> > > > > > +        value = ms->threads;
> > > > > > +    } else if (strncmp(name, "maxcpus", 7) == 0) {
> > > > > > +        value = ms->maxcpus;
> > > > > > +    } else if (strncmp(name, "cpus", 4) == 0) {
> > > > > > +        value = ms->cpus;
> > > > > > +    }
> > > > > > +
> > > > > > +    visit_type_int(v, name, &value, errp);
> > > > > > +}
> > > > > 
> > > > > Any particular for multiplexing all the set / get, rather than having
> > > > > separate callbacks for each property?
> > > > 
> > > > Not really. This way just makes denser code. But I'll go whichever
> > > > direction people prefer.
> > > 
> > > I'd prefer not to have the multiplexer, but I don't care that much.
> > > 
> > > > Actually I should probably add an
> > > > else { error_report(...) } in either case, which means the multifunction
> > > > direction would still contain strncmps.
> > > 
> > > Hrm.. that would seem an odd choice to me if you didn't have the
> > > multiplex.  Not including the strncmp() means you can change the
> > > property name (or add aliases) in a single place, without changing the
> > > callback functions.
> > > 
> > > Note also that the current set of strncmp()s is only correct because
> > > there is a limited set of things bound to this callback.  Remember
> > > that strncmp(name, "sockets", 7) will match "socketsfoo".
> > 
> > Good point. I'll change to multiple functions and just drop the
> > strncmps.
> 
> Multiple functions that just get/set struct fields would be
> easier to convert to use a better property API (that don't
> require writing getter/setters) in the future.
> 
> [...]
> > > > > > +
> > > > > > +    int sockets;
> > > > > > +    int cores;
> > > > > > +    int threads;
> > > > > > +    int maxcpus;
> > > > > > +    int cpus;
> > > > > 
> > > > > Hrm.. as the tests added in earlier patches highlight, essentially one
> > > > > of these properties is redundant.  Is there a reason to include them
> > > > > all, rather than to pick one to drop?
> > > > 
> > > > Well, IMO, only maxcpus could be dropped. I'd prefer not to though
> > > > because it's a convenient state to have pre-calculated, and possibly
> > > > error prone to leave to all users to re-calculate.
> > > 
> > > Sorry, to clarify.  I have no problem with having all 5 variables,
> > > with one precalculated from the others.  What I'm not convinced is a
> > > good idea is exposing all 5 as settable properties.
> > 
> > I see. I think we need them all, especially as we decide which ones
> > can be optional, if given others. For example we need cpus and maxcpus
> > for obvious reasons. threads can be optional (default =1), but if you
> > want to specify more than 1 then we need the property. Currently I
> > think both sockets and cores should be required since we don't know
> > which should be calculated form the other, and therefore can't set
> > a default for either. That's all five.
> 
> If we were designing a new interface, I would prefer to make it
> different, and add properties only to machine classes that really
> make this configurable. But as we are just moving existing data
> and logic from vl.c into MachineState fields, I agree we need all
> the struct fields, so the existing vl.c code can be easily moved
> to machine.c.
> 
> But:
> 
> About the new externally-visible interface: what about we
> register the properties only on the machine subclasses that
> really do something with those options? Then we won't need to
> worry about keeping compatibility in machines that never
> supported multi-core/multi-thread configurations in the first
> place.

I like that. I'll look into it for v1.

Thanks,
drew

> 
> -- 
> Eduardo
>