mbox series

[0/2] target/ppc: Fix PMU instruction counting

Message ID 20240522040411.90655-1-npiggin@gmail.com
Headers show
Series target/ppc: Fix PMU instruction counting | expand

Message

Nicholas Piggin May 22, 2024, 4:04 a.m. UTC 
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(-)

Comments

Richard Henderson May 22, 2024, 10:46 p.m. UTC | #1 
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~
Nicholas Piggin May 23, 2024, 11:27 p.m. UTC | #2 
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