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

[Nilay Shroff <nilay@linux.ibm.com>]

On 10/9/26 7:31 AM, Saravanan D wrote:
> 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.

Yes, reading io_cpu locklessly in the I/O hot path is intentional. However,
to avoid KCSAN splat, we should mark read and write access with READ_ONCE()
and WRITE_ONCE() respectively.

Unfortunately, the Clang context analyzer cannot currently model a lock that
serializes writers while allowing readers to access the protected variable
without holding that lock. Annotating io_cpu with __guarded_by() would therefore
require us to annotate lockless reads with context_unsafe() or data_race(),
which I think we'd prefer to avoid.

I suggest introducing two helpers: one to read io_cpu and another to update it:

static inline int nvme_tcp_queue_io_cpu(struct nvme_tcp_queue *queue)
{
	return READ_ONCE(queue->io_cpu);
}

static inline void nvme_tcp_queue_change_io_cpu(struct nvme_tcp_queue *queue,
		int cpu)
	__must_hold(&queue->ctrl->io_cpu_lock)
{
	WRITE_ONCE(queue->io_cpu, cpu);
}

All readers could then access queue->io_cpu through nvme_tcp_queue_io_cpu(),
while all writes would go through nvme_tcp_queue_change_io_cpu(). The __must_hold()
annotation on the update helper would allow the Clang context analyzer to verify
that callers hold io_cpu_lock when modifying io_cpu, without requiring annotations
for the intentional lockless reads.

> 
>>> +	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.
> 
May not be true always when nvme_tcp_wq is inited with wq_unbound. We
have a module option using which we could toggle this behavior.

> 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?
> 
This looks good. However, I wonder why we need to check NVME_TCP_Q_ALLOCATED
here? If the user can update io_cpu only while the queue's sysfs directory
exists, and that directory is created and removed along with the queue's
allocation and teardown, shouldn't we be able to drop this check?

>> 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,
--Nilay