Re: [PATCH 2/2] iommu/amd: Remove performance counter pre-initialization test

[Alexander Monakov <amonakov@ispras.ru>]

[-- Attachment #1: Type: text/plain, Size: 2742 bytes --]

On Tue, 20 Apr 2021, Suthikulpanit, Suravee wrote:

> David / Joerg,
> 
> On 4/10/2021 5:03 PM, David Coe wrote:
> > 
> > The immediately obvious difference is the with the enormous count seen on
> > mem_dte_mis on the older Ryzen 2400G. Will do some RTFM but anyone
> > with comments and insight?
> > 
> > 841,689,151,202,939       amd_iommu_0/mem_dte_mis/              (33.44%)
> > 
> > Otherwise, all seems to running smoothly (especially for a distribution
> > still in β). Bravo and many thanks all!
> 
> The initial hypothesis is that the issue happens only when users specify more
> number of events than
> the available counters, which Perf will time-multiplex the events onto the
> counters.
> 
> Looking at the Perf and AMD IOMMU PMU multiplexing logic, it requires:
>  1. Stop the counter (i.e. set CSOURCE to zero to stop counting)
>  2. Save the counter value of the current event
>  3. Reload the counter value of the new event (previously saved)
>  4. Start the counter (i.e. set CSOURCE to count new events)
> 
> The problem here is that when the driver writes zero to CSOURCE register in
> step 1, this would enable power-gating,
> which prevents access to the counter and result in writing/reading value in
> step 2 and 3.
> 
> I have found a system that reproduced this case (w/ unusually large number of
> count), and debug the issue further.
> As a hack, I have tried skipping step 1, and it seems to eliminate this issue.
> However, this is logically incorrect,
> and might result in inaccurate data depending on the events.
> 
> Here are the options:
> 1. Continue to look for workaround for this issue.
> 2. Find a way to disable event time-multiplexing (e.g. only limit the number
> of counters to 8)
>    if power gating is enabled on the platform.
> 3. Back to the original logic where we had the pre-init check of the counter
> vlues, which is still the safest choice
>    at the moment unless

If the "power-gated" counter only ignores writes, but yields correct values on
reads, you don't need to change its value.

0. When initializing the counter, save its value as 'raw_counter_value'.

When switching:

1. Set CSOURCE to zero (pauses the counter).
2. Read current counter value ('new_value').
3. Add '(new_value - raw_counter_value) & mask' to the current event count;
   where 'mask' is '(1ULL << 48) - 1' to account for overflow correctly.
4. Assign 'raw_counter_value = new_value'.
5. Set CSOURCE to new configuration value.

Modifying the hardware counter value is only needed to get overflow interrupts,
but there's no code to handle them on Linux (since the interrupts are
asynchronous, and given the nature of the events, I don't see how it would be
useful).

Alexander