Linux Virtualization - Apr 2023 - [PATCH v6 11/11] vhost: allow userspace to create workers

On 4/10/23 10:00 PM, Jason Wang wrote:
>>> vhost_zerocopy_callback(). But since you want to limit the call
before
>>> set_backend, another question comes, do we really need the dynamic
>>> attaching/creating in this case? How about a simple one ioctl that
is
>>> used to build the queue to workers mapping instead?
>>
>>
>> I didn't think we need the dynamic case. It was from a review
comment.
> 
> Right, so we actually don't need three new ioctls but only a single is
> sufficient?
> 
> struct vhost_worker_state {
>       __u16 workers;
>       __u16 queue_to_work_map[];
> };
> 
> And limiting this to be called before datapath can run is sufficient?
> (sorry I missed some of the previous comments).
It's been like 3 years since this was last discussed so no problem :)

Yeah, what you describe is all I need. Originally I just had the one
ioctl:

+#define VHOST_SET_VRING_WORKER _IOWR(VHOST_VIRTIO, 0x15, struct
vhost_vring_worker)

The VHOST_SET_VRING_WORKER created a worker on the virtqueue in the
vhost_vring_worker.

>>>>
>>>>>> -       default:
>>>>>> +       case VHOST_ATTACH_VRING_WORKER:
>>>>>> +               if (copy_from_user(&w, argp,
sizeof(w))) {
>>>>>> +                       r = -EFAULT;
>>>>>> +                       break;
>>>>>> +               }
>>>>>> +               r = vhost_vq_attach_worker(vq, &w);
>>>>>> +               if (!r && copy_to_user(argp,
&w, sizeof(w)))
>>>>>> +                       r = -EFAULT;
>>>>>> +               break;
>>>>>
>>>>> It's a little odd that we have new and attach but only
a free.
>>>>
>>>> Do you mean we would also want a detach? I've been debating
that.
>>>> I'm not sure what the user wanted though.
>>>>
>>>> Would it leave the virtqueue with no worker? Or, does it pick
the first
>>>> worker it finds? If there is no worker because we did the
former or
>>>> because userspace detached all of them, then do we just drop
work that
>>>> gets queued?
>>>>
>>>> It seemed like a user would never want to drop work, so I made
it so
>>>> you can only tell it to use new workers (attach which I guess
is more
>>>> like a swap worker)
>>>
>>> swap and free old worker indeed?
>>
>> I didn't understand the question.
> 
> I mean if I understand the code correctly, the code tries to drop
> refcnt and free the old worker during attach.
I see. We actually don't free until VHOST_FREE_WORKER.

When we create the worker from VHOST_NEW_WORKER we set the refcount
to 1. Then each time a virtqueue and worker are attached to each other
we increase the refcount.

When you do vhost_vq_detach_worker then it drops the refcount from the
attach. Then if you detached the worker from all the virtqueues you
still have the refcount=1 from the VHOST_NEW_WORKER call.

The refcount decrement in VHOST_FREE_WORKER via vhost_worker_put would
be the final put. Note that if userspace just closes the dev without
doing a put, then we do the final puts for it.

Note that I originally didn't have the free. I don't need it for any
specific reason. It was just from a review comment. I originally just had
the one create worker type of ioctl. Then it was suggested to do the split
attach/new/free design.

I can spin another patchset with the single ioctl design so we can compare.

On Wed, Apr 12, 2023 at 6:15?AM Mike Christie
<michael.christie at oracle.com> wrote:
>
> On 4/10/23 10:00 PM, Jason Wang wrote:
> >>> vhost_zerocopy_callback(). But since you want to limit the
call before
> >>> set_backend, another question comes, do we really need the
dynamic
> >>> attaching/creating in this case? How about a simple one ioctl
that is
> >>> used to build the queue to workers mapping instead?
> >>
> >>
> >> I didn't think we need the dynamic case. It was from a review
comment.
> >
> > Right, so we actually don't need three new ioctls but only a
single is
> > sufficient?
> >
> > struct vhost_worker_state {
> >       __u16 workers;
> >       __u16 queue_to_work_map[];
> > };
> >
> > And limiting this to be called before datapath can run is sufficient?
> > (sorry I missed some of the previous comments).
>
> It's been like 3 years since this was last discussed so no problem :)
>
I'm really sorry for that, -ENOMEM :(
> Yeah, what you describe is all I need. Originally I just had the one
> ioctl:
>
> +#define VHOST_SET_VRING_WORKER _IOWR(VHOST_VIRTIO, 0x15, struct
vhost_vring_worker)
>
> The VHOST_SET_VRING_WORKER created a worker on the virtqueue in the
> vhost_vring_worker.
>
>
> >>>>
> >>>>>> -       default:
> >>>>>> +       case VHOST_ATTACH_VRING_WORKER:
> >>>>>> +               if (copy_from_user(&w, argp,
sizeof(w))) {
> >>>>>> +                       r = -EFAULT;
> >>>>>> +                       break;
> >>>>>> +               }
> >>>>>> +               r = vhost_vq_attach_worker(vq,
&w);
> >>>>>> +               if (!r &&
copy_to_user(argp, &w, sizeof(w)))
> >>>>>> +                       r = -EFAULT;
> >>>>>> +               break;
> >>>>>
> >>>>> It's a little odd that we have new and attach but
only a free.
> >>>>
> >>>> Do you mean we would also want a detach? I've been
debating that.
> >>>> I'm not sure what the user wanted though.
> >>>>
> >>>> Would it leave the virtqueue with no worker? Or, does it
pick the first
> >>>> worker it finds? If there is no worker because we did the
former or
> >>>> because userspace detached all of them, then do we just
drop work that
> >>>> gets queued?
> >>>>
> >>>> It seemed like a user would never want to drop work, so I
made it so
> >>>> you can only tell it to use new workers (attach which I
guess is more
> >>>> like a swap worker)
> >>>
> >>> swap and free old worker indeed?
> >>
> >> I didn't understand the question.
> >
> > I mean if I understand the code correctly, the code tries to drop
> > refcnt and free the old worker during attach.
>
> I see. We actually don't free until VHOST_FREE_WORKER.
>
> When we create the worker from VHOST_NEW_WORKER we set the refcount
> to 1. Then each time a virtqueue and worker are attached to each other
> we increase the refcount.
>
> When you do vhost_vq_detach_worker then it drops the refcount from the
> attach. Then if you detached the worker from all the virtqueues you
> still have the refcount=1 from the VHOST_NEW_WORKER call.
>
> The refcount decrement in VHOST_FREE_WORKER via vhost_worker_put would
> be the final put. Note that if userspace just closes the dev without
> doing a put, then we do the final puts for it.
Right, I mis-read the code, you did

        /*
         * We can free the worker if it's not attached to any virtqueues.
         */
        if (refcount_read(&worker->refcount) != 1)
                return -EBUSY;

And I thought it was a dec and test.
>
> Note that I originally didn't have the free. I don't need it for
any
> specific reason. It was just from a review comment. I originally just had
> the one create worker type of ioctl. Then it was suggested to do the split
> attach/new/free design.
I see.
>
> I can spin another patchset with the single ioctl design so we can compare.
So I'm fine with this approach. One last question, I see this:

/* By default, a device gets one vhost_worker that its virtqueues share. This */

I'm wondering if it is better to have a vhost_get_worker() to return
the worker_id of a specific queue. In the future, this might be useful
for devices that have multiple workers by default?

Thanks
>