| Message ID | 20200527054226.232103-2-anup.patel@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISC-V multi-socket support | expand |
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. > 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.patel@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
> -----Original Message----- > From: Daniel P. Berrangé <berrange@redhat.com> > Sent: 27 May 2020 14:16 > To: Anup Patel <Anup.Patel@wdc.com> > Cc: Eduardo Habkost <ehabkost@redhat.com>; Marcel Apfelbaum > <marcel.apfelbaum@gmail.com>; Peter Maydell <peter.maydell@linaro.org>; > Palmer Dabbelt <palmer@dabbelt.com>; Alistair Francis > <Alistair.Francis@wdc.com>; Sagar Karandikar <sagark@eecs.berkeley.edu>; > Atish Patra <Atish.Patra@wdc.com>; qemu-riscv@nongnu.org; qemu- > devel@nongnu.org; Anup Patel <anup@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.patel@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 :|
On Wed, May 27, 2020 at 09:48:39AM +0000, Anup Patel wrote: > > > > -----Original Message----- > > From: Daniel P. Berrangé <berrange@redhat.com> > > Sent: 27 May 2020 14:16 > > To: Anup Patel <Anup.Patel@wdc.com> > > Cc: Eduardo Habkost <ehabkost@redhat.com>; Marcel Apfelbaum > > <marcel.apfelbaum@gmail.com>; Peter Maydell <peter.maydell@linaro.org>; > > Palmer Dabbelt <palmer@dabbelt.com>; Alistair Francis > > <Alistair.Francis@wdc.com>; Sagar Karandikar <sagark@eecs.berkeley.edu>; > > Atish Patra <Atish.Patra@wdc.com>; qemu-riscv@nongnu.org; qemu- > > devel@nongnu.org; Anup Patel <anup@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 ?? IIUC both of these questions are due to backwards compatibility with pre-existing QEMU versions. Regards, Daniel
> -----Original Message----- > From: Daniel P. Berrangé <berrange@redhat.com> > Sent: 27 May 2020 15:21 > To: Anup Patel <Anup.Patel@wdc.com> > Cc: Eduardo Habkost <ehabkost@redhat.com>; Marcel Apfelbaum > <marcel.apfelbaum@gmail.com>; Peter Maydell <peter.maydell@linaro.org>; > Palmer Dabbelt <palmer@dabbelt.com>; Alistair Francis > <Alistair.Francis@wdc.com>; Sagar Karandikar <sagark@eecs.berkeley.edu>; > Atish Patra <Atish.Patra@wdc.com>; qemu-riscv@nongnu.org; qemu- > devel@nongnu.org; Anup Patel <anup@brainfault.org> > Subject: Re: [PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology > > On Wed, May 27, 2020 at 09:48:39AM +0000, Anup Patel wrote: > > > > > > > -----Original Message----- > > > From: Daniel P. Berrangé <berrange@redhat.com> > > > Sent: 27 May 2020 14:16 > > > To: Anup Patel <Anup.Patel@wdc.com> > > > Cc: Eduardo Habkost <ehabkost@redhat.com>; Marcel Apfelbaum > > > <marcel.apfelbaum@gmail.com>; Peter Maydell > > > <peter.maydell@linaro.org>; Palmer Dabbelt <palmer@dabbelt.com>; > > > Alistair Francis <Alistair.Francis@wdc.com>; Sagar Karandikar > > > <sagark@eecs.berkeley.edu>; Atish Patra <Atish.Patra@wdc.com>; > > > qemu-riscv@nongnu.org; qemu- devel@nongnu.org; Anup Patel > > > <anup@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 ?? > > IIUC both of these questions are due to backwards compatibility with pre- > existing QEMU versions. I see that hw/x86/pc.c implements it's own smp_parse() callback. Can we take that route ?? We need to have sockets=1 by default for RISC-V machines because each socket has it's own interrupt controller and other peripherals. Regards, Anup
On Wed, May 27, 2020 at 10:01:21AM +0000, Anup Patel wrote: > > > > -----Original Message----- > > From: Daniel P. Berrangé <berrange@redhat.com> > > Sent: 27 May 2020 15:21 > > To: Anup Patel <Anup.Patel@wdc.com> > > Cc: Eduardo Habkost <ehabkost@redhat.com>; Marcel Apfelbaum > > <marcel.apfelbaum@gmail.com>; Peter Maydell <peter.maydell@linaro.org>; > > Palmer Dabbelt <palmer@dabbelt.com>; Alistair Francis > > <Alistair.Francis@wdc.com>; Sagar Karandikar <sagark@eecs.berkeley.edu>; > > Atish Patra <Atish.Patra@wdc.com>; qemu-riscv@nongnu.org; qemu- > > devel@nongnu.org; Anup Patel <anup@brainfault.org> > > Subject: Re: [PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology > > > > On Wed, May 27, 2020 at 09:48:39AM +0000, Anup Patel wrote: > > > > > > > > > > -----Original Message----- > > > > From: Daniel P. Berrangé <berrange@redhat.com> > > > > Sent: 27 May 2020 14:16 > > > > To: Anup Patel <Anup.Patel@wdc.com> > > > > Cc: Eduardo Habkost <ehabkost@redhat.com>; Marcel Apfelbaum > > > > <marcel.apfelbaum@gmail.com>; Peter Maydell > > > > <peter.maydell@linaro.org>; Palmer Dabbelt <palmer@dabbelt.com>; > > > > Alistair Francis <Alistair.Francis@wdc.com>; Sagar Karandikar > > > > <sagark@eecs.berkeley.edu>; Atish Patra <Atish.Patra@wdc.com>; > > > > qemu-riscv@nongnu.org; qemu- devel@nongnu.org; Anup Patel > > > > <anup@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 ?? > > > > IIUC both of these questions are due to backwards compatibility with pre- > > existing QEMU versions. > > I see that hw/x86/pc.c implements it's own smp_parse() callback. > Can we take that route ?? > > We need to have sockets=1 by default for RISC-V machines because > each socket has it's own interrupt controller and other peripherals. I guess the fact that smp_parse() exists as a callback pretty much says that allowing machine type overrides of default semantics is permitted. So yeah, using a smp_parse callback seems reasonable. Regards, Daniel
> -----Original Message----- > From: Daniel P. Berrangé <berrange@redhat.com> > Sent: 27 May 2020 16:12 > To: Anup Patel <Anup.Patel@wdc.com> > Cc: Peter Maydell <peter.maydell@linaro.org>; qemu-riscv@nongnu.org; > Eduardo Habkost <ehabkost@redhat.com>; Sagar Karandikar > <sagark@eecs.berkeley.edu>; Anup Patel <anup@brainfault.org>; qemu- > devel@nongnu.org; Atish Patra <Atish.Patra@wdc.com>; Alistair Francis > <Alistair.Francis@wdc.com>; Palmer Dabbelt <palmer@dabbelt.com> > Subject: Re: [PATCH v2 1/5] hw: Add sockets_specified field in CpuTopology > > On Wed, May 27, 2020 at 10:01:21AM +0000, Anup Patel wrote: > > > > > > > -----Original Message----- > > > From: Daniel P. Berrangé <berrange@redhat.com> > > > Sent: 27 May 2020 15:21 > > > To: Anup Patel <Anup.Patel@wdc.com> > > > Cc: Eduardo Habkost <ehabkost@redhat.com>; Marcel Apfelbaum > > > <marcel.apfelbaum@gmail.com>; Peter Maydell > > > <peter.maydell@linaro.org>; Palmer Dabbelt <palmer@dabbelt.com>; > > > Alistair Francis <Alistair.Francis@wdc.com>; Sagar Karandikar > > > <sagark@eecs.berkeley.edu>; Atish Patra <Atish.Patra@wdc.com>; > > > qemu-riscv@nongnu.org; qemu- devel@nongnu.org; Anup Patel > > > <anup@brainfault.org> > > > Subject: Re: [PATCH v2 1/5] hw: Add sockets_specified field in > > > CpuTopology > > > > > > On Wed, May 27, 2020 at 09:48:39AM +0000, Anup Patel wrote: > > > > > > > > > > > > > -----Original Message----- > > > > > From: Daniel P. Berrangé <berrange@redhat.com> > > > > > Sent: 27 May 2020 14:16 > > > > > To: Anup Patel <Anup.Patel@wdc.com> > > > > > Cc: Eduardo Habkost <ehabkost@redhat.com>; Marcel Apfelbaum > > > > > <marcel.apfelbaum@gmail.com>; Peter Maydell > > > > > <peter.maydell@linaro.org>; Palmer Dabbelt <palmer@dabbelt.com>; > > > > > Alistair Francis <Alistair.Francis@wdc.com>; Sagar Karandikar > > > > > <sagark@eecs.berkeley.edu>; Atish Patra <Atish.Patra@wdc.com>; > > > > > qemu-riscv@nongnu.org; qemu- devel@nongnu.org; Anup Patel > > > > > <anup@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 ?? > > > > > > IIUC both of these questions are due to backwards compatibility with > > > pre- existing QEMU versions. > > > > I see that hw/x86/pc.c implements it's own smp_parse() callback. > > Can we take that route ?? > > > > We need to have sockets=1 by default for RISC-V machines because each > > socket has it's own interrupt controller and other peripherals. > > I guess the fact that smp_parse() exists as a callback pretty much says that > allowing machine type overrides of default semantics is permitted. So yeah, > using a smp_parse callback seems reasonable. Thanks Danie, I will drop this patch and send v3 with different approach. Regards, Anup
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;
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. 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.patel@wdc.com> --- hw/core/machine.c | 2 ++ include/hw/boards.h | 2 ++ 2 files changed, 4 insertions(+)