diff mbox series

[v1,5/8] cpus-common: ensure auto-assigned cpu_indexes don't clash

Message ID 20200513173200.11830-6-alex.bennee@linaro.org
State New
Headers show
Series plugins/next (cleanup, cpu_index and lockstep) | expand

Commit Message

Alex Bennée May 13, 2020, 5:31 p.m. UTC 
Basing the cpu_index on the number of currently allocated vCPUs fails
when vCPUs aren't removed in a LIFO manner. This is especially true
when we are allocating a cpu_index for each guest thread in
linux-user where there is no ordering constraint on their allocation
and de-allocation.

[I've dropped the assert which is there to guard against out-of-order
removal as this should probably be caught higher up the stack. Maybe
we could just ifdef CONFIG_SOFTTMU it?]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Cc: Nikolay Igotti <igotti@gmail.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
---
 cpus-common.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

-- 
2.20.1

Comments

Alex Bennée May 14, 2020, 4:27 p.m. UTC | #1 
a
Alex Bennée <alex.bennee@linaro.org> writes:

> Basing the cpu_index on the number of currently allocated vCPUs fails

> when vCPUs aren't removed in a LIFO manner. This is especially true

> when we are allocating a cpu_index for each guest thread in

> linux-user where there is no ordering constraint on their allocation

> and de-allocation.

>

> [I've dropped the assert which is there to guard against out-of-order

> removal as this should probably be caught higher up the stack. Maybe

> we could just ifdef CONFIG_SOFTTMU it?]

>

> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> Cc: Nikolay Igotti <igotti@gmail.com>

> Cc: Paolo Bonzini <pbonzini@redhat.com>

> Cc: Igor Mammedov <imammedo@redhat.com>

> Cc: Eduardo Habkost <ehabkost@redhat.com>

> ---

>  cpus-common.c | 9 ++++-----

>  1 file changed, 4 insertions(+), 5 deletions(-)

>

> diff --git a/cpus-common.c b/cpus-common.c

> index 55d5df89237..5a7d2f6132b 100644

> --- a/cpus-common.c

> +++ b/cpus-common.c

> @@ -61,13 +61,14 @@ static bool cpu_index_auto_assigned;

>  static int cpu_get_free_index(void)

>  {

>      CPUState *some_cpu;

> -    int cpu_index = 0;

> +    int max_cpu_index = 0;

>  

>      cpu_index_auto_assigned = true;

>      CPU_FOREACH(some_cpu) {

> -        cpu_index++;

> +        max_cpu_index = MAX(some_cpu->cpu_index, max_cpu_index);

>      }

> -    return cpu_index;

> +    max_cpu_index++;

> +    return max_cpu_index;

>  }


OK some ending up with cpu_index = 1 threw off devices that would do
qemu_get_cpu(0) so I've tweaked the algorithm to:

  static int cpu_get_free_index(void)
  {
      CPUState *some_cpu;
      int max_cpu_index = 0;

      cpu_index_auto_assigned = true;
      CPU_FOREACH(some_cpu) {
          if (some_cpu->cpu_index >= max_cpu_index) {
              max_cpu_index = some_cpu->cpu_index + 1;
          }
      }
      return max_cpu_index;
  }

>  

>  void cpu_list_add(CPUState *cpu)

> @@ -90,8 +91,6 @@ void cpu_list_remove(CPUState *cpu)

>          return;

>      }

>  

> -    assert(!(cpu_index_auto_assigned && cpu != QTAILQ_LAST(&cpus)));

> -

>      QTAILQ_REMOVE_RCU(&cpus, cpu, node);

>      cpu->cpu_index = UNASSIGNED_CPU_INDEX;

>  }



-- 
Alex Bennée
Igor Mammedov May 21, 2020, 3:53 p.m. UTC | #2 
On Thu, 14 May 2020 17:27:53 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:

> a

> Alex Bennée <alex.bennee@linaro.org> writes:

> 

> > Basing the cpu_index on the number of currently allocated vCPUs fails

> > when vCPUs aren't removed in a LIFO manner. This is especially true

> > when we are allocating a cpu_index for each guest thread in

> > linux-user where there is no ordering constraint on their allocation

> > and de-allocation.

> >

> > [I've dropped the assert which is there to guard against out-of-order

> > removal as this should probably be caught higher up the stack. Maybe

> > we could just ifdef CONFIG_SOFTTMU it?]


for machines where we care about cross version migration (arm/virt,s390,x86,spapr),
we do manual cpu_index assignment on keep control on its stability
So orderining probably shouldn't matter for other softmmu boards,
but what I'd watch for is arrays within devices where cpu_index is used as index
(ex: would be apic emulation (but its not affected by this patch since x86 control
cpu_index assignment))


> >

> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> > Cc: Nikolay Igotti <igotti@gmail.com>

> > Cc: Paolo Bonzini <pbonzini@redhat.com>

> > Cc: Igor Mammedov <imammedo@redhat.com>

> > Cc: Eduardo Habkost <ehabkost@redhat.com>

> > ---

> >  cpus-common.c | 9 ++++-----

> >  1 file changed, 4 insertions(+), 5 deletions(-)

> >

> > diff --git a/cpus-common.c b/cpus-common.c

> > index 55d5df89237..5a7d2f6132b 100644

> > --- a/cpus-common.c

> > +++ b/cpus-common.c

> > @@ -61,13 +61,14 @@ static bool cpu_index_auto_assigned;

> >  static int cpu_get_free_index(void)

> >  {

> >      CPUState *some_cpu;

> > -    int cpu_index = 0;

> > +    int max_cpu_index = 0;

> >  

> >      cpu_index_auto_assigned = true;

> >      CPU_FOREACH(some_cpu) {

> > -        cpu_index++;

> > +        max_cpu_index = MAX(some_cpu->cpu_index, max_cpu_index);

> >      }

> > -    return cpu_index;

> > +    max_cpu_index++;

> > +    return max_cpu_index;

> >  }  

> 

> OK some ending up with cpu_index = 1 threw off devices that would do

> qemu_get_cpu(0) so I've tweaked the algorithm to:

> 

>   static int cpu_get_free_index(void)

>   {

>       CPUState *some_cpu;

>       int max_cpu_index = 0;

> 

>       cpu_index_auto_assigned = true;

>       CPU_FOREACH(some_cpu) {

>           if (some_cpu->cpu_index >= max_cpu_index) {

>               max_cpu_index = some_cpu->cpu_index + 1;

>           }

>       }

>       return max_cpu_index;

>   }

> 

> >  

> >  void cpu_list_add(CPUState *cpu)

> > @@ -90,8 +91,6 @@ void cpu_list_remove(CPUState *cpu)

> >          return;

> >      }

> >  

> > -    assert(!(cpu_index_auto_assigned && cpu != QTAILQ_LAST(&cpus)));

> > -

> >      QTAILQ_REMOVE_RCU(&cpus, cpu, node);

> >      cpu->cpu_index = UNASSIGNED_CPU_INDEX;

> >  }  

> 

>
Alex Bennée May 21, 2020, 5:10 p.m. UTC | #3 
Igor Mammedov <imammedo@redhat.com> writes:

> On Thu, 14 May 2020 17:27:53 +0100

> Alex Bennée <alex.bennee@linaro.org> wrote:

>

>> a

>> Alex Bennée <alex.bennee@linaro.org> writes:

>> 

>> > Basing the cpu_index on the number of currently allocated vCPUs fails

>> > when vCPUs aren't removed in a LIFO manner. This is especially true

>> > when we are allocating a cpu_index for each guest thread in

>> > linux-user where there is no ordering constraint on their allocation

>> > and de-allocation.

>> >

>> > [I've dropped the assert which is there to guard against out-of-order

>> > removal as this should probably be caught higher up the stack. Maybe

>> > we could just ifdef CONFIG_SOFTTMU it?]

>

> for machines where we care about cross version migration (arm/virt,s390,x86,spapr),

> we do manual cpu_index assignment on keep control on its stability

> So orderining probably shouldn't matter for other softmmu boards,

> but what I'd watch for is arrays within devices where cpu_index is

> used as index


With the updated version for softmmu you should get the same indexes as
before from startup. It only gets complicated if CPU hotplug is a thing
which I think is only the case for machines that also support migration?

> (ex: would be apic emulation (but its not affected by this patch since x86 control

> cpu_index assignment))


I've just noticed that the gdbstub uses the maximum cpu_index at startup
to size it's array in CONFIG_USER which is obviously wrong but it was
wrong before so I guess that's another bug to look into on my part :-/

>

>

>> >

>> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>> > Cc: Nikolay Igotti <igotti@gmail.com>

>> > Cc: Paolo Bonzini <pbonzini@redhat.com>

>> > Cc: Igor Mammedov <imammedo@redhat.com>

>> > Cc: Eduardo Habkost <ehabkost@redhat.com>

>> > ---

>> >  cpus-common.c | 9 ++++-----

>> >  1 file changed, 4 insertions(+), 5 deletions(-)

>> >

>> > diff --git a/cpus-common.c b/cpus-common.c

>> > index 55d5df89237..5a7d2f6132b 100644

>> > --- a/cpus-common.c

>> > +++ b/cpus-common.c

>> > @@ -61,13 +61,14 @@ static bool cpu_index_auto_assigned;

>> >  static int cpu_get_free_index(void)

>> >  {

>> >      CPUState *some_cpu;

>> > -    int cpu_index = 0;

>> > +    int max_cpu_index = 0;

>> >  

>> >      cpu_index_auto_assigned = true;

>> >      CPU_FOREACH(some_cpu) {

>> > -        cpu_index++;

>> > +        max_cpu_index = MAX(some_cpu->cpu_index, max_cpu_index);

>> >      }

>> > -    return cpu_index;

>> > +    max_cpu_index++;

>> > +    return max_cpu_index;

>> >  }  

>> 

>> OK some ending up with cpu_index = 1 threw off devices that would do

>> qemu_get_cpu(0) so I've tweaked the algorithm to:

>> 

>>   static int cpu_get_free_index(void)

>>   {

>>       CPUState *some_cpu;

>>       int max_cpu_index = 0;

>> 

>>       cpu_index_auto_assigned = true;

>>       CPU_FOREACH(some_cpu) {

>>           if (some_cpu->cpu_index >= max_cpu_index) {

>>               max_cpu_index = some_cpu->cpu_index + 1;

>>           }

>>       }

>>       return max_cpu_index;

>>   }

>> 

>> >  

>> >  void cpu_list_add(CPUState *cpu)

>> > @@ -90,8 +91,6 @@ void cpu_list_remove(CPUState *cpu)

>> >          return;

>> >      }

>> >  

>> > -    assert(!(cpu_index_auto_assigned && cpu != QTAILQ_LAST(&cpus)));

>> > -

>> >      QTAILQ_REMOVE_RCU(&cpus, cpu, node);

>> >      cpu->cpu_index = UNASSIGNED_CPU_INDEX;

>> >  }  

>> 

>> 



-- 
Alex Bennée
Igor Mammedov May 22, 2020, 8:46 a.m. UTC | #4 
On Thu, 21 May 2020 18:10:40 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:

> Igor Mammedov <imammedo@redhat.com> writes:

> 

> > On Thu, 14 May 2020 17:27:53 +0100

> > Alex Bennée <alex.bennee@linaro.org> wrote:

> >  

> >> a

> >> Alex Bennée <alex.bennee@linaro.org> writes:

> >>   

> >> > Basing the cpu_index on the number of currently allocated vCPUs

> >> > fails when vCPUs aren't removed in a LIFO manner. This is

> >> > especially true when we are allocating a cpu_index for each

> >> > guest thread in linux-user where there is no ordering constraint

> >> > on their allocation and de-allocation.

> >> >

> >> > [I've dropped the assert which is there to guard against

> >> > out-of-order removal as this should probably be caught higher up

> >> > the stack. Maybe we could just ifdef CONFIG_SOFTTMU it?]  

> >

> > for machines where we care about cross version migration

> > (arm/virt,s390,x86,spapr), we do manual cpu_index assignment on

> > keep control on its stability So orderining probably shouldn't

> > matter for other softmmu boards, but what I'd watch for is arrays

> > within devices where cpu_index is used as index  

> 

> With the updated version for softmmu you should get the same indexes

> as before from startup. It only gets complicated if CPU hotplug is a

> thing which I think is only the case for machines that also support

> migration?

I'd think so, and those do not (should not) use cpu_get_free_index(), so

Acked-by: Igor Mammedow <imammedo@redhat.com>


> 

> > (ex: would be apic emulation (but its not affected by this patch

> > since x86 control cpu_index assignment))  

> 

> I've just noticed that the gdbstub uses the maximum cpu_index at

> startup to size it's array in CONFIG_USER which is obviously wrong

> but it was wrong before so I guess that's another bug to look into on

> my part :-/

> 

> >

> >  

> >> >

> >> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

> >> > Cc: Nikolay Igotti <igotti@gmail.com>

> >> > Cc: Paolo Bonzini <pbonzini@redhat.com>

> >> > Cc: Igor Mammedov <imammedo@redhat.com>

> >> > Cc: Eduardo Habkost <ehabkost@redhat.com>

> >> > ---

> >> >  cpus-common.c | 9 ++++-----

> >> >  1 file changed, 4 insertions(+), 5 deletions(-)

> >> >

> >> > diff --git a/cpus-common.c b/cpus-common.c

> >> > index 55d5df89237..5a7d2f6132b 100644

> >> > --- a/cpus-common.c

> >> > +++ b/cpus-common.c

> >> > @@ -61,13 +61,14 @@ static bool cpu_index_auto_assigned;

> >> >  static int cpu_get_free_index(void)

> >> >  {

> >> >      CPUState *some_cpu;

> >> > -    int cpu_index = 0;

> >> > +    int max_cpu_index = 0;

> >> >  

> >> >      cpu_index_auto_assigned = true;

> >> >      CPU_FOREACH(some_cpu) {

> >> > -        cpu_index++;

> >> > +        max_cpu_index = MAX(some_cpu->cpu_index, max_cpu_index);

> >> >      }

> >> > -    return cpu_index;

> >> > +    max_cpu_index++;

> >> > +    return max_cpu_index;

> >> >  }    

> >> 

> >> OK some ending up with cpu_index = 1 threw off devices that would

> >> do qemu_get_cpu(0) so I've tweaked the algorithm to:

> >> 

> >>   static int cpu_get_free_index(void)

> >>   {

> >>       CPUState *some_cpu;

> >>       int max_cpu_index = 0;

> >> 

> >>       cpu_index_auto_assigned = true;

> >>       CPU_FOREACH(some_cpu) {

> >>           if (some_cpu->cpu_index >= max_cpu_index) {

> >>               max_cpu_index = some_cpu->cpu_index + 1;

> >>           }

> >>       }

> >>       return max_cpu_index;

> >>   }

> >>   

> >> >  

> >> >  void cpu_list_add(CPUState *cpu)

> >> > @@ -90,8 +91,6 @@ void cpu_list_remove(CPUState *cpu)

> >> >          return;

> >> >      }

> >> >  

> >> > -    assert(!(cpu_index_auto_assigned && cpu !=

> >> > QTAILQ_LAST(&cpus))); -

> >> >      QTAILQ_REMOVE_RCU(&cpus, cpu, node);

> >> >      cpu->cpu_index = UNASSIGNED_CPU_INDEX;

> >> >  }    

> >> 

> >>   

> 

>
diff mbox series

Patch

diff --git a/cpus-common.c b/cpus-common.c
index 55d5df89237..5a7d2f6132b 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -61,13 +61,14 @@  static bool cpu_index_auto_assigned;
 static int cpu_get_free_index(void)
 {
     CPUState *some_cpu;
-    int cpu_index = 0;
+    int max_cpu_index = 0;
 
     cpu_index_auto_assigned = true;
     CPU_FOREACH(some_cpu) {
-        cpu_index++;
+        max_cpu_index = MAX(some_cpu->cpu_index, max_cpu_index);
     }
-    return cpu_index;
+    max_cpu_index++;
+    return max_cpu_index;
 }
 
 void cpu_list_add(CPUState *cpu)
@@ -90,8 +91,6 @@  void cpu_list_remove(CPUState *cpu)
         return;
     }
 
-    assert(!(cpu_index_auto_assigned && cpu != QTAILQ_LAST(&cpus)));
-
     QTAILQ_REMOVE_RCU(&cpus, cpu, node);
     cpu->cpu_index = UNASSIGNED_CPU_INDEX;
 }