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

[Nilay Shroff]

On 10/24/26 4:34 AM, Saravanan D wrote:
> nvme_tcp_set_queue_io_cpu() picks each queue's io_cpu at connect time
> as the least loaded CPU in the queue's blk-mq map group, and all socket
> work runs there for the connection's lifetime. This decision falls
> short when the host partitions its CPUs after connect time. On a 384
> cpu multi tenant host with 128 queue controllers, blk-mq folds three
> CPUs into every map group, some groups straddle two tenants' cpusets,
> and 9% of nvme_tcp_io_work executions ran outside the submitting VM's
> cpuset, seen by the neighbor as steal time.
>
> Expose the io_cpu in each I/O queue's directory
>
>    /sys/class/nvme/nvmeX/queues/<qid>/io_cpu
>
> so a control plane that owns CPU placement can set it directly instead
> of relying on the driver's heuristic. The attribute accepts a decimal
> cpu number, -1 or "unbound". A user assignment, unbound included, is
> marked in the queue flags, persists across reconnects and is never
> reselected by the driver. The read-only managed attribute reports 1
> while io_cpu is driver managed and 0 once the user assigned it. The
> accounting of nvme_tcp_cpu_queues applies regardless of who set the
> io_cpu.
>
> A queue's directory is added once the connect time selection has set its
> io_cpu, so readers never see a placement that is about to change, and
> removed when the queue is freed, before it is torn down, so a sysfs
> store never runs on a freed queue. The queues directory is created
> before the I/O queues are set up in parallel. All io_cpu writes, from
> sysfs and from the driver, serialize with the accounting under
> nvme_tcp_cpu_queues_lock, while the I/O paths read io_cpu locklessly.
> The queue stop now clears NVME_TCP_Q_IO_CPU_SET under that lock, since a
> sysfs store may move io_cpu in between.
>
> nvme_tcp_free_queue() now clears NVME_TCP_Q_IO_CPU_SET and drops its
> count, because a queue can still be counted when it is freed. A sysfs
> store may have run after the queue stop, or the connect may have
> failed, and the setup error path frees such a queue without calling
> nvme_tcp_stop_queue(), where the count is normally dropped.
>
> Suggested-by: Sagi Grimberg <sagi@xxxxxxxxxxx>
> Link: https://lore.kernel.org/linux-nvme/13c160d0-0657-47ce-9a16-58d348267ab0@xxxxxxxxxxx/
> Assisted-by: Claude:claude-opus-5-5 [Claude Code]
> Signed-off-by: Saravanan D <saravanand@xxxxxxxxx>
> ---
>   Documentation/ABI/stable/sysfs-nvme |  31 +++++
>   drivers/nvme/host/tcp.c             | 172 ++++++++++++++++++++++++----
>   2 files changed, 183 insertions(+), 20 deletions(-)

[...]

> @@ -2303,6 +2429,11 @@ static int nvme_tcp_configure_io_queues(struct nvme_ctrl *ctrl, bool new)
>   	if (ret)
>   		return ret;
>   
> +	/* the parallel queue setup below adds the queue directories */
> +	ret = nvme_queue_dir_create(ctrl);
> +	if (ret)
> +		return ret;
> +

I see here nvme_queue_dir_create() is invoked on every reconnect. Although
it is guarded by ctrl->queues_kobj, the directory is controller-lifetime
state, so it would be cleaner to create it once from nvme_add_ctrl().

Moreover, this also makes the ownership asymmetric: nvme-tcp creates the
queues directory, but nvme-core removes it from nvme_uninit_ctrl(). Could
nvme-core manage both creation and destruction instead?

One way to do that is to add queue_attr_groups to struct nvme_ctrl_ops.
A transport that provides per-queue attributes can register its groups
there; for example, NVMe/TCP would set:

        .queue_attr_groups = nvme_tcp_queue_groups,

Then, after cdev_device_add() succeeds, nvme_add_ctrl() can create the
directory when queue_attr_groups is set:

        if (ctrl->ops->queue_attr_groups) {
                ret = nvme_queue_dir_create(ctrl);
                if (ret)
                        goto out_del_cdev;
        }

where out_del_cdev unwinds the successful cdev_device_add().

This lets nvme-core own both creation and removal of the queues
directory, and nvme_queue_dir_create() no longer needs to be exported.
It also means nvme_add_queue_info() need not take attribute groups as
an argument as it can use ctrl->ops->queue_attr_groups directly.

Thanks,
--Nilay