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

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 4822 bytes --]

On Mon, 18 Jul 2016 11:17:25 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Sat, Jul 16, 2016 at 12:11:56AM +0200, Greg Kurz wrote:
> > On Thu, 14 Jul 2016 21:59:45 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Thu, Jul 14, 2016 at 03:50:56PM +0530, Bharata B Rao wrote:  
> > > > On Thu, Jul 14, 2016 at 3:24 PM, Peter Maydell <peter.maydell@linaro.org> wrote:    
> > > > > On 14 July 2016 at 08:57, David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > > > >> ---
> > > > >>  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    
> > > 
> > > Ah.. good point.
> > > 
> > > Hrm, ok, my next idea would be to just (globally) sequentially
> > > allocate cpu_index values for CONFIG_USER, and never try to re-use
> > > them.  Does that seem reasonable?
> > >   
> > 
> > Isn't it only deferring the problem to later ?  
> 
> You mean that we could get duplicate indexes after the value wraps
> around?
> 

Yes.

> I suppose, but duplicates after spawning 4 billion threads seems like
> a substantial improvement over duplicates after spawning 3 in the
> wrong order..
> 

Agreed.

It takes ~ 20 seconds to user QEMU to spawn 10000 threads on my palmetto
box, so the wrap around would occur after ~ 100 days. :)

> > Maybe it is possible to define MAX_CPUMASK_BITS to a much higher
> > value fo CONFIG_USER only instead ?  
> 
> Perhaps.  It does mean carrying around a huge bitmap, though.
> 
> Another option is to remove cpu_index entirely for the user only
> case.  I have some patches for this, which are very ugly but it's
> possible they can be cleaned up to something reasonable (the biggest
> chunk is moving a bunch of ARM stuff under #ifndef CONFIG_USER_ONLY
> for what I think are registers that aren't accessible in user mode).
> 
> 
> > > > But then we didn't have actual removal, but we do now.    
> > > 
> > > You mean patch 1/2 in this set?  Or something else?
> > > 
> > > Even so, 256 does seem a bit low for a number of simultaneously active
> > > threads - there are some bug hairy multi-threaded programs out there.
> > >   
> >   
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]