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

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

[Saravanan D]

On hosts that partition their CPUs after connect time, the connect
time io_cpu selection can land a queue's socket work on CPUs owned by
a different workload. This series lets a control plane set each
queue's io_cpu directly through sysfs.

The first patch adds a small common queue info that a transport embeds
in its queue structure, backing a transport neutral

  /sys/class/nvme/nvmeX/queues/<qid>/

directory under the controller device. The second patch has nvme-tcp
expose io_cpu and managed attributes there.

Changes since v3 [1]:
- Split into two patches and introduced the common queue info that
  Sagi proposed [2], named nvme_queue_info since the PCI driver
  already uses nvme_queue, as Nilay pointed out.
- Renamed the tcp_queues directory to queues so the ABI does not bake
  in the transport, as suggested by Nilay [3].
- Added the read-only managed attribute, 1 while io_cpu is driver
  managed and 0 once the user assigned it, as suggested by Nilay.
- io_cpu now accepts -1 or the string "unbound" to leave the socket
  work unbound, instead of restoring the connect time selection, as
  suggested by Sagi.
- A user assignment is tracked in the common queue info flags and
  still persists across reconnects, following Sagi's earlier guidance
  that io_cpu should not change on reconnect.
- The queue directories register once per controller lifetime and
  persist across reconnects, and the io_cpu accounting moved from the
  queue lock to a controller level lock since a write can now arrive
  while a queue is torn down.
- The io_cpu accounting now also covers a pin to a queue whose connect
  time pick did not bind a cpu, and the queue teardown clears any
  accounting left by a write racing the teardown.

[1] https://lore.kernel.org/linux-nvme/20260909211432.6741-1-saravanand@crusoe.ai/
[2] https://lore.kernel.org/linux-nvme/13c160d0-0657-47ce-9a16-58d348267ab0@grimberg.me/
[3] https://lore.kernel.org/linux-nvme/8dfee10f-89f3-463b-8d16-587873c3394c@linux.ibm.com/

Saravanan D (2):
  nvme: add per-queue sysfs directories
  nvme-tcp: allow setting per-queue io_cpu through sysfs

 Documentation/ABI/stable/sysfs-nvme |  21 ++++
 drivers/nvme/host/core.c            |   2 +
 drivers/nvme/host/nvme.h            |  23 +++++
 drivers/nvme/host/sysfs.c           |  42 ++++++++
 drivers/nvme/host/tcp.c             | 153 +++++++++++++++++++++++++++-
 5 files changed, 240 insertions(+), 1 deletion(-)


base-commit: 2ee54f01f07c0307deaf90ca8691a4643ae0357b
-- 
2.55.0

[PATCH v4 1/2] nvme: add per-queue sysfs directories

[Saravanan D]

Transports have per I/O queue state worth exposing, the nvme-tcp
io_cpu to begin with, but the core has no representation of an
individual queue and no place under the controller device to hang
per queue attributes.

Add a small nvme_queue_info that a transport embeds in its queue
structure, and helpers that register it as

  /sys/class/nvme/nvmeX/queues/<qid>/

with a transport provided ktype. The queues directory is created on
the first registration and released with the controller device.
Registration is expected once per controller lifetime, so the
directories persist while the transport's queues cycle across
reconnects.

Suggested-by: Sagi Grimberg <sagi@grimberg.me>
Link: https://lore.kernel.org/linux-nvme/13c160d0-0657-47ce-9a16-58d348267ab0@grimberg.me/
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
Signed-off-by: Saravanan D <saravanand@crusoe.ai>
---
 drivers/nvme/host/core.c  |  2 ++
 drivers/nvme/host/nvme.h  | 23 +++++++++++++++++++++
 drivers/nvme/host/sysfs.c | 42 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index beea23d04a70..211d05a1cc48 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -5197,6 +5197,8 @@ static void nvme_free_ctrl(struct device *dev)
 
 	ctrl->ops->free_ctrl(ctrl);
 
+	kobject_put(ctrl->queues_kobj);
+
 	if (subsys)
 		nvme_put_subsystem(subsys);
 }
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index bac25b287d25..20410f0d6118 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -339,6 +339,28 @@ enum nvme_ctrl_flags {
 	NVME_CTRL_FROZEN		= 6,
 };
 
+enum nvme_queue_info_flags {
+	NVME_QUEUE_INFO_REGISTERED	= 0,
+	NVME_QUEUE_INFO_IO_CPU_USER	= 1,
+};
+
+/*
+ * Common per-queue state, embedded in the transport's queue structure and
+ * backing the /sys/class/nvme/nvmeX/queues/<qid>/ directory.
+ */
+struct nvme_queue_info {
+	struct kobject		kobj;
+	unsigned int		qid;
+	unsigned long		flags;
+};
+
+struct nvme_ctrl;
+
+int nvme_register_queue_info(struct nvme_ctrl *ctrl,
+			     struct nvme_queue_info *qinfo, unsigned int qid,
+			     const struct kobj_type *ktype);
+void nvme_unregister_queue_info(struct nvme_queue_info *qinfo);
+
 struct nvme_ctrl {
 	bool comp_seen;
 	bool identified;
@@ -360,6 +382,7 @@ struct nvme_ctrl {
 	struct srcu_struct srcu;
 	struct device ctrl_device;
 	struct device *device;	/* char device */
+	struct kobject *queues_kobj;
 #ifdef CONFIG_NVME_HWMON
 	struct device *hwmon_device;
 #endif
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 4ac3ea3a86fd..a2780445c33e 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -1313,3 +1313,45 @@ const struct attribute_group *nvme_subsys_attrs_groups[] = {
 	&nvme_subsys_attrs_group,
 	NULL,
 };
+
+/*
+ * Per-queue sysfs directories under /sys/class/nvme/nvmeX/queues/. The
+ * transport backing the controller registers a directory for each of its
+ * queues and provides the attributes through the ktype. Registration is
+ * expected once per controller lifetime, so the directories persist while
+ * the transport's queues cycle across reconnects. The queues directory
+ * itself is released with the controller device.
+ */
+int nvme_register_queue_info(struct nvme_ctrl *ctrl,
+			     struct nvme_queue_info *qinfo, unsigned int qid,
+			     const struct kobj_type *ktype)
+{
+	int ret;
+
+	if (!ctrl->queues_kobj) {
+		ctrl->queues_kobj = kobject_create_and_add("queues",
+							   &ctrl->device->kobj);
+		if (!ctrl->queues_kobj)
+			return -ENOMEM;
+	}
+
+	qinfo->qid = qid;
+	ret = kobject_init_and_add(&qinfo->kobj, ktype, ctrl->queues_kobj,
+				   "%u", qid);
+	if (ret) {
+		kobject_put(&qinfo->kobj);
+		return ret;
+	}
+
+	set_bit(NVME_QUEUE_INFO_REGISTERED, &qinfo->flags);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(nvme_register_queue_info);
+
+void nvme_unregister_queue_info(struct nvme_queue_info *qinfo)
+{
+	if (!test_and_clear_bit(NVME_QUEUE_INFO_REGISTERED, &qinfo->flags))
+		return;
+	kobject_put(&qinfo->kobj);
+}
+EXPORT_SYMBOL_GPL(nvme_unregister_queue_info);
-- 
2.55.0

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

[Saravanan D]

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.

Embed the nvme_queue_info in the tcp queue and expose the io_cpu
through the controller's queues 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 cpu
number, -1 or "unbound", where -1 and "unbound" leave the socket work
unbound, running each invocation on the cpu that queued it. A user
assignment is marked in the queue info flags, persists across
reconnects and is never re-picked 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.

The queue directories register on the first connect and persist while
the queues cycle across reconnects, so a write can arrive while a
queue is torn down. The io_cpu changes and their accounting therefore
serialize under a controller level lock rather than the queue lock,
which is destroyed with the queue, and the controller teardown waits
for the last kobject release before the queue array is freed.

Suggested-by: Sagi Grimberg <sagi@grimberg.me>
Link: https://lore.kernel.org/linux-nvme/13c160d0-0657-47ce-9a16-58d348267ab0@grimberg.me/
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
Signed-off-by: Saravanan D <saravanand@crusoe.ai>
---
 Documentation/ABI/stable/sysfs-nvme |  21 ++++
 drivers/nvme/host/tcp.c             | 153 +++++++++++++++++++++++++++-
 2 files changed, 173 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/stable/sysfs-nvme b/Documentation/ABI/stable/sysfs-nvme
index a0bb88ca1694..4d0c7688e8f7 100644
--- a/Documentation/ABI/stable/sysfs-nvme
+++ b/Documentation/ABI/stable/sysfs-nvme
@@ -472,3 +472,24 @@ Contact:	Hannes Reinecke <hare@suse.de>
 Description:
 		Shows the subsystem type. Possible values: "discovery",
 		"nvm", "reserved".
+
+What:		/sys/class/nvme/nvmeX/queues/<qid>/io_cpu
+What:		/sys/class/nvme/nvmeX/queues/<qid>/managed
+Date:		September 2026
+KernelVersion:	7.4
+Contact:	Saravanan D <saravanand@crusoe.ai>
+Description:
+		Per I/O queue directories, populated by the transport
+		backing the controller and persistent across controller
+		reconnects. NVMe over TCP exposes:
+
+		io_cpu: (RW) The CPU that runs the socket work for I/O
+		queue <qid>, selected by the driver at connect time.
+		Writing a CPU number overrides the selection and persists
+		across reconnects. Writing -1 or "unbound" leaves the
+		socket work unbound, running each invocation on the CPU
+		that queued it. Reads return the current CPU, or -1 when
+		the queue is unbound.
+
+		managed: (RO) Shows who owns the CPU placement. 1 while
+		io_cpu is driver managed, 0 once the user assigned it.
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 921934028e0b..567c839e92a4 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -141,6 +141,8 @@ struct nvme_tcp_queue {
 	int                     tls_err;
 	struct page_frag_cache	pf_cache;
 
+	struct nvme_queue_info	q_info;
+
 	void (*state_change)(struct sock *);
 	void (*data_ready)(struct sock *);
 	void (*write_space)(struct sock *);
@@ -171,6 +173,11 @@ struct nvme_tcp_ctrl {
 	struct delayed_work	connect_work;
 	struct nvme_tcp_request async_req;
 	u32			io_queues[HCTX_MAX_TYPES];
+	/* serializes io_cpu changes and their nvme_tcp_cpu_queues accounting */
+	struct mutex		io_cpu_lock;
+	unsigned int		nr_queue_infos;
+	atomic_t		qinfo_refs;
+	struct completion	qinfo_release;
 };
 
 static struct workqueue_struct *nvme_tcp_wq;
@@ -1497,6 +1504,12 @@ static void nvme_tcp_free_queue(struct nvme_ctrl *nctrl, int qid)
 	if (!test_and_clear_bit(NVME_TCP_Q_ALLOCATED, &queue->flags))
 		return;
 
+	/* settle accounting a write racing the queue stop may have left */
+	mutex_lock(&ctrl->io_cpu_lock);
+	if (test_and_clear_bit(NVME_TCP_Q_IO_CPU_SET, &queue->flags))
+		atomic_dec(&nvme_tcp_cpu_queues[queue->io_cpu]);
+	mutex_unlock(&ctrl->io_cpu_lock);
+
 	page_frag_cache_drain(&queue->pf_cache);
 
 	/**
@@ -1716,9 +1729,19 @@ static void nvme_tcp_set_queue_io_cpu(struct nvme_tcp_queue *queue)
 	unsigned int *mq_map = NULL;
 	int cpu, min_queues = INT_MAX, io_cpu;
 
+	lockdep_assert_held(&ctrl->io_cpu_lock);
+
 	if (wq_unbound)
 		goto out;
 
+	/* A user assigned io_cpu is kept across reconnects */
+	if (test_bit(NVME_QUEUE_INFO_IO_CPU_USER, &queue->q_info.flags)) {
+		if (queue->io_cpu != WORK_CPU_UNBOUND &&
+		    !test_and_set_bit(NVME_TCP_Q_IO_CPU_SET, &queue->flags))
+			atomic_inc(&nvme_tcp_cpu_queues[queue->io_cpu]);
+		goto out;
+	}
+
 	if (nvme_tcp_default_queue(queue))
 		mq_map = set->map[HCTX_TYPE_DEFAULT].mq_map;
 	else if (nvme_tcp_read_queue(queue))
@@ -1836,6 +1859,114 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
 	return ret;
 }
 
+static struct nvme_tcp_queue *nvme_tcp_kobj_to_queue(struct kobject *kobj)
+{
+	struct nvme_queue_info *qinfo =
+		container_of(kobj, struct nvme_queue_info, kobj);
+
+	return container_of(qinfo, struct nvme_tcp_queue, q_info);
+}
+
+static ssize_t io_cpu_show(struct kobject *kobj, struct kobj_attribute *attr,
+			   char *buf)
+{
+	struct nvme_tcp_queue *queue = nvme_tcp_kobj_to_queue(kobj);
+	int io_cpu = READ_ONCE(queue->io_cpu);
+
+	return sysfs_emit(buf, "%d\n",
+			  io_cpu == WORK_CPU_UNBOUND ? -1 : io_cpu);
+}
+
+static ssize_t io_cpu_store(struct kobject *kobj, struct kobj_attribute *attr,
+			    const char *buf, size_t count)
+{
+	struct nvme_tcp_queue *queue = nvme_tcp_kobj_to_queue(kobj);
+	struct nvme_tcp_ctrl *ctrl = queue->ctrl;
+	int cpu, old;
+	int ret;
+
+	if (sysfs_streq(buf, "unbound")) {
+		cpu = WORK_CPU_UNBOUND;
+	} else {
+		ret = kstrtoint(buf, 0, &cpu);
+		if (ret)
+			return ret;
+		if (cpu == -1)
+			cpu = WORK_CPU_UNBOUND;
+		else if ((unsigned int)cpu >= nr_cpu_ids || !cpu_online(cpu))
+			return -EINVAL;
+	}
+
+	mutex_lock(&ctrl->io_cpu_lock);
+	old = xchg(&queue->io_cpu, cpu);
+	set_bit(NVME_QUEUE_INFO_IO_CPU_USER, &queue->q_info.flags);
+	if (test_bit(NVME_TCP_Q_IO_CPU_SET, &queue->flags)) {
+		atomic_dec(&nvme_tcp_cpu_queues[old]);
+		if (cpu != WORK_CPU_UNBOUND)
+			atomic_inc(&nvme_tcp_cpu_queues[cpu]);
+		else
+			clear_bit(NVME_TCP_Q_IO_CPU_SET, &queue->flags);
+	} else if (cpu != WORK_CPU_UNBOUND &&
+		   test_bit(NVME_TCP_Q_ALLOCATED, &queue->flags)) {
+		/* account a pin to a connected queue the pick left unbound */
+		atomic_inc(&nvme_tcp_cpu_queues[cpu]);
+		set_bit(NVME_TCP_Q_IO_CPU_SET, &queue->flags);
+	}
+	mutex_unlock(&ctrl->io_cpu_lock);
+
+	return count;
+}
+
+static ssize_t managed_show(struct kobject *kobj, struct kobj_attribute *attr,
+			    char *buf)
+{
+	struct nvme_tcp_queue *queue = nvme_tcp_kobj_to_queue(kobj);
+
+	return sysfs_emit(buf, "%d\n",
+			  !test_bit(NVME_QUEUE_INFO_IO_CPU_USER, &queue->q_info.flags));
+}
+
+static struct kobj_attribute nvme_tcp_io_cpu_attr =
+	__ATTR(io_cpu, 0644, io_cpu_show, io_cpu_store);
+static struct kobj_attribute nvme_tcp_managed_attr =
+	__ATTR_RO(managed);
+
+static struct attribute *nvme_tcp_queue_attrs[] = {
+	&nvme_tcp_io_cpu_attr.attr,
+	&nvme_tcp_managed_attr.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(nvme_tcp_queue);
+
+static void nvme_tcp_queue_info_release(struct kobject *kobj)
+{
+	struct nvme_tcp_queue *queue = nvme_tcp_kobj_to_queue(kobj);
+	struct nvme_tcp_ctrl *ctrl = queue->ctrl;
+
+	if (atomic_dec_and_test(&ctrl->qinfo_refs))
+		complete(&ctrl->qinfo_release);
+}
+
+static const struct kobj_type nvme_tcp_queue_ktype = {
+	.sysfs_ops	= &kobj_sysfs_ops,
+	.release	= nvme_tcp_queue_info_release,
+	.default_groups	= nvme_tcp_queue_groups,
+};
+
+static void nvme_tcp_register_queue_sysfs(struct nvme_tcp_queue *queue)
+{
+	struct nvme_tcp_ctrl *ctrl = queue->ctrl;
+
+	if (test_bit(NVME_QUEUE_INFO_REGISTERED, &queue->q_info.flags))
+		return;
+
+	/* a failed registration drops the reference through the release */
+	atomic_inc(&ctrl->qinfo_refs);
+	nvme_register_queue_info(&ctrl->ctrl, &queue->q_info,
+				 nvme_tcp_queue_id(queue),
+				 &nvme_tcp_queue_ktype);
+}
+
 static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl, int qid,
 				key_serial_t pskid)
 {
@@ -1906,7 +2037,8 @@ 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;
+	if (!test_bit(NVME_QUEUE_INFO_IO_CPU_USER, &queue->q_info.flags))
+		queue->io_cpu = WORK_CPU_UNBOUND;
 	queue->request = NULL;
 	queue->data_remaining = 0;
 	queue->ddgst_remaining = 0;
@@ -1974,6 +2106,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_register_queue_sysfs(queue);
+
 	return 0;
 
 err_init_connect:
@@ -2022,8 +2157,10 @@ static void nvme_tcp_stop_queue_nowait(struct nvme_ctrl *nctrl, int qid)
 	if (!test_bit(NVME_TCP_Q_ALLOCATED, &queue->flags))
 		return;
 
+	mutex_lock(&ctrl->io_cpu_lock);
 	if (test_and_clear_bit(NVME_TCP_Q_IO_CPU_SET, &queue->flags))
 		atomic_dec(&nvme_tcp_cpu_queues[queue->io_cpu]);
+	mutex_unlock(&ctrl->io_cpu_lock);
 
 	mutex_lock(&queue->queue_lock);
 	if (test_and_clear_bit(NVME_TCP_Q_LIVE, &queue->flags))
@@ -2085,7 +2222,9 @@ static int nvme_tcp_start_queue(struct nvme_ctrl *nctrl, int idx)
 	nvme_tcp_setup_sock_ops(queue);
 
 	if (idx) {
+		mutex_lock(&ctrl->io_cpu_lock);
 		nvme_tcp_set_queue_io_cpu(queue);
+		mutex_unlock(&ctrl->io_cpu_lock);
 		ret = nvmf_connect_io_queue(nctrl, idx);
 	} else
 		ret = nvmf_connect_admin_queue(nctrl);
@@ -2650,6 +2789,14 @@ static void nvme_tcp_free_ctrl(struct nvme_ctrl *nctrl)
 
 	nvmf_free_options(nctrl->opts);
 free_ctrl:
+	if (ctrl->queues) {
+		unsigned int qid;
+
+		for (qid = 1; qid < ctrl->nr_queue_infos; qid++)
+			nvme_unregister_queue_info(&ctrl->queues[qid].q_info);
+		if (!atomic_dec_and_test(&ctrl->qinfo_refs))
+			wait_for_completion(&ctrl->qinfo_release);
+	}
 	kfree(ctrl->queues);
 	kfree(ctrl);
 }
@@ -3047,6 +3194,10 @@ static struct nvme_tcp_ctrl *nvme_tcp_alloc_ctrl(struct device *dev,
 		ret = -ENOMEM;
 		goto out_free_ctrl;
 	}
+	ctrl->nr_queue_infos = ctrl->ctrl.queue_count;
+	mutex_init(&ctrl->io_cpu_lock);
+	atomic_set(&ctrl->qinfo_refs, 1);
+	init_completion(&ctrl->qinfo_release);
 
 	ret = nvme_init_ctrl(&ctrl->ctrl, dev, &nvme_tcp_ctrl_ops, 0);
 	if (ret)
-- 
2.55.0

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

[Saravanan D]

The Sashiko review of this series [1] found lifecycle bugs in the
sysfs registration, including a use after free of ctrl->queues_kobj
after ops->free_ctrl() and a reference cycle that leaks the controller
on every disconnect. I will address all of its comments in v5, so
please hold off on applying v4.

[1] https://sashiko.dev/#/patchset/20260927070925.47209-1-saravanand%40crusoe.ai

Thanks,
Saravanan D.

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

[Nilay Shroff]

On 9/27/26 12:39 PM, 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.
> 
> Embed the nvme_queue_info in the tcp queue and expose the io_cpu
> through the controller's queues 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 cpu
> number, -1 or "unbound", where -1 and "unbound" leave the socket work
> unbound, running each invocation on the cpu that queued it. A user
> assignment is marked in the queue info flags, persists across
> reconnects and is never re-picked 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.
> 
> The queue directories register on the first connect and persist while
> the queues cycle across reconnects, so a write can arrive while a
> queue is torn down. The io_cpu changes and their accounting therefore
> serialize under a controller level lock rather than the queue lock,
> which is destroyed with the queue, and the controller teardown waits
> for the last kobject release before the queue array is freed.
> 
I understand based on your requirement you may want to persist io_cpu
changes (when managed by user) during controller reconnect. However
in case num of queues changes after reconnect (assume it's 32 at first
connect but after controller reconnects it's reduced to 16) keeping
those 32 queues entries under queues/<qid>/ looks bit odd and not
correct. I'd expect queues entries under queue/<qid>/ to be also
adjusted based on the controller queue count.

[...]

> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 921934028e0b..567c839e92a4 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -141,6 +141,8 @@ struct nvme_tcp_queue {
>   	int                     tls_err;
>   	struct page_frag_cache	pf_cache;
>   
> +	struct nvme_queue_info	q_info;
> +
>   	void (*state_change)(struct sock *);
>   	void (*data_ready)(struct sock *);
>   	void (*write_space)(struct sock *);
> @@ -171,6 +173,11 @@ struct nvme_tcp_ctrl {
>   	struct delayed_work	connect_work;
>   	struct nvme_tcp_request async_req;
>   	u32			io_queues[HCTX_MAX_TYPES];
> +	/* serializes io_cpu changes and their nvme_tcp_cpu_queues accounting */
> +	struct mutex		io_cpu_lock;

The io_cpu_lock protect updates to queue->io_cpu and as we now support clang
context annotations for NVMe host driver, I'd suggest you annotate the
queue->io_cpu with __guarded_by(...) so that clang context analyzer can
validate each access to queue->io_cpu. Furthermore, I see that queue->io_cpu
could be concurrently accessed from both control path and I/O hotpath. So how
would we protect it while it's being accessed from I/O path and concurrently
updated from sysfs path?
> +	unsigned int		nr_queue_infos;
> +	atomic_t		qinfo_refs;
> +	struct completion	qinfo_release;
>   };

I think the qinfo_refs and qinfo_release are added for tracking the
lifecycle of nvme_queue_info object. But do we really need to do
that ?
When a nvme_queue_info->kobj is initialized/added, its parent kobject
is referenced by the kobject infrastructure. Therefore, as long as a
queue-info kobject exists, its parent queues_kobj remains alive, which
in turn keeps ctrl->device->kobj alive. This should already prevent the
controller object from being released while a queue-info kobject still
exists. Could we therefore avoid maintaining a second qinfo_refs
reference count and completion? It seems that entire kobject parent
chain already provides the lifetime dependency that qinfo_refs is
trying to enforce.

[...]

> +static ssize_t io_cpu_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	struct nvme_tcp_queue *queue = nvme_tcp_kobj_to_queue(kobj);
> +	struct nvme_tcp_ctrl *ctrl = queue->ctrl;
> +	int cpu, old;
> +	int ret;
> +
> +	if (sysfs_streq(buf, "unbound")) {
> +		cpu = WORK_CPU_UNBOUND;
> +	} else {
> +		ret = kstrtoint(buf, 0, &cpu);
> +		if (ret)
> +			return ret;
> +		if (cpu == -1)
> +			cpu = WORK_CPU_UNBOUND;
> +		else if ((unsigned int)cpu >= nr_cpu_ids || !cpu_online(cpu))
> +			return -EINVAL;
> +	}
> +
> +	mutex_lock(&ctrl->io_cpu_lock);
> +	old = xchg(&queue->io_cpu, cpu);
> +	set_bit(NVME_QUEUE_INFO_IO_CPU_USER, &queue->q_info.flags);
> +	if (test_bit(NVME_TCP_Q_IO_CPU_SET, &queue->flags)) {
> +		atomic_dec(&nvme_tcp_cpu_queues[old]);
> +		if (cpu != WORK_CPU_UNBOUND)
> +			atomic_inc(&nvme_tcp_cpu_queues[cpu]);
> +		else
> +			clear_bit(NVME_TCP_Q_IO_CPU_SET, &queue->flags);
> +	} else if (cpu != WORK_CPU_UNBOUND &&
> +		   test_bit(NVME_TCP_Q_ALLOCATED, &queue->flags)) {
> +		/* account a pin to a connected queue the pick left unbound */
> +		atomic_inc(&nvme_tcp_cpu_queues[cpu]);
> +		set_bit(NVME_TCP_Q_IO_CPU_SET, &queue->flags);
> +	}
> +	mutex_unlock(&ctrl->io_cpu_lock);
> +
This look overly complicated. In a simple scheme can't we get rid off
NVME_QUEUE_INFO_IO_CPU_USER and instead use NVME_TCP_Q_IO_CPU_SET as an
indicator for differentiating between io_cpu is managed by user or driver?
For instance,

NVME_TCP_Q_IO_CPU_SET = 1
     - driver has selected a specific io_cpu

NVME_TCP_Q_IO_CPU_SET = 0 && io_cpu != WORK_CPU_UNBOUND
     - user has selected a specific io_cpu

NVME_TCP_Q_IO_CPU_SET = 0 && io_cpu == WORK_CPU_UNBOUND
     - unbound; workqueue chooses the execution CPU and so it's driver managed

[...]

Other that what sashiko provided in the review feedback, above are my
few additional comments.

Thanks,
--Nilay

Re: [PATCH v4 1/2] nvme: add per-queue sysfs directories

[Nilay Shroff]

> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index bac25b287d25..20410f0d6118 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -339,6 +339,28 @@ enum nvme_ctrl_flags {
>   	NVME_CTRL_FROZEN		= 6,
>   };
>   
> +enum nvme_queue_info_flags {
> +	NVME_QUEUE_INFO_REGISTERED	= 0,
> +	NVME_QUEUE_INFO_IO_CPU_USER	= 1,
> +};

NVME_QUEUE_INFO_IO_CPU_USER appears to be a transport-specific flag,
so I don't think it belongs in the common NVMe core code. It would
be better to keep this state in the transport-specific queue structure.

[...]

> +/*
> + * Per-queue sysfs directories under /sys/class/nvme/nvmeX/queues/. The
> + * transport backing the controller registers a directory for each of its
> + * queues and provides the attributes through the ktype. Registration is
> + * expected once per controller lifetime, so the directories persist while
> + * the transport's queues cycle across reconnects. The queues directory
> + * itself is released with the controller device.
> + */
> +int nvme_register_queue_info(struct nvme_ctrl *ctrl,
> +			     struct nvme_queue_info *qinfo, unsigned int qid,
> +			     const struct kobj_type *ktype)
> +{
> +	int ret;
> +
> +	if (!ctrl->queues_kobj) {
> +		ctrl->queues_kobj = kobject_create_and_add("queues",
> +							   &ctrl->device->kobj);
> +		if (!ctrl->queues_kobj)
> +			return -ENOMEM;
> +	}
> +
If this function can be invoked concurrently, how is ctrl->queues_kobj creation
serialized? For example, if two callers concurrently pass the !ctrl->queues_kobj
check, both could call kobject_create_and_add(), which is not what we want.

I understand that the current user of this API in patch 2/2 invokes it serially.
If serialization is an API requirement, could we document that in the function
comment? Alternatively, if this API is expected to be safe for concurrent callers,
the creation of queues_kobj should be serialized within this function.

In particular, I'd prefer the API contract to make the expected serialization
explicit rather than relying on the current caller to invoke it serially.

Thanks,
--Nilay

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

[Saravanan D]

On 10/8/26 2:45 AM, Nilay Shroff wrote:
[...]
>> The queue directories register on the first connect and persist while
>> the queues cycle across reconnects, so a write can arrive while a
>> queue is torn down. The io_cpu changes and their accounting therefore
>> serialize under a controller level lock rather than the queue lock,
>> which is destroyed with the queue, and the controller teardown waits
>> for the last kobject release before the queue array is freed.
>>
> I understand based on your requirement you may want to persist io_cpu
> changes (when managed by user) during controller reconnect. However
> in case num of queues changes after reconnect (assume it's 32 at first
> connect but after controller reconnects it's reduced to 16) keeping
> those 32 queues entries under queues/<qid>/ looks bit odd and not
> correct. I'd expect queues entries under queue/<qid>/ to be also
> adjusted based on the controller queue count.

Agreed. In v5 the queue directory follows the queue's connected
lifetime. nvme_tcp_alloc_queue() adds it and nvme_tcp_free_queue()
removes it, so queues/<qid>/ matches the queue count after every
reconnect, as blk-mq does for mq/<n> when nr_hw_queues changes. A user
assigned io_cpu is kept in the nvme_tcp_queue, which persists across
reconnects, so it applies again when the queue comes back. A write
while the controller is reconnecting fails with ENOENT.

[...]
>> @@ -171,6 +173,11 @@ struct nvme_tcp_ctrl {
>>   	struct delayed_work	connect_work;
>>   	struct nvme_tcp_request async_req;
>>   	u32			io_queues[HCTX_MAX_TYPES];
>> +	/* serializes io_cpu changes and their nvme_tcp_cpu_queues accounting */
>> +	struct mutex		io_cpu_lock;
>
> The io_cpu_lock protect updates to queue->io_cpu and as we now support clang
> context annotations for NVMe host driver, I'd suggest you annotate the
> queue->io_cpu with __guarded_by(...) so that clang context analyzer can
> validate each access to queue->io_cpu. Furthermore, I see that queue->io_cpu
> could be concurrently accessed from both control path and I/O hotpath. So how
> would we protect it while it's being accessed from I/O path and concurrently
> updated from sysfs path?

The I/O path reads io_cpu without the lock by design, and a stale read
is benign. Each queue_work_on() call reads io_cpu once, so a call that
races with the store queues io_work on the old cpu one last time and
later calls use the new one. io_work is a single work item, so it
never runs on both cpus at once. The inline send in
nvme_tcp_queue_request() only compares io_cpu with the current cpu and
is serialized by send_mutex. v5 marks the lockless reads with
READ_ONCE() and the writes with WRITE_ONCE().

Annotating io_cpu with __guarded_by(&ctrl->io_cpu_lock) would need
context_unsafe() around each of those hot path reads, so I'd rather
document the lockless readers next to the field. Happy to add the
annotation if you prefer it.

>> +	unsigned int		nr_queue_infos;
>> +	atomic_t		qinfo_refs;
>> +	struct completion	qinfo_release;
>>   };
>
> I think the qinfo_refs and qinfo_release are added for tracking the
> lifecycle of nvme_queue_info object. But do we really need to do
> that ?
> When a nvme_queue_info->kobj is initialized/added, its parent kobject
> is referenced by the kobject infrastructure. Therefore, as long as a
> queue-info kobject exists, its parent queues_kobj remains alive, which
> in turn keeps ctrl->device->kobj alive. This should already prevent the
> controller object from being released while a queue-info kobject still
> exists. Could we therefore avoid maintaining a second qinfo_refs
> reference count and completion? It seems that entire kobject parent
> chain already provides the lifetime dependency that qinfo_refs is
> trying to enforce.

Agreed. With the directory removed in nvme_tcp_free_queue(), no queue
directory outlives its queue, and the kobject release only frees its
own allocation, so the parent chain is enough. v5 drops qinfo_refs,
the completion and nr_queue_infos. Each connection allocates its own
kobject, so a reconnect never reuses one.

[...]
> This look overly complicated. In a simple scheme can't we get rid off
> NVME_QUEUE_INFO_IO_CPU_USER and instead use NVME_TCP_Q_IO_CPU_SET as an
> indicator for differentiating between io_cpu is managed by user or driver?
> For instance,
>
> NVME_TCP_Q_IO_CPU_SET = 1
>      - driver has selected a specific io_cpu
>
> NVME_TCP_Q_IO_CPU_SET = 0 && io_cpu != WORK_CPU_UNBOUND
>      - user has selected a specific io_cpu
>
> NVME_TCP_Q_IO_CPU_SET = 0 && io_cpu == WORK_CPU_UNBOUND
>      - unbound; workqueue chooses the execution CPU and so it's driver managed

I drew your scheme as a state diagram to compare it with v4.

              DRIVER MANAGED          |         USER MANAGED
   (SET=1, or SET=0 && UNBOUND)       |     (SET=0 && io_cpu=c)
                                      |
 connect +--------------------+ store(c)  +--------------------+
 ------->| A  driver chosen   |-------+-->| C  user pin        |
         |    io_cpu=c, SET=1 |       |   |    io_cpu=c, SET=0 |
         |    counted         |       |   |    not counted (1) |
         +--------------------+       |   +--------------------+
           | stop, SET=0 and          |     |
           | io_cpu must be reset (2) |     | store(-1)
           v                          |     |
         +--------------------+       |     |
         | B  unbound         |<------+-----+
         |    io_cpu=UNBOUND  |       |
         |    SET=0           |       |   +- - - - - - - - - - +
         |    also where a    |       |   : D  user unbound    :
         |    user -1 lands(3)|       |   :    not reachable,  :
         |                    |       |   :    see (3)         :
         +--------------------+       |   +- - - - - - - - - - +
           | next connect, reselect   |
           +--> A                     |

(1) A user pin has SET=0, so it drops out of nvme_tcp_cpu_queues.
    When the driver selects io_cpu for other queues and other
    controllers in nvme_tcp_set_queue_io_cpu(), it sees that cpu as
    idle and keeps placing queues on it. v4 counts every bound queue
    regardless of who assigned it.

(2) SET clears at stop. Unless the stop path also resets io_cpu to
    WORK_CPU_UNBOUND once io_work is cancelled, a driver chosen io_cpu
    reads as a user pin after the reconnect and the driver never
    selects a cpu for that queue again. In v4 only the sysfs store
    sets the USER bit, so connect and stop never touch ownership.

(3) The user managed side has no unbound state. A -1 written by the
    user is the same state as a queue the driver has not bound yet,
    so managed reads 1 and the next connect binds it again. Also,
    nvme_tcp_wq is a per-cpu workqueue, so an unbound io_work runs
    on the cpu that queued it rather than on a cpu chosen by the
    workqueue.

The USER bit records who owns io_cpu and survives reconnects, while
SET records whether io_cpu is in the ledger and follows connect and
stop. I'd like to keep both, with USER moved into nvme_tcp_queue flags
as you asked on patch 1. The complexity you point at is the extra
branch in the store, which collapses to a single path with the same
behavior in v5.

	mutex_lock(&ctrl->io_cpu_lock);
	old = xchg(&queue->io_cpu, cpu);
	set_bit(NVME_TCP_Q_IO_CPU_USER, &queue->flags);
	if (test_and_clear_bit(NVME_TCP_Q_IO_CPU_SET, &queue->flags))
		atomic_dec(&nvme_tcp_cpu_queues[old]);
	if (cpu != WORK_CPU_UNBOUND &&
	    test_bit(NVME_TCP_Q_ALLOCATED, &queue->flags)) {
		atomic_inc(&nvme_tcp_cpu_queues[cpu]);
		set_bit(NVME_TCP_Q_IO_CPU_SET, &queue->flags);
	}
	mutex_unlock(&ctrl->io_cpu_lock);

Does that work for you?

> Other that what sashiko provided in the review feedback, above are my
> few additional comments.

Thanks for the review. v5 also fixes the Sashiko findings.

Thanks,
Saravanan D.