Virtuozzo QEMU development (svt-core@virtuozzo.com)
 help / color / mirror / Atom feed
* [PATCH hci-8.1 v2 0/2] vhost-blk: switch from VHOST_SET_NWORKERS to the mainstream worker ioctls
@ 2026-10-27 12:47 Alexander Ivanov
  2026-10-27 12:47 ` [PATCH hci-8.1 v2 1/2] vhost-blk: create kernel threads with " Alexander Ivanov
  2026-10-27 12:47 ` [PATCH hci-8.1 v2 2/2] linux-headers: drop VHOST_SET_NWORKERS Alexander Ivanov
  0 siblings, 2 replies; 9+ messages in thread
From: Alexander Ivanov @ 2026-10-27 12:47 UTC (permalink / raw)
  To: andrey.zhadchenko; +Cc: andrey.drobyshev, svt-core

The kernel drops our VHOST_SET_NWORKERS ioctl in favour of the
mainstream VHOST_NEW_WORKER/VHOST_ATTACH_VRING_WORKER, which vhost-blk
now supports.

Patch 1 makes vhost-blk create its num-threads workers with the
mainstream ioctls, keeping the same worker layout, and fall back to a
single thread on a kernel without them. Patch 2 drops
VHOST_SET_NWORKERS from linux-headers.

v2:
- patch 1: clamp num-threads to VHOST_BLK_MAX_QUEUES
- patch 1: note in the commit message that our kernel, unlike
  mainline, defaults to kthreads

https://virtuozzo.atlassian.net/browse/VSTOR-107537

Alexander Ivanov (2):
  vhost-blk: create kernel threads with the mainstream worker ioctls
  linux-headers: drop VHOST_SET_NWORKERS

 hw/block/vhost-blk.c        | 76 +++++++++++++++++++++++++++++++++++--
 linux-headers/linux/vhost.h |  9 -----
 2 files changed, 72 insertions(+), 13 deletions(-)

-- 
2.53.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH hci-8.1 v2 1/2] vhost-blk: create kernel threads with the mainstream worker ioctls
  2026-10-27 12:47 [PATCH hci-8.1 v2 0/2] vhost-blk: switch from VHOST_SET_NWORKERS to the mainstream worker ioctls Alexander Ivanov
@ 2026-10-27 12:47 ` Alexander Ivanov
  2026-10-27 12:52   ` no-reply
  2026-10-27 14:22   ` Andrey Drobyshev
  2026-10-27 12:47 ` [PATCH hci-8.1 v2 2/2] linux-headers: drop VHOST_SET_NWORKERS Alexander Ivanov
  1 sibling, 2 replies; 9+ messages in thread
From: Alexander Ivanov @ 2026-10-27 12:47 UTC (permalink / raw)
  To: andrey.zhadchenko; +Cc: andrey.drobyshev, svt-core

num-threads was passed to our own VHOST_SET_NWORKERS kernel ioctl.
The kernel is dropping it in favour of the mainstream worker ioctls,
which vhost-blk now supports, so use those: create num-threads - 1
workers with VHOST_NEW_WORKER and attach them to the virtqueues
round-robin with VHOST_ATTACH_VRING_WORKER, the default worker being
the first one. This is the layout VHOST_SET_NWORKERS produced.

VHOST_NEW_WORKER works only in vhost_task mode, while our kernel,
unlike mainline, defaults to kthreads, so switch the device to
vhost_task mode with VHOST_SET_FORK_FROM_OWNER before VHOST_SET_OWNER.
VHOST_RESET_OWNER restores the default, hence this is done on every
start. With num-threads=1 nothing changes: no worker ioctls, kthread
mode.

On a kernel without these ioctls warn and run a single thread rather
than fail to start vhost-blk.

https://virtuozzo.atlassian.net/browse/VSTOR-107537

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 hw/block/vhost-blk.c | 76 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 72 insertions(+), 4 deletions(-)

diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
index abddff02c6d..7b427083478 100644
--- a/hw/block/vhost-blk.c
+++ b/hw/block/vhost-blk.c
@@ -33,6 +33,52 @@
 #include "migration/cpr.h"
 #include "system/runstate.h"
 
+/*
+ * Spread the virtqueues round-robin over num-threads workers, the first one
+ * being the default worker that VHOST_SET_OWNER created.
+ */
+static int vhost_blk_set_workers(VHostBlk *s, int nworkers)
+{
+    uint32_t ids[VHOST_BLK_MAX_QUEUES];
+    struct vhost_worker_state worker;
+    struct vhost_vring_worker vq_worker;
+    int ret, i;
+
+    nworkers = MIN(nworkers, VHOST_BLK_MAX_QUEUES);
+    for (i = 1; i < nworkers; i++) {
+        memset(&worker, 0, sizeof(worker));
+        if (ioctl(s->vhostfd, VHOST_NEW_WORKER, &worker)) {
+            ret = -errno;
+            if (ret == -ENOTTY && i == 1) {
+                warn_report_once("vhost-blk: kernel does not support worker "
+                                 "ioctls, using a single kernel thread");
+                return 0;
+            }
+            error_report("vhost-blk: unable to create kernel thread: %s",
+                         strerror(-ret));
+            return ret;
+        }
+        ids[i] = worker.worker_id;
+    }
+
+    for (i = 0; i < s->dev.nvqs; i++) {
+        if (i % nworkers == 0) {
+            continue;
+        }
+        memset(&vq_worker, 0, sizeof(vq_worker));
+        vq_worker.index = i;
+        vq_worker.worker_id = ids[i % nworkers];
+        if (ioctl(s->vhostfd, VHOST_ATTACH_VRING_WORKER, &vq_worker)) {
+            ret = -errno;
+            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);
@@ -51,15 +97,37 @@ static int vhost_blk_start(VirtIODevice *vdev)
         return 0;
     }
 
+    nworkers = MIN(s->conf.num_threads, s->dev.nvqs);
+    if (nworkers > 1) {
+        /*
+         * VHOST_NEW_WORKER works only in vhost_task mode, and
+         * VHOST_RESET_OWNER brings the kthread default back.
+         */
+        uint8_t fork_owner = VHOST_FORK_OWNER_TASK;
+
+        if (ioctl(s->vhostfd, VHOST_SET_FORK_FROM_OWNER, &fork_owner)) {
+            ret = -errno;
+            if (ret != -ENOTTY) {
+                error_report("vhost-blk: unable to set vhost task mode: %s",
+                             strerror(-ret));
+                return ret;
+            }
+            warn_report_once("vhost-blk: kernel does not support vhost "
+                             "tasks, using a single kernel thread");
+            nworkers = 1;
+        }
+    }
+
     if (ioctl(s->vhostfd, VHOST_SET_OWNER, NULL)) {
         error_report("vhost-blk: unable to set owner");
         return -ENOSYS;
     }
 
-    nworkers = s->conf.num_threads;
-    if (nworkers != 1 && ioctl(s->vhostfd, VHOST_SET_NWORKERS, &nworkers)) {
-        error_report("vhost-blk: unable to set number of kernel threads");
-        return -EINVAL;
+    if (nworkers > 1) {
+        ret = vhost_blk_set_workers(s, nworkers);
+        if (ret < 0) {
+            return ret;
+        }
     }
 
     if (s->conf.serial) {
-- 
2.53.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH hci-8.1 v2 2/2] linux-headers: drop VHOST_SET_NWORKERS
  2026-10-27 12:47 [PATCH hci-8.1 v2 0/2] vhost-blk: switch from VHOST_SET_NWORKERS to the mainstream worker ioctls Alexander Ivanov
  2026-10-27 12:47 ` [PATCH hci-8.1 v2 1/2] vhost-blk: create kernel threads with " Alexander Ivanov
@ 2026-10-27 12:47 ` Alexander Ivanov
  2026-10-27 12:50   ` no-reply
  2026-10-27 14:22   ` Andrey Drobyshev
  1 sibling, 2 replies; 9+ messages in thread
From: Alexander Ivanov @ 2026-10-27 12:47 UTC (permalink / raw)
  To: andrey.zhadchenko; +Cc: andrey.drobyshev, svt-core

vhost-blk no longer uses it, and the kernel drops the ioctl.

https://virtuozzo.atlassian.net/browse/VSTOR-107537

Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
---
 linux-headers/linux/vhost.h | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/linux-headers/linux/vhost.h b/linux-headers/linux/vhost.h
index baad06412d4..b6eb628d973 100644
--- a/linux-headers/linux/vhost.h
+++ b/linux-headers/linux/vhost.h
@@ -102,15 +102,6 @@
 #define VHOST_GET_VRING_WORKER _IOWR(VHOST_VIRTIO, 0x16,		\
 				     struct vhost_vring_worker)
 
-/* Set number of vhost workers
- * Currently number of vhost workers can only be increased.
- * All workers are freed upon reset.
- * If the value is too big it is silently truncated to the maximum number of
- * supported vhost workers
- * Even if the error is returned it is possible that some workers were created
- */
-#define VHOST_SET_NWORKERS _IOW(VHOST_VIRTIO, 0x1F, int)
-
 /* The following ioctls use eventfd file descriptors to signal and poll
  * for events. */
 
-- 
2.53.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH hci-8.1 v2 2/2] linux-headers: drop VHOST_SET_NWORKERS
  2026-10-27 12:47 ` [PATCH hci-8.1 v2 2/2] linux-headers: drop VHOST_SET_NWORKERS Alexander Ivanov
@ 2026-10-27 12:50   ` no-reply
  2026-10-27 14:22   ` Andrey Drobyshev
  1 sibling, 0 replies; 9+ messages in thread
From: no-reply @ 2026-10-27 12:50 UTC (permalink / raw)
  To: Alexander Ivanov, andrey.zhadchenko; +Cc: svt-core, andrey.drobyshev

> vhost-blk no longer uses it, and the kernel drops the ioctl.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-107537
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261027124759.2979043-1-alexander.ivanov@virtuozzo.com?part=2


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH hci-8.1 v2 1/2] vhost-blk: create kernel threads with the mainstream worker ioctls
  2026-10-27 12:47 ` [PATCH hci-8.1 v2 1/2] vhost-blk: create kernel threads with " Alexander Ivanov
@ 2026-10-27 12:52   ` no-reply
  2026-10-27 13:05     ` Alexander Ivanov
  2026-10-27 14:22   ` Andrey Drobyshev
  1 sibling, 1 reply; 9+ messages in thread
From: no-reply @ 2026-10-27 12:52 UTC (permalink / raw)
  To: andrey.zhadchenko, Alexander Ivanov; +Cc: andrey.drobyshev, svt-core

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] vhost-blk: num-threads=0 silently accepted as one thread, no validation
--

Commit b390c3229288ae4b409cac07cdeba78f79d9b1e7
Author: Alexander Ivanov <alexander.ivanov@virtuozzo.com>

vhost-blk: create kernel threads with the mainstream worker ioctls

This patch replaces the VHOST_SET_NWORKERS ioctl in vhost_blk_start() with
VHOST_SET_FORK_FROM_OWNER, VHOST_NEW_WORKER and VHOST_ATTACH_VRING_WORKER.
It creates num-threads - 1 extra workers and attaches them to the virtqueues
round-robin. If the kernel lacks these ioctls, it warns and runs a single
thread.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-107537

> diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
> --- a/hw/block/vhost-blk.c
> +++ b/hw/block/vhost-blk.c

[ ... ]

> @@ -51,15 +97,37 @@ static int vhost_blk_start(VirtIODevice *vdev)
>          return 0;
>      }
>  
> +    nworkers = MIN(s->conf.num_threads, s->dev.nvqs);
> +    if (nworkers > 1) {

[Severity: Low]
This isn't a crash, but the new code changes how num-threads=0 and values
above the queue count are handled. Is that intended?

Before this patch, vhost_blk_start() passed nworkers to VHOST_SET_NWORKERS
whenever it was not 1. A value of 0 was therefore passed to the kernel,
and as far as I can tell start failed with "unable to set number of kernel
threads". I could not check how the kernel handled 0.

Now nworkers = MIN(num_threads, nvqs) evaluates to 0 for num-threads=0, so
"nworkers > 1" is false. The device starts silently with a single kernel
thread, as if num-threads=1 had been given.

The same happens when num-threads is larger than the number of queues. The
count is clamped to nvqs and no diagnostic is printed.

The num-threads property is DEFINE_PROP_UINT16 with a default of 1, and
vhost_blk_device_realize() only validates num_queues. Nothing checks
num_threads there either.

Should realize reject num-threads=0, and should it warn or fail when
num-threads exceeds num-queues?

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261027124759.2979043-1-alexander.ivanov@virtuozzo.com?part=1

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH hci-8.1 v2 1/2] vhost-blk: create kernel threads with the mainstream worker ioctls
  2026-10-27 12:52   ` no-reply
@ 2026-10-27 13:05     ` Alexander Ivanov
  0 siblings, 0 replies; 9+ messages in thread
From: Alexander Ivanov @ 2026-10-27 13:05 UTC (permalink / raw)
  To: no-reply, andrey.zhadchenko; +Cc: andrey.drobyshev, svt-core

>> @@ -51,15 +97,37 @@ static int vhost_blk_start(VirtIODevice *vdev)
>>           return 0;
>>       }
>>   
>> +    nworkers = MIN(s->conf.num_threads, s->dev.nvqs);
>> +    if (nworkers > 1) {
> [Severity: Low]
> This isn't a crash, but the new code changes how num-threads=0 and values
> above the queue count are handled. Is that intended?
>
> Before this patch, vhost_blk_start() passed nworkers to VHOST_SET_NWORKERS
> whenever it was not 1. A value of 0 was therefore passed to the kernel,
> and as far as I can tell start failed with "unable to set number of kernel
> threads". I could not check how the kernel handled 0.
>
> Now nworkers = MIN(num_threads, nvqs) evaluates to 0 for num-threads=0, so
> "nworkers > 1" is false. The device starts silently with a single kernel
> thread, as if num-threads=1 had been given.
>
> The same happens when num-threads is larger than the number of queues. The
> count is clamped to nvqs and no diagnostic is printed.
>
> The num-threads property is DEFINE_PROP_UINT16 with a default of 1, and
> vhost_blk_device_realize() only validates num_queues. Nothing checks
> num_threads there either.
>
> Should realize reject num-threads=0, and should it warn or fail when
> num-threads exceeds num-queues?
This matches the old behaviour. The kernel's VHOST_SET_NWORKERS handler
clamped n to nvqs and, for n=0, created no extra workers and returned
success, so num-threads=0 already ran a single thread and values above
num-queues were already clamped silently. The patch keeps those semantics.
Rejecting num-threads=0 in realize would break configurations that work
today, so I'd leave validation out of this series.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH hci-8.1 v2 1/2] vhost-blk: create kernel threads with the mainstream worker ioctls
  2026-10-27 12:47 ` [PATCH hci-8.1 v2 1/2] vhost-blk: create kernel threads with " Alexander Ivanov
  2026-10-27 12:52   ` no-reply
@ 2026-10-27 14:22   ` Andrey Drobyshev
  2026-10-28 11:48     ` Alexander Ivanov
  1 sibling, 1 reply; 9+ messages in thread
From: Andrey Drobyshev @ 2026-10-27 14:22 UTC (permalink / raw)
  To: Alexander Ivanov, andrey.zhadchenko; +Cc: svt-core

On 10/27/26 2:47 PM, Alexander Ivanov wrote:
> num-threads was passed to our own VHOST_SET_NWORKERS kernel ioctl.
> The kernel is dropping it in favour of the mainstream worker ioctls,
> which vhost-blk now supports, so use those: create num-threads - 1
> workers with VHOST_NEW_WORKER and attach them to the virtqueues
> round-robin with VHOST_ATTACH_VRING_WORKER, the default worker being
> the first one. This is the layout VHOST_SET_NWORKERS produced.
> 
> VHOST_NEW_WORKER works only in vhost_task mode, while our kernel,
> unlike mainline, defaults to kthreads, so switch the device to
> vhost_task mode with VHOST_SET_FORK_FROM_OWNER before VHOST_SET_OWNER.
> VHOST_RESET_OWNER restores the default, hence this is done on every
> start. With num-threads=1 nothing changes: no worker ioctls, kthread
> mode.
> 
> On a kernel without these ioctls warn and run a single thread rather
> than fail to start vhost-blk.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-107537
> 
> Signed-off-by: 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);
>   }

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().

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.

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)).

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".

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.

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) {
>       ...

Andrey

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH hci-8.1 v2 2/2] linux-headers: drop VHOST_SET_NWORKERS
  2026-10-27 12:47 ` [PATCH hci-8.1 v2 2/2] linux-headers: drop VHOST_SET_NWORKERS Alexander Ivanov
  2026-10-27 12:50   ` no-reply
@ 2026-10-27 14:22   ` Andrey Drobyshev
  1 sibling, 0 replies; 9+ messages in thread
From: Andrey Drobyshev @ 2026-10-27 14:22 UTC (permalink / raw)
  To: Alexander Ivanov, andrey.zhadchenko; +Cc: svt-core

On 10/27/26 2:47 PM, Alexander Ivanov wrote:
> vhost-blk no longer uses it, and the kernel drops the ioctl.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-107537
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>  [...]
Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH hci-8.1 v2 1/2] vhost-blk: create kernel threads with the mainstream worker ioctls
  2026-10-27 14:22   ` Andrey Drobyshev
@ 2026-10-28 11:48     ` Alexander Ivanov
  0 siblings, 0 replies; 9+ messages in thread
From: Alexander Ivanov @ 2026-10-28 11:48 UTC (permalink / raw)
  To: Andrey Drobyshev, andrey.zhadchenko; +Cc: svt-core

> 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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-10-28 11:48 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-27 12:47 [PATCH hci-8.1 v2 0/2] vhost-blk: switch from VHOST_SET_NWORKERS to the mainstream worker ioctls Alexander Ivanov
2026-10-27 12:47 ` [PATCH hci-8.1 v2 1/2] vhost-blk: create kernel threads with " Alexander Ivanov
2026-10-27 12:52   ` no-reply
2026-10-27 13:05     ` Alexander Ivanov
2026-10-27 14:22   ` Andrey Drobyshev
2026-10-28 11:48     ` Alexander Ivanov
2026-10-27 12:47 ` [PATCH hci-8.1 v2 2/2] linux-headers: drop VHOST_SET_NWORKERS Alexander Ivanov
2026-10-27 12:50   ` no-reply
2026-10-27 14:22   ` Andrey Drobyshev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox