From: michael.christie@oracle.com To: Stefan Hajnoczi <stefanha@redhat.com> Cc: target-devel@vger.kernel.org, linux-scsi@vger.kernel.org, pbonzini@redhat.com, jasowang@redhat.com, mst@redhat.com, sgarzare@redhat.com, virtualization@lists.linux-foundation.org Subject: Re: [PATCH 7/9] vhost: allow userspace to create workers Date: Sat, 5 Jun 2021 18:53:58 -0500 [thread overview] Message-ID: <0c1aef53-4850-8c46-0706-9b7276716e68@oracle.com> (raw) In-Reply-To: <YLjnk5GpFaCCOqCU@stefanha-x1.localdomain> On 6/3/21 9:30 AM, Stefan Hajnoczi wrote: >> + if (info->pid == VHOST_VRING_NEW_WORKER) { >> + worker = vhost_worker_create(dev); > > The maximum number of kthreads created is limited by > vhost_dev_init(nvqs)? For example VHOST_SCSI_MAX_VQ 128. > > IIUC kthread_create is not limited by or accounted against the current > task, so I'm a little worried that a process can create a lot of > kthreads. > > I haven't investigated other kthread_create() users reachable from > userspace applications to see how they bound the number of threads > effectively. Do we want something like io_uring's copy_process use? It's what fork uses, so we get checks like RLIMIT_NPROC and max_threads. I know I didn't look at everything, but it looks like for some software drivers we just allow the user to run wild. For example for nbd, when we create the device to do alloc_workqueue and use the default max_active value (256). We then don't have a limit on connections, so we could end up with 256 workqueue threads per device. And then there is no limit on devices a user can make. > > Any thoughts? > Is the concern a bad VM could create N devs each with 128 vqs/threads and it would slow down other VMs? How do we handle the case where some VM makes M * N devs and that is equal to N * 128 so we would end up with the same number of threads either way? Is there a limit to the number of vhost devices a VM can make and can I just stick in a similar check for workers? For vhost-scsi specifically, the 128 limit does not make a lot of sense. I think we want the max to be the number of vCPUs the VM has so we can add checks for that. Then we would assume someone making a VM with lots of CPUs is going to have the resources to support them. Note: It does make sense from the point of view that we don't know the number of vCPUs when vhost-scsi calls vhost_dev_init, so I get we had to select an initial limit. >> + if (!dev->workers) { >> + vhost_worker_put(worker); >> + return -ENOMEM; >> + } >> + } >> + >> + vq->worker = worker; >> + >> + dev->workers[dev->num_workers] = worker; >> + dev->num_workers++; > > Hmm...should we really append to workers[] in the vhost_worker_find() > case? As it's coded now, yes. Every successful vhost_worker_find call does a get on the worker's refcount. Later when we delete the device, we loop over the workers array and for every entry we do a put. I can add in some code to first check if the worker is already in the dev's worker list. If so then skip the refcount and skip adding to the workers array. If not in the dev's worker list then do a vhost_worker_find. I thought it might be nicer how it is now with the single path. It's less code at least. Later if we add support to change a vq's worker then we also don't have to worry about refcounts as much. We just always drop the count taken from when it was added.
WARNING: multiple messages have this Message-ID (diff)
From: michael.christie@oracle.com To: Stefan Hajnoczi <stefanha@redhat.com> Cc: linux-scsi@vger.kernel.org, mst@redhat.com, virtualization@lists.linux-foundation.org, target-devel@vger.kernel.org, pbonzini@redhat.com Subject: Re: [PATCH 7/9] vhost: allow userspace to create workers Date: Sat, 5 Jun 2021 18:53:58 -0500 [thread overview] Message-ID: <0c1aef53-4850-8c46-0706-9b7276716e68@oracle.com> (raw) In-Reply-To: <YLjnk5GpFaCCOqCU@stefanha-x1.localdomain> On 6/3/21 9:30 AM, Stefan Hajnoczi wrote: >> + if (info->pid == VHOST_VRING_NEW_WORKER) { >> + worker = vhost_worker_create(dev); > > The maximum number of kthreads created is limited by > vhost_dev_init(nvqs)? For example VHOST_SCSI_MAX_VQ 128. > > IIUC kthread_create is not limited by or accounted against the current > task, so I'm a little worried that a process can create a lot of > kthreads. > > I haven't investigated other kthread_create() users reachable from > userspace applications to see how they bound the number of threads > effectively. Do we want something like io_uring's copy_process use? It's what fork uses, so we get checks like RLIMIT_NPROC and max_threads. I know I didn't look at everything, but it looks like for some software drivers we just allow the user to run wild. For example for nbd, when we create the device to do alloc_workqueue and use the default max_active value (256). We then don't have a limit on connections, so we could end up with 256 workqueue threads per device. And then there is no limit on devices a user can make. > > Any thoughts? > Is the concern a bad VM could create N devs each with 128 vqs/threads and it would slow down other VMs? How do we handle the case where some VM makes M * N devs and that is equal to N * 128 so we would end up with the same number of threads either way? Is there a limit to the number of vhost devices a VM can make and can I just stick in a similar check for workers? For vhost-scsi specifically, the 128 limit does not make a lot of sense. I think we want the max to be the number of vCPUs the VM has so we can add checks for that. Then we would assume someone making a VM with lots of CPUs is going to have the resources to support them. Note: It does make sense from the point of view that we don't know the number of vCPUs when vhost-scsi calls vhost_dev_init, so I get we had to select an initial limit. >> + if (!dev->workers) { >> + vhost_worker_put(worker); >> + return -ENOMEM; >> + } >> + } >> + >> + vq->worker = worker; >> + >> + dev->workers[dev->num_workers] = worker; >> + dev->num_workers++; > > Hmm...should we really append to workers[] in the vhost_worker_find() > case? As it's coded now, yes. Every successful vhost_worker_find call does a get on the worker's refcount. Later when we delete the device, we loop over the workers array and for every entry we do a put. I can add in some code to first check if the worker is already in the dev's worker list. If so then skip the refcount and skip adding to the workers array. If not in the dev's worker list then do a vhost_worker_find. I thought it might be nicer how it is now with the single path. It's less code at least. Later if we add support to change a vq's worker then we also don't have to worry about refcounts as much. We just always drop the count taken from when it was added. _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2021-06-05 23:54 UTC|newest] Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-25 18:05 vhost: multiple worker support Mike Christie 2021-05-25 18:05 ` Mike Christie 2021-05-25 18:05 ` [PATCH 1/9] vhost: move worker thread fields to new struct Mike Christie 2021-05-25 18:05 ` Mike Christie 2021-06-03 10:16 ` Stefan Hajnoczi 2021-06-03 10:16 ` Stefan Hajnoczi 2021-05-25 18:05 ` [PATCH 2/9] vhost: move vhost worker creation to kick setup Mike Christie 2021-05-25 18:05 ` Mike Christie 2021-06-03 10:28 ` Stefan Hajnoczi 2021-06-03 10:28 ` Stefan Hajnoczi 2021-05-25 18:05 ` [PATCH 3/9] vhost: modify internal functions to take a vhost_worker Mike Christie 2021-05-25 18:05 ` Mike Christie 2021-06-03 10:45 ` Stefan Hajnoczi 2021-06-03 10:45 ` Stefan Hajnoczi 2021-05-25 18:05 ` [PATCH 4/9] vhost: allow vhost_polls to use different vhost_workers Mike Christie 2021-05-25 18:05 ` Mike Christie 2021-06-03 13:51 ` Stefan Hajnoczi 2021-06-03 13:51 ` Stefan Hajnoczi 2021-05-25 18:05 ` [PATCH 5/9] vhost-scsi: flush IO vqs then send TMF rsp Mike Christie 2021-05-25 18:05 ` Mike Christie 2021-06-03 13:54 ` Stefan Hajnoczi 2021-06-03 13:54 ` Stefan Hajnoczi 2021-05-25 18:05 ` [PATCH 6/9] vhost-scsi: make SCSI cmd completion per vq Mike Christie 2021-05-25 18:05 ` Mike Christie 2021-06-03 13:57 ` Stefan Hajnoczi 2021-06-03 13:57 ` Stefan Hajnoczi 2021-05-25 18:05 ` [PATCH 7/9] vhost: allow userspace to create workers Mike Christie 2021-05-25 18:05 ` Mike Christie 2021-06-03 14:30 ` Stefan Hajnoczi 2021-06-03 14:30 ` Stefan Hajnoczi 2021-06-05 23:53 ` michael.christie [this message] 2021-06-05 23:53 ` michael.christie 2021-06-07 15:19 ` Stefan Hajnoczi 2021-06-07 15:19 ` Stefan Hajnoczi 2021-06-09 21:03 ` Mike Christie 2021-06-09 21:03 ` Mike Christie 2021-06-10 8:06 ` Stefan Hajnoczi 2021-06-10 8:06 ` Stefan Hajnoczi 2021-06-18 2:49 ` Mike Christie 2021-06-18 2:49 ` Mike Christie 2021-06-21 13:41 ` Stefan Hajnoczi 2021-06-21 13:41 ` Stefan Hajnoczi 2021-05-25 18:05 ` [PATCH 8/9] vhost: add vhost_dev pointer to vhost_work Mike Christie 2021-05-25 18:05 ` Mike Christie 2021-06-03 14:31 ` Stefan Hajnoczi 2021-06-03 14:31 ` Stefan Hajnoczi 2021-05-25 18:06 ` [PATCH 9/9] vhost: support sharing workers across devs Mike Christie 2021-05-25 18:06 ` Mike Christie 2021-06-03 14:32 ` Stefan Hajnoczi 2021-06-03 14:32 ` Stefan Hajnoczi 2021-06-07 2:18 ` Jason Wang 2021-06-07 2:18 ` Jason Wang 2021-06-03 10:13 ` vhost: multiple worker support Stefan Hajnoczi 2021-06-03 10:13 ` Stefan Hajnoczi 2021-06-03 18:45 ` Mike Christie 2021-06-03 18:45 ` Mike Christie 2021-06-03 14:37 ` Stefan Hajnoczi 2021-06-03 14:37 ` Stefan Hajnoczi 2021-06-03 22:16 ` Mike Christie 2021-06-03 22:16 ` Mike Christie 2021-06-05 22:40 ` michael.christie 2021-06-05 22:40 ` michael.christie 2021-06-07 15:23 ` Stefan Hajnoczi 2021-06-07 15:23 ` Stefan Hajnoczi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=0c1aef53-4850-8c46-0706-9b7276716e68@oracle.com \ --to=michael.christie@oracle.com \ --cc=jasowang@redhat.com \ --cc=linux-scsi@vger.kernel.org \ --cc=mst@redhat.com \ --cc=pbonzini@redhat.com \ --cc=sgarzare@redhat.com \ --cc=stefanha@redhat.com \ --cc=target-devel@vger.kernel.org \ --cc=virtualization@lists.linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.