| Message ID | 529EC2C3.3050703@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 04, 2013 at 04:50:59PM +1100, Alexey Kardashevskiy wrote: > On 12/04/2013 01:47 AM, Eduardo Habkost wrote: > > On Tue, Dec 03, 2013 at 02:30:48PM +0100, Andreas Färber wrote: > >> Am 03.12.2013 00:03, schrieb Alexey Kardashevskiy: > >>> On 12/03/2013 09:09 AM, Andreas Färber wrote: > >>>> Am 02.12.2013 18:06, schrieb Michael Tokarev: > >>>>> 25.11.2013 07:39, Alexey Kardashevskiy wrote: > >>>>>> Since modern POWER7/POWER8 chips can have more that 256 CPU threads > >>>>>> (>2000 actually), remove this check from smp_parse. > >>>>>> > >>>>>> The CPUs number is still checked against machine->max_cpus and this check > >>>>>> should be enough not to break other archs. > >>>> > >>>> "should be" is not exactly the highest level of confidence for a > >>>> "trivial" patch... :/ > >> [...] > >>>> Alexey, did you actually check that, e.g., x86 machines don't break with > >>>> 256 or 257 CPUs now? > >>> > >>> PC_DEFAULT_MACHINE_OPTIONS sets it to 255. And I cannot find any machine > >>> which would not define max_cpus, have I missed any? > >> > >> If you've actually *checked* the other machines' code then fine with me, > >> just say so in the commit message. :) > > > > I just grepped for "max_cpus" and checked every match. The largest > > values I found were: > > > > hw/ppc/spapr.c: 256 > > s390: 255 > > pc: 255 > > > > All the rest had values <= 32. > > > > Machines with missing max_cpus value shouldn't be a problem, as > > max_cpus==0 is interpreted as 1 by the vl.c code. > > > > But we still need to add a check for max_cpus > machine->max_cpus to > > vl.c, before we eliminate the smp_parse() check. > > > Since smp_parse() checks if (max_cpus >= smp_cpus), this should just work: > > diff --git a/vl.c b/vl.c > index e6ed260..544165a 100644 > --- a/vl.c > +++ b/vl.c > @@ -3882,9 +3882,9 @@ int main(int argc, char **argv, char **envp) > smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); > > machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */ > - if (smp_cpus > machine->max_cpus) { > + if (max_cpus > machine->max_cpus) { > fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus " > - "supported by machine `%s' (%d)\n", smp_cpus, machine->name, > + "supported by machine `%s' (%d)\n", max_cpus, machine->name, > machine->max_cpus); > exit(1); > } > > > > There's also this, at main(): > > > > if (i == nb_numa_nodes) { > > for (i = 0; i < max_cpus; i++) { > > set_bit(i, node_cpumask[i % nb_numa_nodes]); > > } > > } > > > > node_cpumask[] is initialized using bitmap_new(MAX_CPUMASK_BITS), and > > MAX_CPUMASK_BITS is 255. To fix this, we can initialize node_cpumask[] using > > max_cpus instead, if we initialize it after smp_parse(). > > > Nope. At the moment when we parse -numa in vl.c, we may not know yet what > machine is going to be used and machines can have different max_cpus. This will be changed by: Subject: [PATCH V17 04/11] NUMA: convert -numa option to use OptsVisitor Message-Id: <1386143939-19142-5-git-send-email-gaowanlong@cn.fujitsu.com> http://article.gmane.org/gmane.comp.emulators.qemu/244826 > > For now, I would simply change MAX_CPUMASK_BITS to something crazy, like > 16384 (2KB per numa node), I hope QEMU can survive such a memory waste :) > > Ok? I'm OK with that as long the code has proper checks in case max_cpus gets set to a crazily large value (larger than MAX_CPUMASK_BITS) in the far future, or if we prevent max_cpus from being larger than MAX_CPUMASK_BITS.
On 12/04/2013 11:48 PM, Eduardo Habkost wrote: > On Wed, Dec 04, 2013 at 04:50:59PM +1100, Alexey Kardashevskiy wrote: >> On 12/04/2013 01:47 AM, Eduardo Habkost wrote: >>> On Tue, Dec 03, 2013 at 02:30:48PM +0100, Andreas Färber wrote: >>>> Am 03.12.2013 00:03, schrieb Alexey Kardashevskiy: >>>>> On 12/03/2013 09:09 AM, Andreas Färber wrote: >>>>>> Am 02.12.2013 18:06, schrieb Michael Tokarev: >>>>>>> 25.11.2013 07:39, Alexey Kardashevskiy wrote: >>>>>>>> Since modern POWER7/POWER8 chips can have more that 256 CPU threads >>>>>>>> (>2000 actually), remove this check from smp_parse. >>>>>>>> >>>>>>>> The CPUs number is still checked against machine->max_cpus and this check >>>>>>>> should be enough not to break other archs. >>>>>> >>>>>> "should be" is not exactly the highest level of confidence for a >>>>>> "trivial" patch... :/ >>>> [...] >>>>>> Alexey, did you actually check that, e.g., x86 machines don't break with >>>>>> 256 or 257 CPUs now? >>>>> >>>>> PC_DEFAULT_MACHINE_OPTIONS sets it to 255. And I cannot find any machine >>>>> which would not define max_cpus, have I missed any? >>>> >>>> If you've actually *checked* the other machines' code then fine with me, >>>> just say so in the commit message. :) >>> >>> I just grepped for "max_cpus" and checked every match. The largest >>> values I found were: >>> >>> hw/ppc/spapr.c: 256 >>> s390: 255 >>> pc: 255 >>> >>> All the rest had values <= 32. >>> >>> Machines with missing max_cpus value shouldn't be a problem, as >>> max_cpus==0 is interpreted as 1 by the vl.c code. >>> >>> But we still need to add a check for max_cpus > machine->max_cpus to >>> vl.c, before we eliminate the smp_parse() check. >> >> >> Since smp_parse() checks if (max_cpus >= smp_cpus), this should just work: >> >> diff --git a/vl.c b/vl.c >> index e6ed260..544165a 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -3882,9 +3882,9 @@ int main(int argc, char **argv, char **envp) >> smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); >> >> machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */ >> - if (smp_cpus > machine->max_cpus) { >> + if (max_cpus > machine->max_cpus) { >> fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus " >> - "supported by machine `%s' (%d)\n", smp_cpus, machine->name, >> + "supported by machine `%s' (%d)\n", max_cpus, machine->name, >> machine->max_cpus); >> exit(1); >> } >> >> >>> There's also this, at main(): >>> >>> if (i == nb_numa_nodes) { >>> for (i = 0; i < max_cpus; i++) { >>> set_bit(i, node_cpumask[i % nb_numa_nodes]); >>> } >>> } >>> >>> node_cpumask[] is initialized using bitmap_new(MAX_CPUMASK_BITS), and >>> MAX_CPUMASK_BITS is 255. To fix this, we can initialize node_cpumask[] using >>> max_cpus instead, if we initialize it after smp_parse(). >> >> >> Nope. At the moment when we parse -numa in vl.c, we may not know yet what >> machine is going to be used and machines can have different max_cpus. > > This will be changed by: > > Subject: [PATCH V17 04/11] NUMA: convert -numa option to use OptsVisitor > Message-Id: <1386143939-19142-5-git-send-email-gaowanlong@cn.fujitsu.com> > http://article.gmane.org/gmane.comp.emulators.qemu/244826 Any progress with this? Thanks. >> >> For now, I would simply change MAX_CPUMASK_BITS to something crazy, like >> 16384 (2KB per numa node), I hope QEMU can survive such a memory waste :) >> >> Ok? > > I'm OK with that as long the code has proper checks in case max_cpus > gets set to a crazily large value (larger than MAX_CPUMASK_BITS) in the > far future, or if we prevent max_cpus from being larger than > MAX_CPUMASK_BITS. >
Il 14/02/2014 07:56, Alexey Kardashevskiy ha scritto: >> > Subject: [PATCH V17 04/11] NUMA: convert -numa option to use OptsVisitor >> > Message-Id: <1386143939-19142-5-git-send-email-gaowanlong@cn.fujitsu.com> >> > http://article.gmane.org/gmane.comp.emulators.qemu/244826 > > Any progress with this? Thanks. Yes, there was some progress though for some reason it happened onlist. Igor/Wanlong, can you send the first patches of the "common" NUMA/memory series, i.e. excluding the memdev backend? Paolo
diff --git a/vl.c b/vl.c index e6ed260..544165a 100644 --- a/vl.c +++ b/vl.c @@ -3882,9 +3882,9 @@ int main(int argc, char **argv, char **envp) smp_parse(qemu_opts_find(qemu_find_opts("smp-opts"), NULL)); machine->max_cpus = machine->max_cpus ?: 1; /* Default to UP */ - if (smp_cpus > machine->max_cpus) { + if (max_cpus > machine->max_cpus) { fprintf(stderr, "Number of SMP cpus requested (%d), exceeds max cpus " - "supported by machine `%s' (%d)\n", smp_cpus, machine->name, + "supported by machine `%s' (%d)\n", max_cpus, machine->name, machine->max_cpus); exit(1); }