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

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

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