Re: [Qemu-devel] [RFC 2/2] linux-user: Fix cpu_index generation

[Bharata B Rao]

On Thu, Jul 14, 2016 at 3:24 PM, Peter Maydell <address@hidden> wrote:
> On 14 July 2016 at 08:57, David Gibson <address@hidden> wrote:
>> With CONFIG_USER_ONLY, generation of cpu_index values is done differently
>> than for full system targets.  This method turns out to be broken, since
>> it can fairly easily result in duplicate cpu_index values for
>> simultaneously active cpus (i.e. threads in the emulated process).
>>
>> Consider this sequence:
>>     Create thread 1
>>     Create thread 2
>>     Exit thread 1
>>     Create thread 3
>>
>> With the current logic thread 1 will get cpu_index 1, thread 2 will get
>> cpu_index 2 and thread 3 will also get cpu_index 2 (because there are 2
>> threads in the cpus list at the point of its creation).
>>
>> We mostly get away with this because cpu_index values aren't that important
>> for userspace emulation.  Still, it can't be good, so this patch fixes it
>> by making CONFIG_USER_ONLY use the same bitmap based allocation that full
>> system targets already use.
>>
>> Signed-off-by: David Gibson <address@hidden>
>> ---
>>  exec.c | 19 -------------------
>>  1 file changed, 19 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index 011babd..e410dab 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -596,7 +596,6 @@ AddressSpace *cpu_get_address_space(CPUState *cpu, int 
>> asidx)
>>  }
>>  #endif
>>
>> -#ifndef CONFIG_USER_ONLY
>>  static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
>>
>>  static int cpu_get_free_index(Error **errp)
>> @@ -617,24 +616,6 @@ static void cpu_release_index(CPUState *cpu)
>>  {
>>      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
>>  }
>> -#else
>> -
>> -static int cpu_get_free_index(Error **errp)
>> -{
>> -    CPUState *some_cpu;
>> -    int cpu_index = 0;
>> -
>> -    CPU_FOREACH(some_cpu) {
>> -        cpu_index++;
>> -    }
>> -    return cpu_index;
>> -}
>> -
>> -static void cpu_release_index(CPUState *cpu)
>> -{
>> -    return;
>> -}
>> -#endif
>
> Won't this change impose a maximum limit of 256 simultaneous
> threads? That seems a little low for comfort.

This was the reason why the bitmap logic wasn't applied to
CONFIG_USER_ONLY when it was introduced.

https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01980.html

But then we didn't have actual removal, but we do now.

Regards,
Bharata.