| Message ID | 20200513173200.11830-6-alex.bennee@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | plugins/next (cleanup, cpu_index and lockstep) | expand |
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; > }
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; > > } > >
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; >> > } >> >>
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 --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; }
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(-)