Re: [PATCH v4 2/2] nvme-tcp: allow setting per-queue io_cpu through sysfs
From: Nilay Shroff
Date: Thu Oct 08 2026 - 05:51:47 EST
On 9/27/26 12:39 PM, Saravanan D wrote:
nvme_tcp_set_queue_io_cpu() picks each queue's io_cpu at connect timeI understand based on your requirement you may want to persist io_cpu
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.
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,This look overly complicated. In a simple scheme can't we get rid off
+ 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);
+
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