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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 9 Dec 2021 00:22:03 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     Ingo Molnar <mingo@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jiri Olsa <jolsa@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Stephane Eranian <eranian@...gle.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Ian Rogers <irogers@...gle.com>,
        Song Liu <songliubraving@...com>
Subject: Re: [PATCH v3] perf/core: Set event shadow time for inactive events
 too

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?