Re: [PATCH hci-8.1 v4 4/5] vhost-blk: create kernel threads with the mainstream worker ioctls

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

>> +static void vhost_blk_set_workers(struct vhost_dev *hdev, int nworkers)
>> +{
>> +    uint32_t ids[VHOST_BLK_MAX_QUEUES];
>> +    int i, ret, err = 0, nfailed = 0;
>> +
>> +    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;
>> +        }
>> +
>> +        if (i < nworkers) {
>> +            struct vhost_worker_state worker = { };
>> +
>> +            ret = hdev->vhost_ops->vhost_new_worker(hdev, &worker);
>> +            if (ret < 0) {
>> +                warn_report("vhost-blk: unable to create kernel thread: %s",
>> +                            strerror(-ret));
>> +                nworkers = i;
>> +                continue;
>> +            }
> [Severity: Low]
> When vhost_new_worker() fails for queue i, the code sets nworkers = i and
> does continue.  Doesn't that skip the attach step for queue i itself?
>
> Queue i then stays on the default thread, even though workers 1..i-1
> already exist and could take it.  Later queues are spread correctly,
> because w = i % nworkers uses the reduced nworkers.
>
> The commit message says "if VHOST_NEW_WORKER fails partway, spread the
> remaining virtqueues over the threads created so far".  Should the failing
> queue also be attached to one of the existing workers, for example by
> recomputing w and falling through to the attach step?
Queue i lands on thread 0, which is exactly where round-robin over
the reduced nworkers puts it (i % i == 0); attaching it would be a no-op.
>> @@ -52,15 +99,24 @@ static int vhost_blk_start(VirtIODevice *vdev)
>>           return 0;
>>       }
>>   
>> +    nworkers = MIN(s->conf.num_threads, hdev->nvqs);
>> +    if (nworkers > 1) {
> [Severity: Low]
> With -device vhost-blk,num-threads=0, nworkers = MIN(0, nvqs) = 0, so
> every "nworkers > 1" branch is skipped and no warning is printed.
>
> The old code did this:
>
>      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;
>      }
>
> So 0 was passed to the kernel, and the device failed to start if the ioctl
> rejected it.  I could not check the kernel handler.
>
> The num-threads property is a plain DEFINE_PROP_UINT16 with default 1, and
> I found no realize-time check that rejects 0.  Is silently running
> num-threads=0 on the single default thread intended?  The commit message
> only says that num-threads=1 is unchanged.
This is intended and unchanged: the old VHOST_SET_NWORKERS handler
accepted 0 without error (its worker-creation loop starts at 1, so all
virtqueues stayed on the default thread), and MIN(num_threads, nvqs) with
the nworkers > 1 checks keeps exactly that.
>> +    if (nworkers > 1) {
>> +        vhost_blk_set_workers(hdev, nworkers);
>>       }
>>   
>>       if (s->conf.serial) {
> [Severity: Low]
> Can the extra workers created by vhost_blk_set_workers() outlive a failed
> start?
>
> Once vhost_blk_set_workers() has run, later failures in vhost_blk_start()
> do not issue VHOST_RESET_OWNER or VHOST_FREE_WORKER.  These are the
> VHOST_BLK_SET_SERIAL failure, set_guest_notifiers, and the err_set,
> err_disable and err_stop paths.
>
> The only place that does the teardown is vhost_blk_reset():
>
>      ret = ioctl(s->vhostfd, VHOST_RESET_OWNER, NULL);
>
> So after a failed start with num-threads > 1 and at least 2 virtqueues, the
> owner and the extra worker threads seem to stay alive until a later reset
> or until the vhost fd is closed.
>
> The missing owner reset on these error paths was there before this patch.
> But this patch adds worker threads to what stays alive on them.  I could not
> verify how the vz kernel handles a retried VHOST_SET_OWNER.  Should the
> error paths release the workers?
The extra workers live exactly as long as the owner, which a failed 
start already
leaks, so adding VHOST_RESET_OWNER to the error paths is a separate fix.