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.
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
