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.