Re: [Qemu-trivial] [Qemu-devel] [PATCH] vl: remove (max_cpus > 255) check from smp_parse

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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.
> 


-- 
Alexey