On 5/4/21 10:56 AM, Stefano Garzarella wrote: > Hi Mike, > > On Wed, Apr 28, 2021 at 05:36:59PM -0500, Mike Christie wrote: >> The following patches apply over mst's vhost branch and were tested >> againt that branch and also mkp's 5.13 branch which has some vhost-scsi >> changes. >> >> 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. >> >> The kernel patches here allow us to then do N workers device and also >> share workers across devices. > > I took a first look and left a few comments. > > In general it looks good to me, I'm just worried if it's okay to use the > kthread pid directly or it's better to use an internal id. > > For example I think if this can be affected by the pid namespaces or some > security issues. > I admit I don't know much about this topic, so this might be silly. >
Ah yeah, that was my other TODO. I did the lazy loop and left the "hash on pid" TODO in patch 11 because I was not sure. I thought it was ok, because only the userspace process that does the VHOST_SET_OWNER call can do the other ioctls. I can do pid or use a xarray/ida/idr type if struct and pass that id between user/kernel space if it's preferred. >> >> I included a patch for qemu so you can get an idea of how it works. >> >> TODO: >> ----- >> - polling >> - Allow sharing workers across devices. Kernel support is added and I >> hacked up userspace to test, but I'm still working on a sane way to >> manage it in userspace. >> - Bind to specific CPUs. Commands like "virsh emulatorpin" work with >> these patches and allow us to set the group of vhost threads to different >> CPUs. But we can also set a specific vq's worker to run on a CPU. >> - I'm handling old kernel by just checking for EPERM. Does this require >> a feature? > > Do you mean when the new ioctls are not available? We do not return > ENOIOCTLCMD? vhost_vring_ioctl returns -ENOIOCTLCMD but that does not get propagated to the app. Check out the comment in include/linux/errno.h and fs/ioctl.c:vfs_ioctl() where -ENOIOCTLCMD is caught and -ENOTTY is returned. To make this reply a little complicated note that at the time when I wrote the code I thought something was translating -ENOTTY to -EPERM but then after posting I realized that ioctl() always returns -1 on error (I thought the -1 was a -EPERM from the kernel). errno is set to -ENOTTY as expected when a ioctl is not implemented, so the -EPERM references should be errno and -ENOTTY. _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
