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

[Saravanan D]

On 10/11/26 1:27 PM, Nilay Shroff wrote:
> On 10/9/26 7:31 AM, Saravanan D wrote:
[...]
>> 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.

Agreed. v5 adds both helpers as you wrote them. Every reader goes
through nvme_tcp_queue_io_cpu(), and the two writers, the cpu
selection at connect and the sysfs store, go through
nvme_tcp_queue_change_io_cpu() with io_cpu_lock held.

[...]
>> (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.

You're right, that holds only for the default per-cpu nvme_tcp_wq.
With wq_unbound the workqueue picks the cpu as you describe, and
nvme_tcp_set_queue_io_cpu() leaves every driver managed queue unbound.
A user written io_cpu still selects the worker pool for that cpu, so
how closely io_work follows it depends on the affinity_scope of
nvme_tcp_wq. With affinity_scope=cpu and affinity_strict=1, io_work
runs on that cpu, provided the cpu is in the cpumask of nvme_tcp_wq.
With the default cache_shard scope, io_work starts on a cpu in the
same shard, and the scheduler may move it afterwards.
v5 documents this in the ABI entry.

[...]
>> 	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?

Agreed, v5 drops the check. nvme_tcp_free_queue() removes the
directory before it clears NVME_TCP_Q_ALLOCATED, so no store can run
on a freed queue. The SET backstop stays after the removal, for a
store that lands between the queue stop and the free.

Thanks,
Saravanan D.