Re: [PATCH hci-8.1 v2 1/2] vhost-blk: create kernel threads with the mainstream worker ioctls

[Alexander Ivanov <alexander.ivanov@virtuozzo.com>]

> The overall logic is fine, but I'd like patch to be simplified.
>
> First, there're dedicated vhost_ops->vhost_new_worker() and
> vhost_ops->vhost_attach_vring_worker() calls.  We should reuse them.
> For consistency, let's add another patch "vhost-backend: add
> vhost_set_fork_from_owner op".  Smth like:
>
>>    /* hw/virtio/vhost-backend.c */
>>    static int vhost_kernel_set_fork_from_owner(struct vhost_dev *dev,
>>                                                uint8_t fork_owner)
>>    {
>>        return vhost_kernel_call(dev, VHOST_SET_FORK_FROM_OWNER, &fork_owner);
>>    }
Done.
> Other issues (by Claude):
>
> 1. Dead fallback in vhost_blk_set_workers(). The ENOTTY && i == 1 branch
> handles a kernel that has VHOST_SET_FORK_FROM_OWNER (6.17 / vz10) but
> not VHOST_NEW_WORKER (6.4) — no such kernel exists. Once the fork-mode
> ioctl succeeded, a worker-ioctl failure is a real error.  Drop, do a
> single fallback point in start().
The fallback is not dead. VHOST_SET_FORK_FROM_OWNER is in our
6.12 kernel as a RHEL 10 backport. The worker ioctls are generic,
but each vhost driver has to route them to vhost_worker_ioctl(),
and vhost-blk only does that with "drivers/vhost/blk: add support
for worker ioctls" (VSTOR-107537), which is not merged yet.
> 2. Fork-mode block in vhost_blk_start() is hard to read. A declaration
> inside the if, the ENOTTY test nested under 'ret != -ENOTTY', the
> 'nworkers = 1' fall-through at the bottom. Switch to flat three-way if
> (ret == -ENOTTY) … else if (ret < 0) … at the call site, ioctl plumbing
> in the backend.
Done.
> 3. Redundant clamp. MIN(nworkers, VHOST_BLK_MAX_QUEUES) is already
> guaranteed by nworkers ≤ nvqs ≤ VHOST_BLK_MAX_QUEUES (realize, :666); a
> clamp hides an invariant. Switch to assert(nworkers <= ARRAY_SIZE(ids)).
Done, switched to the assert. To make it hold, I added a patch that rejects
num-queues above VHOST_BLK_MAX_QUEUES at realize: before, only the
auto value was capped there.
> 4. memset() + field stores for two structs per iteration. Switch to
> designated initializers, and the two loops merged into one: thread w is
> created by its first user vq (i < nworkers), then attached — same ioctl
> count, one error path per ioctl, loop body reads as "which thread,
> create if first, attach".
Done.  The structs use designated initializers, and the two loops
are merged into one: a thread is created by its first vq, then attached.
> 5. s->dev. repeated throughout. Use struct vhost_dev *hdev = &s->dev;
> once, as vhost-scsi does; vhost_blk_set_workers() takes hdev and no
> longer needs VHostBlk.
Done. vhost_blk_start() uses hdev throughout, and vhost_blk_set_workers()
takes hdev instead of VHostBlk.
> 6. Commit message nits (1/2). "The kernel is dropping it" needs the
> kernel change/ticket; say that VHOST_NEW_WORKER returns -EFAULT (not
> ENOTTY) in kthread mode, which is why the mode switch — not the worker
> ioctl — is the capability probe; "default worker being the first one"
> belongs in the code comment, where it already is.
>
> Here's what the changed version might look like:
>
>> /*
>>     * Spread the virtqueues round-robin over nworkers kernel threads. Thread 0
>>     * is the default worker VHOST_SET_OWNER created; vq 0 and every nworkers-th
>>     * vq after it stay on it.
>>     */
>>    static int vhost_blk_set_workers(struct vhost_dev *hdev, int nworkers)
>>    {
>>        uint32_t ids[VHOST_BLK_MAX_QUEUES];     /* ids[0]: default worker, unused */
>>        int i, ret;
>>
>>        assert(nworkers <= ARRAY_SIZE(ids));
>>
>>        for (i = 1; i < hdev->nvqs; i++) {
>>            int w = i % nworkers;
>>            struct vhost_vring_worker vq_worker = { .index = i };
>>
>>            if (w == 0) {
>>                continue;
>>            }
>>
>>            /* vq i < nworkers is the first user of thread i: create it here */
>>            if (i < nworkers) {
>>                struct vhost_worker_state worker = { };
>>
>>                ret = hdev->vhost_ops->vhost_new_worker(hdev, &worker);
>>                if (ret < 0) {
>>                    error_report("vhost-blk: unable to create kernel thread: %s",
>>                                 strerror(-ret));
>>                    return ret;
>>                }
>>                ids[w] = worker.worker_id;
>>            }
>>
>>            vq_worker.worker_id = ids[w];
>>            ret = hdev->vhost_ops->vhost_attach_vring_worker(hdev, &vq_worker);
>>            if (ret < 0) {
>>                error_report("vhost-blk: unable to attach kernel thread to "
>>                             "queue %d: %s", i, strerror(-ret));
>>                return ret;
>>            }
>>        }
>>
>>        return 0;
>>    }
>>
>>    static int vhost_blk_start(VirtIODevice *vdev)
>>    {
>>        VHostBlk *s = VHOST_BLK(vdev);
>>        struct vhost_dev *hdev = &s->dev;
>>        ...
>>        int ret, i, nworkers;
>>        ...
>>        if (s->vhost_started) {
>>            return 0;
>>        }
>>
>>        nworkers = MIN(s->conf.num_threads, hdev->nvqs);
>>        if (nworkers > 1) {
>>            /*
>>             * VHOST_NEW_WORKER needs vhost_task mode; our kernel defaults to
>>             * kthreads and VHOST_RESET_OWNER brings that default back, so ask
>>             * on every start. Must precede VHOST_SET_OWNER.
>>             */
>>            ret = hdev->vhost_ops->vhost_set_fork_from_owner(hdev,
>>                                                             VHOST_FORK_OWNER_TASK);
>>            if (ret == -ENOTTY) {
>>                warn_report_once("vhost-blk: kernel does not support vhost tasks, "
>>                                 "using a single kernel thread");
>>                nworkers = 1;
>>            } else if (ret < 0) {
>>                error_report("vhost-blk: unable to set vhost task mode: %s",
>>                             strerror(-ret));
>>                return ret;
>>            }
>>        }
>>
>>        if (ioctl(s->vhostfd, VHOST_SET_OWNER, NULL)) {
>>            error_report("vhost-blk: unable to set owner");
>>            return -ENOSYS;
>>        }
>>
>>        if (nworkers > 1) {
>>            ret = vhost_blk_set_workers(hdev, nworkers);
>>            if (ret < 0) {
>>                return ret;
>>            }
>>        }
>>
>>        if (s->conf.serial) {
>>        ...
Done.