Re: [PATCH v6 08/13] alpha/perf: optionally randomize the ProfileMe sampling interval

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] alpha/perf: negative period_left in alpha_perf_event_set_period() jitter
--

commit 9ea17dccaea7d498bc01f69fffc0c162389f6e92
Author: Matt Turner <mattst88@gmail.com>

alpha/perf: optionally randomize the ProfileMe sampling interval

This commit adds a randomize config bit for raw ProfileMe perf events on
Alpha that varies each sampling interval by up to 1/16 of the period in
either direction using a per-CPU prandom state.

> diff --git a/arch/alpha/kernel/perf_event.c b/arch/alpha/kernel/perf_event.c
> index f6546d6481333..b33677cc23caa 100644
> --- a/arch/alpha/kernel/perf_event.c
> +++ b/arch/alpha/kernel/perf_event.c

[ ... ]

> @@ -389,6 +399,22 @@ static int alpha_perf_event_set_period(struct perf_event *event,
>  		ret = 1;
>  	}
>  
> +	/*
> +	 * Vary each new interval by up to 1/16 of the period either way, so
> +	 * the samples cannot phase-lock with a loop whose length divides the
> +	 * period. The count stays exact because prev_count records the value
> +	 * written.
> +	 */
> +	if (ret && (hwc->flags & ALPHA_PERF_FLAG_RANDOMIZE) && period >= 8) {
> +		long jitter = period / 8;
> +		u32 r = prandom_u32_state(this_cpu_ptr(&alpha_perf_rnd));
> +		long offset = mul_u64_u32_shr(jitter, r, 32) - jitter / 2;
> +
> +		if (check_add_overflow(left, offset, &left))
> +			left = LONG_MAX;
> +		local64_set(&hwc->period_left, left);
> +	}

[Severity: Medium]
Can a negative offset here drive left and hwc->period_left to zero or a
negative value?

In alpha_perf_event_set_period(), when left enters in the range
[-period + 1, -period + (period / 8) / 2], the first check (left <= -period)
is false and the second check (left <= 0) adds period to left, leaving left
in [1, (period / 8) / 2] with ret = 1:

arch/alpha/kernel/perf_event.c:alpha_perf_event_set_period() {
	if (unlikely(left <= -period)) {
		left = period;
		local64_set(&hwc->period_left, left);
		hwc->last_period = period;
		ret = 1;
	}

	if (unlikely(left <= 0)) {
		left += period;
		local64_set(&hwc->period_left, left);
		hwc->last_period = period;
		ret = 1;
	}
	...
	if (unlikely(left < alpha_pmu->pmc_left[idx]))
		left = alpha_pmu->pmc_left[idx];
}

Since offset can be as negative as -(jitter / 2), adding offset to left can
produce left <= 0, which is then stored into hwc->period_left.

The subsequent hardware minimum check only clamps the local variable left to
alpha_pmu->pmc_left[idx] (16) before programming the counter, leaving
hwc->period_left <= 0. If the event is later stopped and reloaded before the
next overflow (for example on a scheduler tick or context switch via
alpha_pmu_stop() and alpha_pmu_start()), alpha_perf_event_set_period() will
see left <= 0 and add another full period without emitting a sample, or the
next overflow will subtract delta from an already non-positive
hwc->period_left.

Should left (and hwc->period_left) be clamped so that the jittered interval
remains positive?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-alpha-profileme-v6-0-63362e9290a3@gmail.com?part=8