From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57208) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bNwcq-0007QP-DM for qemu-devel@nongnu.org; Fri, 15 Jul 2016 02:29:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bNwco-0000vc-71 for qemu-devel@nongnu.org; Fri, 15 Jul 2016 02:29:27 -0400 Date: Fri, 15 Jul 2016 08:29:14 +0200 From: Andrew Jones Message-ID: <20160715062914.tvg4wcta5eflgk5j@hawk.localdomain> References: <1465580427-13596-1-git-send-email-drjones@redhat.com> <1465580427-13596-6-git-send-email-drjones@redhat.com> <20160614020026.GH4882@voom.fritz.box> <20160614060807.tkdq2dy2kw6d3bw4@hawk.localdomain> <20160615003759.GR4882@voom.fritz.box> <20160615071113.jvvuqpgtkg6okqdg@hawk.localdomain> <20160714201820.GE31865@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160714201820.GE31865@thinpad.lan.raisama.net> Subject: Re: [Qemu-devel] [PATCH RFC 05/16] hw/core/machine: add smp properites List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: peter.maydell@linaro.org, agraf@suse.de, qemu-devel@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org, imammedo@redhat.com, pbonzini@redhat.com, dgibson@redhat.com, David Gibson 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 > > > > > > --- > > > > > > 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 >