Re: [PATCH v3] perf/core: Set event shadow time for inactive events too

[Peter Zijlstra]

On Sun, Dec 05, 2021 at 02:48:43PM -0800, Namhyung Kim wrote:
> While commit f79256532682 ("perf/core: fix userpage->time_enabled of
> inactive events") fixed this problem for user rdpmc usage,

You're referring to 'this problem' before actually describing a problem :-(

Also, you now have me looking at that commit again, and I'm still hating
it. Also, I'm again struggling to make sense of it; all except the very
last hunk that is.

So the whole, full-fat, mmap self-monitor thing looks like:


	u32 seq, time_mult, time_shift, index, width = 64;
	u64 count, enabled, running;
	u64 cyc, time_offset, time_cycles = 0, time_mask = ~0ULL;
	u64 quot, rem, delta;
	s64 pmc = 0;

	do {
		seq = pc->lock;
		barrier();

		enabled = pc->time_enabled;
		running = pc->time_running;

		if (pc->cap_user_time && enabled != running) {
			cyc = rdtsc();
			time_offset = pc->time_offset;
			time_mult   = pc->time_mult;
			time_shift  = pc->time_shift;
		}

		if (pc->cap_user_time_short) {
			time_cycles = pc->time_cycles;
			time_mask   = pc->time_mask;
		}

		index = pc->index;
		count = pc->offset;
		if (pc->cap_user_rdpmc && index) {
			width = pc->pmc_width;
			pmc = rdpmc(index - 1);
		}

		barrier();
	} while (pc->lock != seq);

	if (width < 64) {
		pmc <<= 64 - width;
		pmc >>= 64 - width;
	}
	count += pmc;

	cyc = time_cycles + ((cyc - time_cycles) & time_mask);

	quot = (cyc >> time_shift);
	rem = cyc & ((1ULL < time_shift) - 1);
	delta = time_offset + quot * time_mult +
		((rem * time_mult) >> time_shift);

	enabled += delta;
	if (index)
		running += delta;

	quot = count / running;
	rem  = count % running;
	count = quot * enabled + (rem * enabled) / running;


Now, the thing that sticks out to me is that 'enabled' is
unconditionally advanced. It *always* runs.

So how can not updating ->time_enabled when the counter is INACTIVE due
to rotation (which causes ->index == 0), cause enabled to not be
up-to-date?

Can we please figure that out so I can go revert all but the last hunk
of that patch?