> -----Original Message-----
> From: Daniel P. Berrangé <berra...@redhat.com>
> Sent: 27 May 2020 14:16
> To: Anup Patel <anup.pa...@wdc.com>
> Cc: Eduardo Habkost <ehabk...@redhat.com>; Marcel Apfelbaum
> <marcel.apfelb...@gmail.com>; Peter Maydell <peter.mayd...@linaro.org>;
> Palmer Dabbelt <pal...@dabbelt.com>; Alistair Francis
> <alistair.fran...@wdc.com>; Sagar Karandikar <sag...@eecs.berkeley.edu>;
> Atish Patra <atish.pa...@wdc.com>; qemu-ri...@nongnu.org; qemu-
> de...@nongnu.org; Anup Patel <a...@brainfault.org>
> Subject: Re: [PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology
>
> On Wed, May 27, 2020 at 11:12:22AM +0530, Anup Patel wrote:
> > When "sockets" sub-option of "-smp" option is not specified, the
> > smp_parse() function will assume one CPU per-socket and set the number
> > of sockets equal to number of CPUs.
> >
> > This is counter-intuitive and we should allow machine emulation to
> > decide default number of sockets when "sockets" sub-option is not
> > specified.
>
> I don't agree with this. Having the semantics of the -smp option be the same
> across all targets/machines *is* intuitive. Changing semantics of -smp per-
> machine will create a worse experiance for people configuring QEMU as the
> configuration will mean different things depending on the machine choce.
Okay then why don't we default to sockets=1 in smp_parse() when "sockets"
sub-options is not specified ?? This will make it uniform across machines.
Is there a reason to by default have sockets=max_cpus ??
Regards,
Anup
>
>
> > To achieve this, we add boolean flag sockets_specified in
> > struct CpuTopology which tells machine emulation whether the "sockets"
> > sub-option was specified in command-line.
> >
> > Signed-off-by: Anup Patel <anup.pa...@wdc.com>
> > ---
> > hw/core/machine.c | 2 ++
> > include/hw/boards.h | 2 ++
> > 2 files changed, 4 insertions(+)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c index
> > bb3a7b18b1..fd5ef5a4bb 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -706,6 +706,8 @@ static void smp_parse(MachineState *ms, QemuOpts
> *opts)
> > unsigned cores = qemu_opt_get_number(opts, "cores", 0);
> > unsigned threads = qemu_opt_get_number(opts, "threads", 0);
> >
> > + ms->smp.sockets_specified = (sockets == 0) ? false : true;
> > +
> > /* compute missing values, prefer sockets over cores over threads
> > */
> > if (cpus == 0 || sockets == 0) {
> > cores = cores > 0 ? cores : 1; diff --git
> > a/include/hw/boards.h b/include/hw/boards.h index
> > 18815d9be2..59b28ada65 100644
> > --- a/include/hw/boards.h
> > +++ b/include/hw/boards.h
> > @@ -244,6 +244,7 @@ typedef struct DeviceMemoryState {
> > * @cores: the number of cores in one package
> > * @threads: the number of threads in one core
> > * @sockets: the number of sockets on the machine
> > + * @sockets_specified: the number of sockets were specified for the
> > + machine
> > * @max_cpus: the maximum number of logical processors on the machine
> > */
> > typedef struct CpuTopology {
> > @@ -251,6 +252,7 @@ typedef struct CpuTopology {
> > unsigned int cores;
> > unsigned int threads;
> > unsigned int sockets;
> > + bool sockets_specified;
> > unsigned int max_cpus;
> > } CpuTopology;
> >
> > --
> > 2.25.1
> >
> >
>
> Regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
