Re: [PATCH 2/3] nvme: Add hysteresis and idle detection to link rate switching

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] nvme: data race between timer and worker causes stuck link rate
--

commit 281ed4a8d51a45615fb54b154554270a5683089c
Author: Liao Xuan <liaoxuan@open-hieco.net>

nvme: Add hysteresis and idle detection to link rate switching
    
This patch adds counters to avoid excessive rate switching when the
workload fluctuates, requiring consecutive windows above or below a
threshold to trigger an upgrade or downgrade. It also stops monitoring
after 10 seconds of inactivity.

> diff --git a/drivers/nvme/host/speed_switch.c b/drivers/nvme/host/speed_switch.c
> index 88c046fcdde89..22f180c8119b2 100644
> --- a/drivers/nvme/host/speed_switch.c
> +++ b/drivers/nvme/host/speed_switch.c
[ ... ]
> @@ -247,31 +252,62 @@ static int nvme_check_io_and_decide_speed(struct nvme_speed_switch *sw)
>  		stat->write_bytes = 0;
>  	}
>  
> +	if (read_bytes || write_bytes) {
> +		*io_activity = true;
> +		sw->idle_cnt = 0;
> +	} else {
> +		sw->idle_cnt++;
> +	}
> +
> +	/* Stop monitoring after 10 seconds of inactivity (100 windows of 100ms). */
> +	if (sw->idle_cnt >= 100)
> +		*io_activity = false;
> +
>  	read_kb = read_bytes / 1024;
>  	write_kb = write_bytes / 1024;
>  	dev_dbg(ctrl->device,
>  		"I/O in window: read=%lu KB, write=%lu KB, threshold=%u KB\n",
>  		read_kb, write_kb, READ_ONCE(sw->threshold));
>  
> -	if (read_kb >= sw->threshold || write_kb >= sw->threshold)
> +	if (read_kb >= sw->threshold || write_kb >= sw->threshold) {
> +		sw->up_cnt++;
> +		sw->down_cnt = 0;
> +	} else {
> +		sw->down_cnt++;
> +		sw->up_cnt = 0;
> +	}
> +
> +	if (sw->up_cnt > READ_ONCE(sw->up_threshold)) {
> +		sw->up_cnt = 0;
>  		return sw->max_speed;
> +	}
> +
> +	if (sw->down_cnt > READ_ONCE(sw->down_threshold) || !*io_activity) {
> +		sw->down_cnt = 0;
> +		return sw->min_speed;
> +	}
>  
> -	return sw->min_speed;
> +	return READ_ONCE(sw->cur_speed);

[Severity: High]
Does returning cur_speed here cause a data race between the timer and the
worker that leaves the hardware link rate permanently throttled?

If the timer previously returned min_speed and scheduled
nvme_speed_switch_work(), the worker will freeze the queues and begin a slow
hardware transition via pcie_set_target_speed().

While the worker is blocked on the hardware transition, I/O is 0 because the
queues are frozen. The concurrent timer will increment down_cnt, but since it
is below down_threshold, it will fall through and return
READ_ONCE(sw->cur_speed).

Because the worker hasn't updated sw->cur_speed yet, it's still max_speed.
The timer then updates sw->target_speed to max_speed:

drivers/nvme/host/speed_switch.c:nvme_speed_switch_timer_fn() {
    ...
	sw->target_speed = nvme_check_io_and_decide_speed(sw, &io_activity);
    ...
}

When the worker finishes setting the hardware to min_speed, it double-reads
sw->target_speed:

drivers/nvme/host/speed_switch.c:nvme_speed_switch_work() {
    ...
	ret = pcie_set_target_speed(bridge, nvme_speed_to_bus_speed(sw->target_speed),
				    true);
	if (ret) {
        ...
	} else {
		WRITE_ONCE(sw->cur_speed, sw->target_speed);
	}
    ...
}

This overwrites cur_speed with the timer-corrupted value (max_speed).

The hardware is now throttled at min_speed, but software thinks it is at
max_speed.

When high I/O resumes, the timer will decide the target is max_speed, but
since cur_speed is already max_speed, it won't schedule the worker to restore
the link rate, causing a severe performance regression.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790222172.git.liaoxuan@hygon.cn?part=2