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.