| Message ID | yt9d5ykkhrvv.fsf_-_@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu-system-s390x hang in tcg (was: Re: [PATCH v8 23/70] mm/mmap: change do_brk_flags() to expand existing VMA and add do_brk_munmap()) | expand |
Alex Bennée <alex.bennee@linaro.org> writes: > Sven Schnelle <svens@linux.ibm.com> writes: > >> Hi, >> >> David Hildenbrand <david@redhat.com> writes: >> >>> On 04.05.22 09:37, Janosch Frank wrote: >>>> I had a short look yesterday and the boot usually hangs in the raid6 >>>> code. Disabling vector instructions didn't make a difference but a few >>>> interruptions via GDB solve the problem for some reason. >>>> >>>> CCing David and Thomas for TCG >>>> >>> >>> I somehow recall that KASAN was always disabled under TCG, I might be >>> wrong (I thought we'd get a message early during boot that the HW >>> doesn't support KASAN). >>> >>> I recall that raid code is a heavy user of vector instructions. >>> >>> How can I reproduce? Compile upstream (or -next?) with kasan support and >>> run it under TCG? >> >> I spent some time looking into this. It's usually hanging in >> s390vx8_gen_syndrome(). My first thought was that it is a problem with >> the VX instructions, but turned out that it hangs even if i remove all >> the code from s390vx8_gen_syndrome(). >> >> Tracing the execution of TB's, i see that the generated code is always >> jumping between a few TB's, but never exiting the TB's to check for >> interrupts (i.e. return to cpu_tb_exec(). I only see calls to >> helper_lookup_tb_ptr to lookup the tb pointer for the next TB. >> >> The raid6 code is waiting for some time to expire by reading jiffies, >> but interrupts are never processed and therefore jiffies doesn't change. >> So the raid6 code hangs forever. >> >> As a test, i made a quick change to test: >> >> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c >> index c997c2e8e0..35819fd5a7 100644 >> --- a/accel/tcg/cpu-exec.c >> +++ b/accel/tcg/cpu-exec.c >> @@ -319,7 +319,8 @@ const void *HELPER(lookup_tb_ptr)(CPUArchState *env) >> cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags); >> >> cflags = curr_cflags(cpu); >> - if (check_for_breakpoints(cpu, pc, &cflags)) { >> + if (check_for_breakpoints(cpu, pc, &cflags) || >> + unlikely(qatomic_read(&cpu->interrupt_request))) { >> cpu_loop_exit(cpu); >> } >> >> And that makes the problem go away. But i'm not familiar with the TCG >> internals, so i can't say whether the generated code is incorrect or >> something else is wrong. I have tcg log files of a failing + working run >> if someone wants to take a look. They are rather large so i would have to >> upload them somewhere. > > Whatever is setting cpu->interrupt_request should be calling > cpu_exit(cpu) which sets the exit flag which is checked at the start of > every TB execution (see gen_tb_start). Thanks, that was very helpful. I added debugging and it turned out that the TB is left because of a pending irq. The code then calls s390_cpu_exec_interrupt: bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request) { if (interrupt_request & CPU_INTERRUPT_HARD) { S390CPU *cpu = S390_CPU(cs); CPUS390XState *env = &cpu->env; if (env->ex_value) { /* Execution of the target insn is indivisible from the parent EXECUTE insn. */ return false; } if (s390_cpu_has_int(cpu)) { s390_cpu_do_interrupt(cs); return true; } if (env->psw.mask & PSW_MASK_WAIT) { /* Woken up because of a floating interrupt but it has already * been delivered. Go back to sleep. */ cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HALT); } } return false; } Note the 'if (env->ex_value) { }' check. It looks like this function just returns false in case tcg is executing an EX instruction. After that the information that the TB should be exited because of an interrupt is gone. So the TB's are never exited again, although the interrupt wasn't handled. At least that's my assumption now, if i'm wrong please tell me. So the raid6 code is spinning waiting that the jiffies value reaches a timeout, but as the timer interrupt was lost it will never change. So i wonder now how this could be fixed.
Sven Schnelle <svens@linux.ibm.com> writes: > Sven Schnelle <svens@linux.ibm.com> writes: > >> Alex Bennée <alex.bennee@linaro.org> writes: >> >>> Sven Schnelle <svens@linux.ibm.com> writes: >>> >>>> Hi, >>>> >>>> David Hildenbrand <david@redhat.com> writes: >>>> >>>>> On 04.05.22 09:37, Janosch Frank wrote: >>>>>> I had a short look yesterday and the boot usually hangs in the raid6 >>>>>> code. Disabling vector instructions didn't make a difference but a few >>>>>> interruptions via GDB solve the problem for some reason. >>>>>> >>>>>> CCing David and Thomas for TCG >>>>>> >>>>> >>>>> I somehow recall that KASAN was always disabled under TCG, I might be >>>>> wrong (I thought we'd get a message early during boot that the HW >>>>> doesn't support KASAN). >>>>> >>>>> I recall that raid code is a heavy user of vector instructions. >>>>> >>>>> How can I reproduce? Compile upstream (or -next?) with kasan support and >>>>> run it under TCG? >>>> >>>> I spent some time looking into this. It's usually hanging in >>>> s390vx8_gen_syndrome(). My first thought was that it is a problem with >>>> the VX instructions, but turned out that it hangs even if i remove all >>>> the code from s390vx8_gen_syndrome(). >>>> >>>> Tracing the execution of TB's, i see that the generated code is always >>>> jumping between a few TB's, but never exiting the TB's to check for >>>> interrupts (i.e. return to cpu_tb_exec(). I only see calls to >>>> helper_lookup_tb_ptr to lookup the tb pointer for the next TB. >>>> >>>> The raid6 code is waiting for some time to expire by reading jiffies, >>>> but interrupts are never processed and therefore jiffies doesn't change. >>>> So the raid6 code hangs forever. >>>> >>>> As a test, i made a quick change to test: >>>> >>>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c >>>> index c997c2e8e0..35819fd5a7 100644 >>>> --- a/accel/tcg/cpu-exec.c >>>> +++ b/accel/tcg/cpu-exec.c >>>> @@ -319,7 +319,8 @@ const void *HELPER(lookup_tb_ptr)(CPUArchState *env) >>>> cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags); >>>> >>>> cflags = curr_cflags(cpu); >>>> - if (check_for_breakpoints(cpu, pc, &cflags)) { >>>> + if (check_for_breakpoints(cpu, pc, &cflags) || >>>> + unlikely(qatomic_read(&cpu->interrupt_request))) { >>>> cpu_loop_exit(cpu); >>>> } >>>> >>>> And that makes the problem go away. But i'm not familiar with the TCG >>>> internals, so i can't say whether the generated code is incorrect or >>>> something else is wrong. I have tcg log files of a failing + working run >>>> if someone wants to take a look. They are rather large so i would have to >>>> upload them somewhere. >>> >>> Whatever is setting cpu->interrupt_request should be calling >>> cpu_exit(cpu) which sets the exit flag which is checked at the start of >>> every TB execution (see gen_tb_start). >> >> Thanks, that was very helpful. I added debugging and it turned out >> that the TB is left because of a pending irq. The code then calls >> s390_cpu_exec_interrupt: >> >> bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request) >> { >> if (interrupt_request & CPU_INTERRUPT_HARD) { >> S390CPU *cpu = S390_CPU(cs); >> CPUS390XState *env = &cpu->env; >> >> if (env->ex_value) { >> /* Execution of the target insn is indivisible from >> the parent EXECUTE insn. */ >> return false; >> } >> if (s390_cpu_has_int(cpu)) { >> s390_cpu_do_interrupt(cs); >> return true; >> } >> if (env->psw.mask & PSW_MASK_WAIT) { >> /* Woken up because of a floating interrupt but it has already >> * been delivered. Go back to sleep. */ >> cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HALT); >> } >> } >> return false; >> } >> >> Note the 'if (env->ex_value) { }' check. It looks like this function >> just returns false in case tcg is executing an EX instruction. After >> that the information that the TB should be exited because of an >> interrupt is gone. So the TB's are never exited again, although the >> interrupt wasn't handled. At least that's my assumption now, if i'm >> wrong please tell me. > > Looking at the code i see CF_NOIRQ to prevent TB's from getting > interrupted. But i only see that used in the core tcg code. Would > that be a possibility, or is there something else/better? Yes CF_NOIRQ is exactly the compiler flag you would use to prevent a block from exiting early when you absolutely want to execute the next block. We currently only use it from core code to deal with icount related things but I can see it's use here. I would probably still wrap it in a common function in cpu-exec-common.c I'm unsure of the exact semantics for s390 so I will defer to Richard and others but something like (untested): /* * Ensure the next N instructions are not interrupted by IRQ checks. */ void cpu_loop_exit_unint(CPUState *cpu, uintptr_t pc, int len) { if (pc) { cpu_restore_state(cpu, pc, true); } cpu->cflags_next_tb = len | CF_LAST_IO | CF_NOIRQ | curr_cflags(cpu); cpu_loop_exit(cpu); } And then in HELPER(ex) you can end the helper with: void HELPER(ex)(CPUS390XState *env, uint32_t ilen, uint64_t r1, uint64_t addr) { ... /* * We must execute the next instruction exclusively so exit the loop * and trigger a NOIRQ TB which won't check for an interrupt until * it finishes executing. */ cpu_loop_exit_unint(cpu, 0, 1); } Some notes: * Take care to ensure the CPU state is synchronised Which means the helper cannot use the flags TCG_CALL_NO_(READ_GLOBALS|WRITE_GLOBALS|SIDE_EFFECTS). And you you will to make sure you write the current PC in the tcg gen code in op_ex() * I think the env->ex_value can be removed after this * We will actually exit the execution loop (via a sigjmp) but the IRQ check in cpu_handle_interrupt() will be skipped due to the custom flags. When the next block is looked up (or generated) it will be entered but then immediately exit * I think even a branch to self should work because the second iteration will be interuptable > Sorry for the dumb questions, i'm not often working on qemu ;-) There are no dumb questions, just opportunities for better documentation ;-)
On 6/29/22 16:16, Sven Schnelle wrote: > Thanks, that was very helpful. I added debugging and it turned out > that the TB is left because of a pending irq. The code then calls > s390_cpu_exec_interrupt: > > bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request) > { > if (interrupt_request & CPU_INTERRUPT_HARD) { > S390CPU *cpu = S390_CPU(cs); > CPUS390XState *env = &cpu->env; > > if (env->ex_value) { > /* Execution of the target insn is indivisible from > the parent EXECUTE insn. */ > return false; > } > if (s390_cpu_has_int(cpu)) { > s390_cpu_do_interrupt(cs); > return true; > } > if (env->psw.mask & PSW_MASK_WAIT) { > /* Woken up because of a floating interrupt but it has already > * been delivered. Go back to sleep. */ > cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HALT); > } > } > return false; > } > > Note the 'if (env->ex_value) { }' check. It looks like this function > just returns false in case tcg is executing an EX instruction. After > that the information that the TB should be exited because of an > interrupt is gone. So the TB's are never exited again, although the > interrupt wasn't handled. At least that's my assumption now, if i'm > wrong please tell me. Ah, yes, I see. We wanted to treat ex_value != 0 as if interrupts are disabled, because we have no way of stacking that value for re-execution after the interrupt (which itself could use EXECUTE). One solution might be to zap ex_value and arrange to re-execute the EXECUTE instruction after the interrupt. Another solution is to generate an exit from any TB translating ex_value, so that interrupts are re-examined. This is probably cleanest. I'll prepare a patch. r~
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c index c997c2e8e0..35819fd5a7 100644 --- a/accel/tcg/cpu-exec.c +++ b/accel/tcg/cpu-exec.c @@ -319,7 +319,8 @@ const void *HELPER(lookup_tb_ptr)(CPUArchState *env) cpu_get_tb_cpu_state(env, &pc, &cs_base, &flags); cflags = curr_cflags(cpu); - if (check_for_breakpoints(cpu, pc, &cflags)) { + if (check_for_breakpoints(cpu, pc, &cflags) || + unlikely(qatomic_read(&cpu->interrupt_request))) { cpu_loop_exit(cpu); }