This change removes the implied BQL from the cpu_handle_interrupt,
and cpu_handle_exception paths. This BQL acquire is being pushed
down into the per arch implementation.
Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
accel/tcg/cpu-exec.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 80d0e649b2..8e2bfd97a1 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -517,9 +517,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
#else
if (replay_exception()) {
CPUClass *cc = CPU_GET_CLASS(cpu);
- qemu_mutex_lock_iothread();
cc->do_interrupt(cpu);
- qemu_mutex_unlock_iothread();
cpu->exception_index = -1;
if (unlikely(cpu->singlestep_enabled)) {
@@ -558,7 +556,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
if (unlikely(cpu_interrupt_request(cpu))) {
int interrupt_request;
- qemu_mutex_lock_iothread();
+ cpu_mutex_lock(cpu);
interrupt_request = cpu_interrupt_request(cpu);
if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) {
/* Mask out external interrupts for this step. */
@@ -567,7 +565,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
if (interrupt_request & CPU_INTERRUPT_DEBUG) {
cpu_reset_interrupt(cpu, CPU_INTERRUPT_DEBUG);
cpu->exception_index = EXCP_DEBUG;
- qemu_mutex_unlock_iothread();
+ cpu_mutex_unlock(cpu);
return true;
}
if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) {
@@ -577,13 +575,15 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT);
cpu_halted_set(cpu, 1);
cpu->exception_index = EXCP_HLT;
- qemu_mutex_unlock_iothread();
+ cpu_mutex_unlock(cpu);
return true;
}
#if defined(TARGET_I386)
else if (interrupt_request & CPU_INTERRUPT_INIT) {
X86CPU *x86_cpu = X86_CPU(cpu);
CPUArchState *env = &x86_cpu->env;
+ cpu_mutex_unlock(cpu);
+ qemu_mutex_lock_iothread();
replay_interrupt();
cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0);
do_cpu_init(x86_cpu);
@@ -595,7 +595,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
else if (interrupt_request & CPU_INTERRUPT_RESET) {
replay_interrupt();
cpu_reset(cpu);
- qemu_mutex_unlock_iothread();
+ cpu_mutex_unlock(cpu);
return true;
}
#endif
@@ -604,7 +604,9 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
True when it is, and we should restart on a new TB,
and via longjmp via cpu_loop_exit. */
else {
+ cpu_mutex_unlock(cpu);
if (cc->cpu_exec_interrupt(cpu, interrupt_request)) {
+ cpu_mutex_lock(cpu);
replay_interrupt();
/*
* After processing the interrupt, ensure an EXCP_DEBUG is
@@ -614,6 +616,8 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
cpu->exception_index =
(cpu->singlestep_enabled ? EXCP_DEBUG : -1);
*last_tb = NULL;
+ } else {
+ cpu_mutex_lock(cpu);
}
/* The target hook may have updated the 'cpu->interrupt_request';
* reload the 'interrupt_request' value */
@@ -627,7 +631,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
}
/* If we exit via cpu_loop_exit/longjmp it is reset in cpu_exec */
- qemu_mutex_unlock_iothread();
+ cpu_mutex_unlock(cpu);
}
/* Finally, check if we need to exit to the main loop. */
@@ -691,7 +695,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
}
#endif
}
-
/* main execution loop */
int cpu_exec(CPUState *cpu)
--
2.17.1
On 8/5/20 11:12 AM, Robert Foley wrote:
> This change removes the implied BQL from the cpu_handle_interrupt,
> and cpu_handle_exception paths. This BQL acquire is being pushed
> down into the per arch implementation.
>
> Signed-off-by: Robert Foley <robert.foley@linaro.org>
> ---
> accel/tcg/cpu-exec.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 80d0e649b2..8e2bfd97a1 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -517,9 +517,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
> #else
> if (replay_exception()) {
> CPUClass *cc = CPU_GET_CLASS(cpu);
> - qemu_mutex_lock_iothread();
> cc->do_interrupt(cpu);
> - qemu_mutex_unlock_iothread();
> cpu->exception_index = -1;
>
This patch is not bisectable. The removal of the lock here needs to happen at
the end, or something.
r~
On 05/08/20 21:18, Richard Henderson wrote:
> On 8/5/20 11:12 AM, Robert Foley wrote:
>> This change removes the implied BQL from the cpu_handle_interrupt,
>> and cpu_handle_exception paths. This BQL acquire is being pushed
>> down into the per arch implementation.
>>
>> Signed-off-by: Robert Foley <robert.foley@linaro.org>
>> ---
>> accel/tcg/cpu-exec.c | 19 +++++++++++--------
>> 1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>> index 80d0e649b2..8e2bfd97a1 100644
>> --- a/accel/tcg/cpu-exec.c
>> +++ b/accel/tcg/cpu-exec.c
>> @@ -517,9 +517,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
>> #else
>> if (replay_exception()) {
>> CPUClass *cc = CPU_GET_CLASS(cpu);
>> - qemu_mutex_lock_iothread();
>> cc->do_interrupt(cpu);
>> - qemu_mutex_unlock_iothread();
>> cpu->exception_index = -1;
>>
>
> This patch is not bisectable. The removal of the lock here needs to happen at
> the end, or something.
Indeed the series should be structured like this:
1) rename all *_do_interrupt functions to *_do_interrupt_locked
2) add back *_do_interrupt that takes the BQL and calls
*_do_interrupt_locked, point ->do_interrupt to it, remove the BQL from
cpu-exec.c
3) modify the cpu_mutex and BQL critical sections around
->cpu_exec_interrupt, so that the BQL critical section covers just the
call to ->cpu_exec_interrupt. Document which fields are now covered by
cpu_mutex.
4/5) same as 1/2 for ->cpu_exec_interrupt
Patches 1/2 would be pretty large, but they're trivial to review just by
grepping for "->do_interrupt\s*=", and likewise for 4/5.
Thanks,
Paolo
On Thu, 6 Aug 2020 at 05:22, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 05/08/20 21:18, Richard Henderson wrote:
> > On 8/5/20 11:12 AM, Robert Foley wrote:
> >> This change removes the implied BQL from the cpu_handle_interrupt,
> >> and cpu_handle_exception paths. This BQL acquire is being pushed
> >> down into the per arch implementation.
> >>
> >> Signed-off-by: Robert Foley <robert.foley@linaro.org>
> >> ---
> >> accel/tcg/cpu-exec.c | 19 +++++++++++--------
> >> 1 file changed, 11 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> >> index 80d0e649b2..8e2bfd97a1 100644
> >> --- a/accel/tcg/cpu-exec.c
> >> +++ b/accel/tcg/cpu-exec.c
> >> @@ -517,9 +517,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
> >> #else
> >> if (replay_exception()) {
> >> CPUClass *cc = CPU_GET_CLASS(cpu);
> >> - qemu_mutex_lock_iothread();
> >> cc->do_interrupt(cpu);
> >> - qemu_mutex_unlock_iothread();
> >> cpu->exception_index = -1;
> >>
> >
> > This patch is not bisectable. The removal of the lock here needs to happen at
> > the end, or something.
>
> Indeed the series should be structured like this:
>
> 1) rename all *_do_interrupt functions to *_do_interrupt_locked
>
> 2) add back *_do_interrupt that takes the BQL and calls
> *_do_interrupt_locked, point ->do_interrupt to it, remove the BQL from
> cpu-exec.c
>
> 3) modify the cpu_mutex and BQL critical sections around
> ->cpu_exec_interrupt, so that the BQL critical section covers just the
> call to ->cpu_exec_interrupt. Document which fields are now covered by
> cpu_mutex.
>
> 4/5) same as 1/2 for ->cpu_exec_interrupt
>
> Patches 1/2 would be pretty large, but they're trivial to review just by
> grepping for "->do_interrupt\s*=", and likewise for 4/5.
>
Thanks for the details !
It seems like we will have 3 separate patches for this series, 1/2, 3, and 4/5.
We will go in this direction.
Thanks,
-Rob
> Thanks,
>
> Paolo
>
On 06/08/20 18:11, Robert Foley wrote: >> Indeed the series should be structured like this: >> >> 1) rename all *_do_interrupt functions to *_do_interrupt_locked >> >> 2) add back *_do_interrupt that takes the BQL and calls >> *_do_interrupt_locked, point ->do_interrupt to it, remove the BQL from >> cpu-exec.c >> >> 3) modify the cpu_mutex and BQL critical sections around >> ->cpu_exec_interrupt, so that the BQL critical section covers just the >> call to ->cpu_exec_interrupt. Document which fields are now covered by >> cpu_mutex. >> >> 4/5) same as 1/2 for ->cpu_exec_interrupt >> >> Patches 1/2 would be pretty large, but they're trivial to review just by >> grepping for "->do_interrupt\s*=", and likewise for 4/5. >> > > Thanks for the details ! > > It seems like we will have 3 separate patches for this series, 1/2, 3, and 4/5. No, five patches. :) Paolo
The comment around documenting the cpu_mutex fields and critical sections
got us thinking and revisiting our locking assumptions in cpu_handle_interrupt.
Initially we were thinking that removing the BQL from cpu_handle_interrupt
meant that we needed to replace it with the cpu mutex to protect the per cpu
data that is accessed like interrupt_request. We are reconsidering this and
now thinking that the cpu mutex might not be needed here.
BQL is clearly needed to protect the critical section around the call to
->cpu_exec_interrupt. What else is the BQL protecting in cpu_handle_interrupt
that we need to consider? Are we missing anything here?
It's also worth mentioning that we did give this approach a try.
We tried out changes to cpu_handle_interrupt(),
dropping the BQL from all but around ->cpu_exec_interrupt, and not using the
cpu_mutex at all. It seemed to be functional, passing all the tests that
we tried (so far). :)
Thanks,
-Rob
On Thu, 6 Aug 2020 at 12:11, Robert Foley <robert.foley@linaro.org> wrote:
>
> On Thu, 6 Aug 2020 at 05:22, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > On 05/08/20 21:18, Richard Henderson wrote:
> > > On 8/5/20 11:12 AM, Robert Foley wrote:
> > >> This change removes the implied BQL from the cpu_handle_interrupt,
> > >> and cpu_handle_exception paths. This BQL acquire is being pushed
> > >> down into the per arch implementation.
> > >>
> > >> Signed-off-by: Robert Foley <robert.foley@linaro.org>
> > >> ---
> > >> accel/tcg/cpu-exec.c | 19 +++++++++++--------
> > >> 1 file changed, 11 insertions(+), 8 deletions(-)
> > >>
> > >> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> > >> index 80d0e649b2..8e2bfd97a1 100644
> > >> --- a/accel/tcg/cpu-exec.c
> > >> +++ b/accel/tcg/cpu-exec.c
> > >> @@ -517,9 +517,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
> > >> #else
> > >> if (replay_exception()) {
> > >> CPUClass *cc = CPU_GET_CLASS(cpu);
> > >> - qemu_mutex_lock_iothread();
> > >> cc->do_interrupt(cpu);
> > >> - qemu_mutex_unlock_iothread();
> > >> cpu->exception_index = -1;
> > >>
> > >
> > > This patch is not bisectable. The removal of the lock here needs to happen at
> > > the end, or something.
> >
> > Indeed the series should be structured like this:
> >
> > 1) rename all *_do_interrupt functions to *_do_interrupt_locked
> >
> > 2) add back *_do_interrupt that takes the BQL and calls
> > *_do_interrupt_locked, point ->do_interrupt to it, remove the BQL from
> > cpu-exec.c
> >
> > 3) modify the cpu_mutex and BQL critical sections around
> > ->cpu_exec_interrupt, so that the BQL critical section covers just the
> > call to ->cpu_exec_interrupt. Document which fields are now covered by
> > cpu_mutex.
> >
> > 4/5) same as 1/2 for ->cpu_exec_interrupt
> >
> > Patches 1/2 would be pretty large, but they're trivial to review just by
> > grepping for "->do_interrupt\s*=", and likewise for 4/5.
> >
>
> Thanks for the details !
>
> It seems like we will have 3 separate patches for this series, 1/2, 3, and 4/5.
>
> We will go in this direction.
>
> Thanks,
> -Rob
>
> > Thanks,
> >
> > Paolo
> >
We found cases where a few of the *_cpu_exec_interrupt per arch functions
call CPU class's cc->do_interrupt function pointer (for example
arm_cpu_exec_interrupt does this).
This is an issue because *_cpu_exec_interrupt will hold the BQL across the
call to cc->do_interrupt, and *_do_interrupt will also hold the BQL.
Most of the arches do not have this issue because they call the *_do_interrupt
function for that arch directly, and in those cases we will change to call
the function *_do_interrupt_locked.
We see a few possible solutions to this:
1) We could go back to the option of conditionalizing the BQL inside
these few *_do_interrupt functions, only acquiring the BQL if it is not
already held. I counted 3 different arches that directly use the
->do_interrupt
function from their *_cpu_exec_interrupt.
2) Another perhaps cleaner option is to add a new cpu class function
->do_interrupt_locked.
This lets callers like *_cpu_exec_interrupt call to ->do_interrupt_locked
with lock held and solves the issue without resorting to conditional locking.
Another benefit we could gain from this approach is to simplify our solution
overall by adding a common do_interrupt function.
void cpu_common_do_interrupt(CPUState *cs)
{
CPUClass *cc = CPU_GET_CLASS(cpu);
qemu_mutex_lock_iothread();
cc->do_interrupt_locked(cpu);
qemu_mutex_unlock_iothread();
}
cc->do_interrupt would be set to cpu_common_do_interrupt by default
in cpu_class_init.
In other words, the base cpu class would handle holding the BQL for us,
and we would not need to implement a new *_do_interrupt function
for each arch.
We are thinking that 2) would be a good option.
What are the opinions on these possible solutions? Or are there other
solutions that we should consider here?
Thanks & Regards,
-Rob
On Thu, 6 Aug 2020 at 16:04, Robert Foley <robert.foley@linaro.org> wrote:
>
> The comment around documenting the cpu_mutex fields and critical sections
> got us thinking and revisiting our locking assumptions in cpu_handle_interrupt.
>
> Initially we were thinking that removing the BQL from cpu_handle_interrupt
> meant that we needed to replace it with the cpu mutex to protect the per cpu
> data that is accessed like interrupt_request. We are reconsidering this and
> now thinking that the cpu mutex might not be needed here.
>
> BQL is clearly needed to protect the critical section around the call to
> ->cpu_exec_interrupt. What else is the BQL protecting in cpu_handle_interrupt
> that we need to consider? Are we missing anything here?
>
> It's also worth mentioning that we did give this approach a try.
> We tried out changes to cpu_handle_interrupt(),
> dropping the BQL from all but around ->cpu_exec_interrupt, and not using the
> cpu_mutex at all. It seemed to be functional, passing all the tests that
> we tried (so far). :)
>
> Thanks,
> -Rob
>
> On Thu, 6 Aug 2020 at 12:11, Robert Foley <robert.foley@linaro.org> wrote:
> >
> > On Thu, 6 Aug 2020 at 05:22, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >
> > > On 05/08/20 21:18, Richard Henderson wrote:
> > > > On 8/5/20 11:12 AM, Robert Foley wrote:
> > > >> This change removes the implied BQL from the cpu_handle_interrupt,
> > > >> and cpu_handle_exception paths. This BQL acquire is being pushed
> > > >> down into the per arch implementation.
> > > >>
> > > >> Signed-off-by: Robert Foley <robert.foley@linaro.org>
> > > >> ---
> > > >> accel/tcg/cpu-exec.c | 19 +++++++++++--------
> > > >> 1 file changed, 11 insertions(+), 8 deletions(-)
> > > >>
> > > >> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> > > >> index 80d0e649b2..8e2bfd97a1 100644
> > > >> --- a/accel/tcg/cpu-exec.c
> > > >> +++ b/accel/tcg/cpu-exec.c
> > > >> @@ -517,9 +517,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
> > > >> #else
> > > >> if (replay_exception()) {
> > > >> CPUClass *cc = CPU_GET_CLASS(cpu);
> > > >> - qemu_mutex_lock_iothread();
> > > >> cc->do_interrupt(cpu);
> > > >> - qemu_mutex_unlock_iothread();
> > > >> cpu->exception_index = -1;
> > > >>
> > > >
> > > > This patch is not bisectable. The removal of the lock here needs to happen at
> > > > the end, or something.
> > >
> > > Indeed the series should be structured like this:
> > >
> > > 1) rename all *_do_interrupt functions to *_do_interrupt_locked
> > >
> > > 2) add back *_do_interrupt that takes the BQL and calls
> > > *_do_interrupt_locked, point ->do_interrupt to it, remove the BQL from
> > > cpu-exec.c
> > >
> > > 3) modify the cpu_mutex and BQL critical sections around
> > > ->cpu_exec_interrupt, so that the BQL critical section covers just the
> > > call to ->cpu_exec_interrupt. Document which fields are now covered by
> > > cpu_mutex.
> > >
> > > 4/5) same as 1/2 for ->cpu_exec_interrupt
> > >
> > > Patches 1/2 would be pretty large, but they're trivial to review just by
> > > grepping for "->do_interrupt\s*=", and likewise for 4/5.
> > >
> >
> > Thanks for the details !
> >
> > It seems like we will have 3 separate patches for this series, 1/2, 3, and 4/5.
> >
> > We will go in this direction.
> >
> > Thanks,
> > -Rob
> >
> > > Thanks,
> > >
> > > Paolo
> > >
On 08/08/20 00:18, Robert Foley wrote:
> 2) Another perhaps cleaner option is to add a new cpu class function
> ->do_interrupt_locked.
> This lets callers like *_cpu_exec_interrupt call to ->do_interrupt_locked
> with lock held and solves the issue without resorting to conditional locking.
>
> Another benefit we could gain from this approach is to simplify our solution
> overall by adding a common do_interrupt function.
>
> void cpu_common_do_interrupt(CPUState *cs)
> {
> CPUClass *cc = CPU_GET_CLASS(cpu);
> qemu_mutex_lock_iothread();
> cc->do_interrupt_locked(cpu);
> qemu_mutex_unlock_iothread();
> }
> cc->do_interrupt would be set to cpu_common_do_interrupt by default
> in cpu_class_init.
> In other words, the base cpu class would handle holding the BQL for us,
> and we would not need to implement a new *_do_interrupt function
> for each arch.
>
> We are thinking that 2) would be a good option.
Yes, it is. The only slight complication is that you'd have both
->do_interrupt and ->do_interrupt_locked so you probably should add some
consistency check, for example
/*
* cc->do_interrupt_locked should only be needed if
* the class uses cpu_common_do_interrupt.
*/
assert(cc->do_interrupt == cpu_common_do_interrupt ||
!cc->do_interrupt_locked);
Therefore, a variant is to add ->do_interrupt_locked to ARMCPUClass and
CRISCPUClass (target/avr/helper.c can just call
avr_cpu_do_interrupt_locked, because that's the only value that
cc->do_interrupt can have). Then ARM and CRIS can have a do_interrupt
like you wrote above:
void arm_do_interrupt(CPUState *cs)
{
ARMCPUClass *acc = ARM_CPU_GET_CLASS(cs);
qemu_mutex_lock_iothread();
acc->do_interrupt_locked(cpu);
qemu_mutex_unlock_iothread();
}
with a small duplication between ARM and CRIS but on the other hand a
simpler definition of the common CPUClass.
Paolo
On Sat, 8 Aug 2020 at 08:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > We are thinking that 2) would be a good option.
>
> Yes, it is. The only slight complication is that you'd have both
> ->do_interrupt and ->do_interrupt_locked so you probably should add some
> consistency check, for example
>
> /*
> * cc->do_interrupt_locked should only be needed if
> * the class uses cpu_common_do_interrupt.
> */
> assert(cc->do_interrupt == cpu_common_do_interrupt ||
> !cc->do_interrupt_locked);
>
> Therefore, a variant is to add ->do_interrupt_locked to ARMCPUClass and
> CRISCPUClass (target/avr/helper.c can just call
> avr_cpu_do_interrupt_locked, because that's the only value that
> cc->do_interrupt can have). Then ARM and CRIS can have a do_interrupt
> like you wrote above:
>
> void arm_do_interrupt(CPUState *cs)
> {
> ARMCPUClass *acc = ARM_CPU_GET_CLASS(cs);
> qemu_mutex_lock_iothread();
> acc->do_interrupt_locked(cpu);
> qemu_mutex_unlock_iothread();
> }
>
> with a small duplication between ARM and CRIS but on the other hand a
> simpler definition of the common CPUClass.
Thanks for all the details! It sounds like a good approach and we will
move forward with it.
Thanks & Regards,
-Rob
>
> Paolo
>
© 2016 - 2025 Red Hat, Inc.