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

[David Gibson]

On Mon, Jul 18, 2016 at 09:25:58AM +0200, Igor Mammedov wrote:
> On Mon, 18 Jul 2016 11:17:25 +1000
> David Gibson <address@hidden> 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 <address@hidden> 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 <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   
> > > > >  
> > > > 
> > > > 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?
> > 
> > I suppose, but duplicates after spawning 4 billion threads seems like
> > a substantial improvement over duplicates after spawning 3 in the
> > wrong order..
> > 
> > > 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).
> could we remove cpu_index altogether for bot *-user and *-softmmu targets?

Well.. not in the same way I'm looking at removing it for *-user, at
any rate.  From looking through all the users of cpu_index, nearly all
of them are in two categories:

    1) Labelling debug or error messages with a CPU #

There's not something obvious to replace this with for *-softmmu.  For
*-user, however, we can use the host tid, which is probably more
useful than an essentially arbitrary cpu index.

     2) Initializing cpu specific registers

That's "cpu specific" in both the sense of ISA specific and in the
sense of specific to a particular CPU in an SMP system.  These
registers are generally privileged and so don't need to be emulated
for *-user.  Finding a substitute for *-softmmu is rather harder.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature