Re: [PATCH v2 2/2] vhost-scsi: Add support for a worker thread per virtqueue

[Mike Christie <michael.christie@oracle.com>]

On 11/29/23 3:30 AM, Stefano Garzarella wrote:
> On Sun, Nov 26, 2023 at 06:28:34PM -0600, Mike Christie wrote:
>> This adds support for vhost-scsi to be able to create a worker thread
>> per virtqueue. Right now for vhost-net we get a worker thread per
>> tx/rx virtqueue pair which scales nicely as we add more virtqueues and
>> CPUs, but for scsi we get the single worker thread that's shared by all
>> virtqueues. When trying to send IO to more than 2 virtqueues the single
>> thread becomes a bottlneck.
>>
>> This patch adds a new setting, workers_per_virtqueue, which can be set
>> to:
>>
>> false: Existing behavior where we get the single worker thread.
>> true: Create a worker per IO virtqueue.
>>
>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>> ---
>> hw/scsi/vhost-scsi.c            | 60 +++++++++++++++++++++++++++++++++
>> include/hw/virtio/virtio-scsi.h |  1 +
>> 2 files changed, 61 insertions(+)
>>
>> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
>> index 3126df9e1d9d..77eef9474c23 100644
>> --- a/hw/scsi/vhost-scsi.c
>> +++ b/hw/scsi/vhost-scsi.c
>> @@ -165,6 +165,57 @@ static const VMStateDescription vmstate_virtio_vhost_scsi = {
>>     .pre_save = vhost_scsi_pre_save,
>> };
>>
>> +static int vhost_scsi_set_workers(VHostSCSICommon *vsc, bool per_virtqueue)
>> +{
>> +    struct vhost_dev *dev = &vsc->dev;
>> +    struct vhost_vring_worker vq_worker;
>> +    struct vhost_worker_state worker;
>> +    int i, ret;
>> +
>> +    /* Use default worker */
>> +    if (!per_virtqueue || dev->nvqs == VHOST_SCSI_VQ_NUM_FIXED + 1) {
>> +        return 0;
>> +    }
>> +
>> +    /*
>> +     * ctl/evt share the first worker since it will be rare for them
>> +     * to send cmds while IO is running.
>> +     */
>> +    for (i = VHOST_SCSI_VQ_NUM_FIXED + 1; i < dev->nvqs; i++) {
>> +        memset(&worker, 0, sizeof(worker));
>> +
>> +        ret = dev->vhost_ops->vhost_new_worker(dev, &worker);
>> +        if (ret == -ENOTTY) {
>> +            /*
>> +             * worker ioctls are not implemented so just ignore and
>> +             * and continue device setup.
>> +             */
> 
> IIUC here the user has asked to use a worker for each virtqueue, but the
> kernel does not support it so we ignore it.
> 
> Should we at least print a warning?
> 

We should. I'll add it.