RE: [PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology

[Anup Patel]

> -----Original Message-----
> From: Daniel P. Berrangé <address@hidden>
> Sent: 27 May 2020 14:16
> To: Anup Patel <address@hidden>
> Cc: Eduardo Habkost <address@hidden>; Marcel Apfelbaum
> <address@hidden>; Peter Maydell <address@hidden>;
> Palmer Dabbelt <address@hidden>; Alistair Francis
> <address@hidden>; Sagar Karandikar <address@hidden>;
> Atish Patra <address@hidden>; address@hidden; qemu-
> address@hidden; Anup Patel <address@hidden>
> 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 <address@hidden>
> > ---
> >  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 :|