Re: [PATCH] [v2] virtio_net: Remove BUG() to avoid machine dead
On Sat, Jun 05, 2021 at 11:31:00AM -0400, Xianting Tian wrote: > From: Xianting Tian > > We should not directly BUG() when there is hdr error, it is > better to output a print when such error happens. Currently, > the caller of xmit_skb() already did it. > > Signed-off-by: Xianting Tian > --- > drivers/net/virtio_net.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index 9b6a4a8..7f11ea4 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -1623,7 +1623,7 @@ static int xmit_skb(struct send_queue *sq, struct > sk_buff *skb) > if (virtio_net_hdr_from_skb(skb, >hdr, > virtio_is_little_endian(vi->vdev), false, > 0)) > - BUG(); > + return -EPROTO; Yeah, as we discussed, BUG*() macros in non-core code that checks in-kernel API better to be deleted. Thanks, Reviewed-by: Leon Romanovsky ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 7/9] vhost: allow userspace to create workers
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
Re: vhost: multiple worker support
On 6/3/21 5:16 PM, Mike Christie wrote: > On 6/3/21 9:37 AM, Stefan Hajnoczi wrote: >> On Tue, May 25, 2021 at 01:05:51PM -0500, Mike Christie wrote: >>> The following patches apply over linus's tree or mst's vhost branch >>> and my cleanup patchset: >>> >>> https://urldefense.com/v3/__https://lists.linuxfoundation.org/pipermail/virtualization/2021-May/054354.html__;!!GqivPVa7Brio!P55eslnMW_iZkoTUZckwhnSw_9Z35JBScgtSYRh4CMFTRkSsWwKOYqY0huUfBfIPM8BV$ >>> >>> >>> These patches allow us to support multiple vhost workers per device. I >>> ended up just doing Stefan's original idea where userspace has the >>> kernel create a worker and we pass back the pid. This has the benefit >>> over the workqueue and userspace thread approach where we only have >>> one'ish code path in the kernel during setup to detect old tools. The >>> main IO paths and device/vq setup/teardown paths all use common code. >>> >>> The kernel patches here allow us to then do N workers device and also >>> share workers across devices. >>> >>> I've also included a patch for qemu so you can get an idea of how it >>> works. If we are ok with the kernel code then I'll break that up into >>> a patchset and send to qemu-devel. >> >> It seems risky to allow userspace process A to "share" a vhost worker >> thread with userspace process B based on a matching pid alone. Should >> they have ptrace_may_access() or similar? >> > > I'm not sure. I already made it a little restrictive in this posting, but Made a mistake here. In this posting I did not make it restrictive and I was allowing any old 2 processes to share. So we would need something like ptrace_may_access if go this route. If we restrict sharing workers with the same owner, then I'm not sure if need anything. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: simplify gendisk and request_queue allocation for blk-mq based drivers
On Fri, Jun 04, 2021 at 11:58:34AM -0400, Konrad Rzeszutek Wilk wrote: > On Wed, Jun 02, 2021 at 09:53:15AM +0300, Christoph Hellwig wrote: > > Hi all, > > Hi! > > You wouldn't have a nice git repo to pull so one can test it easily? git://git.infradead.org/users/hch/block.git alloc_disk-part2 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
[PATCH] vdpa: fix error code in vp_vdpa_probe()
Return -ENOMEM if vp_modern_map_vq_notify() fails. Currently it
returns success.
Fixes: 11d8ffed00b2 ("vp_vdpa: switch to use vp_modern_map_vq_notify()")
Signed-off-by: Dan Carpenter
---
drivers/vdpa/virtio_pci/vp_vdpa.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/vdpa/virtio_pci/vp_vdpa.c
b/drivers/vdpa/virtio_pci/vp_vdpa.c
index c76ebb531212..e5d92db728d3 100644
--- a/drivers/vdpa/virtio_pci/vp_vdpa.c
+++ b/drivers/vdpa/virtio_pci/vp_vdpa.c
@@ -442,6 +442,7 @@ static int vp_vdpa_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
vp_modern_map_vq_notify(mdev, i,
_vdpa->vring[i].notify_pa);
if (!vp_vdpa->vring[i].notify) {
+ ret = -ENOMEM;
dev_warn(>dev, "Fail to map vq notify %d\n", i);
goto err;
}
--
2.30.2
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
