Jason Wang
2023-Apr-12 07:56 UTC
[PATCH v6 11/11] vhost: allow userspace to create workers
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>
Mike Christie
2023-Apr-13 22:36 UTC
[PATCH v6 11/11] vhost: allow userspace to create workers
On 4/12/23 2:56 AM, Jason Wang wrote:>> 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?Yeah, it would be useful. Just 2 questions. You mean an ioctl command to get this info right? I had added one in one version a while back, but dropped it because for some reason I thought we were going to do a generic vhost sysfs type of interface. We are not doing sysfs right?
Maybe Matching Threads
- [PATCH v6 11/11] vhost: allow userspace to create workers
- [PATCH v6 11/11] vhost: allow userspace to create workers
- [PATCH v6 11/11] vhost: allow userspace to create workers
- [PATCH v6 11/11] vhost: allow userspace to create workers
- [PATCH v6 00/11] vhost: multiple worker support