Re: [PATCH v5 2/2] nvme-tcp: allow setting per-queue io_cpu through sysfs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] nvme-tcp: sysfs io_cpu exposed before nvme_tcp_set_queue_io_cpu() runs
--

commit e947efa489c70b4e98e2f6a3345b13b02a0aef83
Author: Saravanan D <saravanand@crusoe.ai>

nvme-tcp: allow setting per-queue io_cpu through sysfs

This commit exposes per-queue io_cpu and managed sysfs attributes under
/sys/class/nvme/nvmeX/queues/<qid>/ so userspace can view and override
the CPU placement for NVMe-TCP I/O queue socket work.

Link: https://lore.kernel.org/linux-nvme/13c160d0-0657-47ce-9a16-58d348267ab0@grimberg.me/

> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index ec678ed07af8d..699997c0e8e24 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
[ ... ]
> @@ -1917,7 +2038,11 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
>  
>  	queue->sock->sk->sk_allocation = GFP_ATOMIC;
>  	queue->sock->sk->sk_use_task_frag = false;
> -	queue->io_cpu = WORK_CPU_UNBOUND;
> +	/* no store races here, the lock is for the write helper */
> +	spin_lock(&nvme_tcp_cpu_queues_lock);
> +	if (!test_bit(NVME_TCP_Q_IO_CPU_USER, &queue->flags))
> +		nvme_tcp_queue_change_io_cpu(queue, WORK_CPU_UNBOUND);
> +	spin_unlock(&nvme_tcp_cpu_queues_lock);
>  	queue->request = NULL;
>  	queue->data_remaining = 0;
>  	queue->ddgst_remaining = 0;
> @@ -1985,6 +2110,9 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
>  
>  	set_bit(NVME_TCP_Q_ALLOCATED, &queue->flags);
>  
> +	if (qid)
> +		nvme_tcp_add_queue_sysfs(queue);
> +
>  	return 0;

[Severity: Medium]
In nvme_tcp_setup_queue_async(), nvme_tcp_alloc_queue() runs before
nvme_tcp_start_queue(), where nvme_tcp_set_queue_io_cpu() is called:

nvme_tcp_setup_queue_async()
  nvme_tcp_alloc_queue()
    if (!test_bit(NVME_TCP_Q_IO_CPU_USER, &queue->flags))
      nvme_tcp_queue_change_io_cpu(queue, WORK_CPU_UNBOUND);
    ...
    nvme_tcp_add_queue_sysfs(queue);
  nvme_tcp_start_queue()
    if (idx) {
      nvme_tcp_set_queue_io_cpu(queue);
      ret = nvmf_connect_io_queue(nctrl, idx);
    }

Because nvme_tcp_add_queue_sysfs() publishes the sysfs attributes in
nvme_tcp_alloc_queue() before nvme_tcp_set_queue_io_cpu() runs in
nvme_tcp_start_queue(), can a userspace control plane reading io_cpu and
managed as soon as the queue directory appears observe io_cpu as -1
(unbound) with managed=1 even when wq_unbound is false?

Also, during parallel reconnects in nvme_tcp_setup_io_queues(), a queue
with NVME_TCP_Q_IO_CPU_USER set does not re-increment
nvme_tcp_cpu_queues[io_cpu] until it reaches nvme_tcp_set_queue_io_cpu()
after completing the blocking TCP, TLS, and ICReq handshakes in
nvme_tcp_alloc_queue(). If a driver-managed queue finishes its handshake
first, could nvme_tcp_set_queue_io_cpu() pick a CPU without accounting for
the user-assigned queue's load?

Would it make sense to either call nvme_tcp_set_queue_io_cpu() before
nvme_tcp_add_queue_sysfs() in nvme_tcp_alloc_queue(), or defer
nvme_tcp_add_queue_sysfs() until after nvme_tcp_set_queue_io_cpu() has
run?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018010059.83120-1-saravanand@crusoe.ai?part=2