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.mayd...@linaro.org>
> wrote:
> > On 14 July 2016 at 08:57, David Gibson <da...@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 <da...@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?
> 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.
--
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
