Re: [PATCH 0/2] target/ppc: Fix PMU instruction counting
On Thu May 23, 2024 at 8:46 AM AEST, Richard Henderson wrote: > On 5/21/24 21:04, Nicholas Piggin wrote: > > The crux of the problem being that dynamic exits from a TB would > > not count instructions previously executed in the TB. I don't > > know how important it is for PMU to count instructions exactly, > > however for instruction replay this can lead to different counts > > for the same execution (e.g., because TBs can be different sized) > > and that blows up reverse debugging. > > > > I posted something on this out before, but missed a few things > > (most notably faulting memory access). And found that forcing 1 > > insn per TB seems to be the only feasible way to do this. > > > > Sorry to ping you on this again Richard, it's not urgent but > > you're the guru with this stuff and I'm hesitant to change it > > without a better opinion ... Simple band aid for the meanwhile > > could be leave it as is but just disable counting if > > record/replay is in use. > > When we unwind, we know how many insns remain in the tb. > With icount, we adjust cpu->neg.icount_decr.u16.low. > > My suggestion is to change restore_state_to_opc to pass in either the raw > insns_left, or > the inverse: tb->icount - insns_left. > > That'll be a trivial mechanical change for the signature of the hook, first. That gives me a better place to start looking. Thanks, Nick
Re: [PATCH 0/2] target/ppc: Fix PMU instruction counting
On 5/21/24 21:04, Nicholas Piggin wrote: The crux of the problem being that dynamic exits from a TB would not count instructions previously executed in the TB. I don't know how important it is for PMU to count instructions exactly, however for instruction replay this can lead to different counts for the same execution (e.g., because TBs can be different sized) and that blows up reverse debugging. I posted something on this out before, but missed a few things (most notably faulting memory access). And found that forcing 1 insn per TB seems to be the only feasible way to do this. Sorry to ping you on this again Richard, it's not urgent but you're the guru with this stuff and I'm hesitant to change it without a better opinion ... Simple band aid for the meanwhile could be leave it as is but just disable counting if record/replay is in use. When we unwind, we know how many insns remain in the tb. With icount, we adjust cpu->neg.icount_decr.u16.low. My suggestion is to change restore_state_to_opc to pass in either the raw insns_left, or the inverse: tb->icount - insns_left. That'll be a trivial mechanical change for the signature of the hook, first. r~
[PATCH 0/2] target/ppc: Fix PMU instruction counting
The crux of the problem being that dynamic exits from a TB would not count instructions previously executed in the TB. I don't know how important it is for PMU to count instructions exactly, however for instruction replay this can lead to different counts for the same execution (e.g., because TBs can be different sized) and that blows up reverse debugging. I posted something on this out before, but missed a few things (most notably faulting memory access). And found that forcing 1 insn per TB seems to be the only feasible way to do this. Sorry to ping you on this again Richard, it's not urgent but you're the guru with this stuff and I'm hesitant to change it without a better opinion ... Simple band aid for the meanwhile could be leave it as is but just disable counting if record/replay is in use. Thanks, Nick Nicholas Piggin (2): target/ppc: Fix PMC5 instruction counting target/ppc: Tidy pmu_count_insns implementation target/ppc/translate.c | 163 + 1 file changed, 83 insertions(+), 80 deletions(-) -- 2.43.0
