All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dave Jiang <dave.jiang@intel.com>
To: linux-cxl@vger.kernel.org, linux-perf-users@vger.kernel.org
Cc: jic23@kernel.org, will@kernel.org, mark.rutland@arm.com,
	dave@stgolabs.net, robin.murphy@arm.com, sashiko-bot@kernel.org
Subject: [PATCH v3 3/9] perf/cxl: Fix the counter overflow delta fixup
Date: Fri, 31 Jul 2026 16:28:21 -0700	[thread overview]
Message-ID: <20260731232827.401447-4-dave.jiang@intel.com> (raw)
In-Reply-To: <20260731232827.401447-1-dave.jiang@intel.com>

The counter is masked to counter_width, so the subtraction in
__cxl_pmu_read() throws away the bit that records the wrap. A delta of one
whole period reads back as 0, the same as no events at all, and only the
overflow status tells the two apart. That is why __cxl_pmu_read() takes an
overflow argument.

The fixup keys off the delta rather than the operands:

	delta = (new_cnt - prev_cnt) & GENMASK_ULL(counter_width - 1, 0);
	if (overflow && delta < GENMASK_ULL(counter_width - 1, 0))
		delta += (1UL << counter_width);

so it cannot tell whether the subtraction needed the period. What decides
that is where prev_count sits when the counter wraps:

  event_start         polled read          wrap
  ctr = 0 ..........  ctr = P/2 .........  mask -> 0 (+r)
  prev = 0            prev = P/2                 IRQ reads new = r

  prev = 0    (no read yet):  new >= prev, fell short -> add period
  prev = P/2  (polled):       new <  prev, spans wrap -> add nothing

Both rows are the same overflow interrupt and the old guard adds a period
in both, so a mid-period read makes the event over-count. 'perf stat -I'
hits that.

Condition the fixup on new_cnt >= prev_cnt instead, the one case the
subtraction cannot express. Dropping it outright would break the first row.
The residual r need not be 0 either, since some events increment by more
than 1 per cycle (CXL r4.0 8.2.7.2.1, Threshold), and keying off the
operands stays exact whatever r is.

Use mask + 1 for the period rather than a shift. It is 0 for a 64-bit
counter, and avoids the old 1UL << counter_width, undefined for width 64
and for >= 32 on 32-bit kernels.

Fixes: 5d7107c72796 ("perf: CXL Performance Monitoring Unit driver")
Reported-by: sashiko-bot@kernel.org
Closes: https://sashiko.dev/#/patchset/20260715191454.459673-1-dave@stgolabs.net?part=1
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
v3:
- Rewrite the rationale. v2 blamed the counter counting on past the wrap,
  which the spec permits but which was not what made the old code wrong
  (Jonathan).
- Note that events incrementing by more than 1 per cycle make a non-zero
  residual reachable, so the fixup has to hold for any residual (Jonathan).
- Drop the claim that the old code loses a period on the first overflow
  after event_start(). It does not. The subtraction yields 0 there and the
  old guard then adds the period back correctly. The reachable bug is the
  over-count in the other direction.
- No code change.
---
 drivers/perf/cxl_pmu.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/perf/cxl_pmu.c b/drivers/perf/cxl_pmu.c
index b16e2e4090a3..3511d049aea4 100644
--- a/drivers/perf/cxl_pmu.c
+++ b/drivers/perf/cxl_pmu.c
@@ -689,7 +689,7 @@ static void __cxl_pmu_read(struct perf_event *event, bool overflow)
 {
 	struct cxl_pmu_info *info = pmu_to_cxl_pmu_info(event->pmu);
 	struct hw_perf_event *hwc = &event->hw;
-	u64 new_cnt, prev_cnt, delta;
+	u64 new_cnt, prev_cnt, delta, mask;
 
 	do {
 		prev_cnt = local64_read(&hwc->prev_count);
@@ -697,12 +697,17 @@ static void __cxl_pmu_read(struct perf_event *event, bool overflow)
 	} while (local64_cmpxchg(&hwc->prev_count, prev_cnt, new_cnt) != prev_cnt);
 
 	/*
-	 * If we know an overflow occur then take that into account.
-	 * Note counter is not reset as that would lose events
+	 * The mask discards exactly the bit that says the counter wrapped, so a
+	 * delta of one whole period reads back as 0, the same as no events at
+	 * all. Only the overflow status distinguishes them, and new_cnt >=
+	 * prev_cnt is that case, so add the period back. mask + 1 is
+	 * 2^counter_width, which is 0 for a 64-bit counter - the right value to
+	 * add once the top bit is gone, and no undefined 1 << 64.
 	 */
-	delta = (new_cnt - prev_cnt) & GENMASK_ULL(info->counter_width - 1, 0);
-	if (overflow && delta < GENMASK_ULL(info->counter_width - 1, 0))
-		delta += (1UL << info->counter_width);
+	mask = GENMASK_ULL(info->counter_width - 1, 0);
+	delta = (new_cnt - prev_cnt) & mask;
+	if (overflow && new_cnt >= prev_cnt)
+		delta += mask + 1;
 
 	local64_add(delta, &event->count);
 }
-- 
2.55.0


  parent reply	other threads:[~2026-07-31 23:28 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-31 23:28 [PATCH v3 0/9] perf/cxlpmu: Misc sashiko raised issues fixes Dave Jiang
2026-07-31 23:28 ` [PATCH v3 1/9] perf/cxl: Program the requested event group on configurable counters Dave Jiang
2026-07-31 23:38   ` sashiko-bot
2026-07-31 23:28 ` [PATCH v3 2/9] perf/cxl: Clear stale event fields before reprogramming a counter Dave Jiang
2026-07-31 23:40   ` sashiko-bot
2026-07-31 23:28 ` Dave Jiang [this message]
2026-07-31 23:37   ` [PATCH v3 3/9] perf/cxl: Fix the counter overflow delta fixup sashiko-bot
2026-07-31 23:28 ` [PATCH v3 4/9] perf/cxl: Split the MSI vector out of info->irq Dave Jiang
2026-07-31 23:45   ` sashiko-bot
2026-07-31 23:28 ` [PATCH v3 5/9] perf/cxl: Accept an overflow interrupt on MSI message number 0 Dave Jiang
2026-07-31 23:28 ` [PATCH v3 6/9] perf/cxl: Don't share the overflow interrupt, and keep it pinned Dave Jiang
2026-07-31 23:50   ` sashiko-bot
2026-07-31 23:28 ` [PATCH v3 7/9] perf/cxl: Unfreeze counters after handling an overflow interrupt Dave Jiang
2026-07-31 23:40   ` sashiko-bot
2026-07-31 23:28 ` [PATCH v3 8/9] perf/cxl: Validate the hardware-reported counter width Dave Jiang
2026-07-31 23:28 ` [PATCH v3 9/9] perf/cxl: Don't log through pmu.dev in the overflow interrupt handler Dave Jiang
2026-07-31 23:46   ` sashiko-bot

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=20260731232827.401447-4-dave.jiang@intel.com \
    --to=dave.jiang@intel.com \
    --cc=dave@stgolabs.net \
    --cc=jic23@kernel.org \
    --cc=linux-cxl@vger.kernel.org \
    --cc=linux-perf-users@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=robin.murphy@arm.com \
    --cc=sashiko-bot@kernel.org \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.