LKML Archive on
help / color / mirror / Atom feed
From: Suzuki K Poulose <>
To: Mark Rutland <>
Subject: Re: [PATCH v2 5/5] arm64: perf: Add support for chaining event counters
Date: Fri, 8 Jun 2018 17:05:09 +0100	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On 08/06/18 16:24, Mark Rutland wrote:
> On Fri, Jun 08, 2018 at 03:46:57PM +0100, Suzuki K Poulose wrote:
>> On 06/06/2018 07:01 PM, Mark Rutland wrote:
>>>> Thus we need special allocation schemes
>>>> to make the full use of available counters. So, we allocate the
>>>> counters from either ends. i.e, chained counters are allocated
>>>> from the lower end in pairs of two and the normal counters are
>>>> allocated from the higher number. Also makes necessary changes to
>>>> handle the chained events as a single event with 2 counters.
>>> Why do we need to allocate in this way?
>>> I can see this might make allocating a pair of counters more likely in
>>> some cases, but there are still others where it wouldn't be possible
>>> (and we'd rely on the rotation logic to repack the counters for us).
>> It makes the efficient use of the counters in all cases and allows
>> counting maximum number of events with any given set, keeping the precedence
>> on the order of their "inserts".
>> e.g, if the number of counters happened to be "odd" (not sure if it is even
>> possible).
> Unfortunately, that doesn't always work.
> Say you have a system with 8 counters, and you open 8 (32-bit) events.

I was talking about the following (imaginary) case :

We have 7 counters, and you have 5 32bit counters and 1 64bit counter.
Without the above scheme, you would place first 5 events on the first
5 counters and then you can't place the 64bit counter, as you are now
left with a low/odd counter and a high/even counter.

> Then you close the events in counters 0, 2, 4, and 6. The live events
> aren't moved, and stay where they are, in counters 1, 3, 5, and 7.
> Now, say you open a 64-bit event. When we try to add it, we try to
> allocate an index for two consecutive counters, and find that we can't,
> despite 4 counters being free.
> We return -EAGAIN to the core perf code, whereupon it removes any other
> events in that group (none in this case).
> Then we wait for the rotation logic to pull all of the events out and
> schedule them back in, re-packing them, which should (eventually) work
> regardless of how we allocate counters.
> ... we might need to re-pack events to solve that. :/

I agree, removing and putting them back in is not going to work unless
we re-pack or delay allocating the counters until we start the PMU.

> [...]
>>>> +static inline void armv8pmu_write_chain_counter(int idx, u64 value)
>>>> +{
>>>> +	armv8pmu_write_evcntr(idx, value >> 32);
>>>> +	isb();
>>>> +	armv8pmu_write_evcntr(idx - 1, value);
>>>> +}
>>> Can we use upper_32_bits() and lower_32_bits() here?
>>> As a more general thing, I think we need to clean up the way we
>>> read/write counters, because I don't think that it's right that we poke
>>> them while they're running -- that means you get some arbitrary skew on
>>> counter groups.
>>> It looks like the only case we do that is the IRQ handler, so we should
>>> be able to stop/start the PMU there.
>> Since we don't stop the "counting" of events usually when an IRQ is
>> triggered, the skew will be finally balanced when the events are stopped
>> in a the group. So, I think, stopping the PMU may not be really a good
>> thing after all. Just my thought.
> That's not quite right -- if one event in a group overflows, we'll
> reprogram it *while* other events are counting, losing some events in
> the process.

Oh yes, you're right. I can fix it.

> Stopping the PMU for the duration of the IRQ handler ensures that events
> in a group are always in-sync with one another.


  reply	other threads:[~2018-06-08 16:05 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-29 10:55 [PATCH v2 0/5] arm64: perf: Support for chained counters Suzuki K Poulose
2018-05-29 10:55 ` [PATCH v2 1/5] arm_pmu: Clean up maximum period handling Suzuki K Poulose
2018-05-29 10:55 ` [PATCH v2 2/5] arm_pmu: Change API to support 64bit counter values Suzuki K Poulose
2018-05-29 10:55 ` [PATCH v2 3/5] arm_pmu: Add support for 64bit event counters Suzuki K Poulose
2018-06-06 16:48   ` Mark Rutland
2018-06-07  7:34     ` Suzuki K Poulose
2018-05-29 10:55 ` [PATCH v2 4/5] arm_pmu: Tidy up clear_event_idx call backs Suzuki K Poulose
2018-05-29 10:55 ` [PATCH v2 5/5] arm64: perf: Add support for chaining event counters Suzuki K Poulose
2018-06-06 18:01   ` Mark Rutland
2018-06-08 14:46     ` Suzuki K Poulose
2018-06-08 15:24       ` Mark Rutland
2018-06-08 16:05         ` Suzuki K Poulose [this message]
2018-06-11 13:54       ` Suzuki K Poulose
2018-06-11 14:24         ` Mark Rutland
2018-06-11 16:18           ` Suzuki K Poulose
2018-06-05 15:00 ` [PATCH v2 0/5] arm64: perf: Support for chained counters Julien Thierry

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \
    --subject='Re: [PATCH v2 5/5] arm64: perf: Add support for chaining event counters' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).