On 5/4/21 10:30 AM, Stefano Garzarella wrote:
> On Wed, Apr 28, 2021 at 05:37:11PM -0500, Mike Christie wrote:
>> This patch allows userspace to create workers and bind them to vqs, so you
>> can have N workers per dev and also share N workers with M vqs. The next
>> patch will allow sharing across devices.
>>
>> Signed-off-by: Mike Christie <michael.chris...@oracle.com>
>> ---
>> drivers/vhost/vhost.c | 95 +++++++++++++++++++++++++++++++-
>> drivers/vhost/vhost.h | 3 +
>> include/uapi/linux/vhost.h | 6 ++
>> include/uapi/linux/vhost_types.h | 9 +++
>> 4 files changed, 111 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index 345ade0af133..fecdae0d18c7 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -42,6 +42,9 @@ module_param(max_iotlb_entries, int, 0444);
>> MODULE_PARM_DESC(max_iotlb_entries,
>> "Maximum number of iotlb entries. (default: 2048)");
>>
>> +static LIST_HEAD(vhost_workers_list);
>> +static DEFINE_SPINLOCK(vhost_workers_lock);
>> +
>> enum {
>> VHOST_MEMORY_F_LOG = 0x1,
>> };
>> @@ -617,8 +620,16 @@ static void vhost_detach_mm(struct vhost_dev *dev)
>> dev->mm = NULL;
>> }
>>
>> -static void vhost_worker_free(struct vhost_worker *worker)
>> +static void vhost_worker_put(struct vhost_worker *worker)
>> {
>> + spin_lock(&vhost_workers_lock);
>> + if (!refcount_dec_and_test(&worker->refcount)) {
>> + spin_unlock(&vhost_workers_lock);
>> + return;
>> + }
>> + list_del(&worker->list);
>> + spin_unlock(&vhost_workers_lock);
>> +
>> WARN_ON(!llist_empty(&worker->work_list));
>> kthread_stop(worker->task);
>> kfree(worker);
>> @@ -632,7 +643,7 @@ static void vhost_workers_free(struct vhost_dev *dev)
>> return;
>>
>> for (i = 0; i < dev->num_workers; i++)
>> - vhost_worker_free(dev->workers[i]);
>> + vhost_worker_put(dev->workers[i]);
>>
>> kfree(dev->workers);
>> dev->num_workers = 0;
>> @@ -652,6 +663,8 @@ static struct vhost_worker *vhost_worker_create(struct
>> vhost_dev *dev)
>> worker->id = dev->num_workers;
>> worker->dev = dev;
>> init_llist_head(&worker->work_list);
>> + INIT_LIST_HEAD(&worker->list);
>> + refcount_set(&worker->refcount, 1);
>>
>> task = kthread_create(vhost_worker, worker, "vhost-%d", current->pid);
>> if (IS_ERR(task))
>> @@ -664,6 +677,9 @@ static struct vhost_worker *vhost_worker_create(struct
>> vhost_dev *dev)
>> if (ret)
>> goto stop_worker;
>>
>> + spin_lock(&vhost_workers_lock);
>> + list_add_tail(&worker->list, &vhost_workers_list);
>> + spin_unlock(&vhost_workers_lock);
>> return worker;
>>
>> stop_worker:
>> @@ -673,6 +689,71 @@ static struct vhost_worker *vhost_worker_create(struct
>> vhost_dev *dev)
>> return NULL;
>> }
>>
>> +static struct vhost_worker *vhost_worker_find(struct vhost_dev *dev, pid_t
>> pid)
>> +{
>> + struct vhost_worker *worker;
>> +
>> + /* TODO hash on pid? */
>> + spin_lock(&vhost_workers_lock);
>> + list_for_each_entry(worker, &vhost_workers_list, list) {
>> + if (worker->task->pid != pid)
>> + continue;
>> +
>> + /* tmp - next patch allows sharing across devs */
>> + if (worker->dev != dev) {
>> + spin_unlock(&vhost_workers_lock);
>> + return NULL;
>> + }
>> +
>> + refcount_inc(&worker->refcount);
>> + spin_unlock(&vhost_workers_lock);
>> + return worker;
>> + }
>> + spin_unlock(&vhost_workers_lock);
>> + return NULL;
>
> I would like to have a single point where we release the lock to avoid
> future issues, how about changing vhost_worker_find() to:
>
> static struct vhost_worker *vhost_worker_find(struct vhost_dev *dev, pid_t
> pid)
> {
> struct vhost_worker *worker, *found_worker = NULL;
>
> spin_lock(&vhost_workers_lock);
> list_for_each_entry(worker, &vhost_workers_list, list) {
> if (worker->task->pid == pid) {
> /* tmp - next patch allows sharing across devs */
> if (worker->dev != dev)
> break;
>
> found_worker = worker;
> refcount_inc(&found_worker->refcount);
> break;
> }
> }
> spin_unlock(&vhost_workers_lock);
> return found_worker;
> }
Nice. Will do.
>
>> +}
>> +
>> +/* Caller must have device mutex */
>> +static int vhost_vq_set_worker(struct vhost_virtqueue *vq,
>> + struct vhost_vring_worker *info)
>> +{
>> + struct vhost_dev *dev = vq->dev;
>> + struct vhost_worker *worker;
>> +
>> + if (vq->worker) {
>> + /* TODO - support changing while works are running */
>> + return -EBUSY;
>> + }
>> +
>> + if (info->pid == -1) {
>> + worker = vhost_worker_create(dev);
>> + if (!worker)
>> + return -ENOMEM;
>> +
>> + info->pid = worker->task->pid;
>> + } else {
>> + worker = vhost_worker_find(dev, info->pid);
>> + if (!worker)
>> + return -ENODEV;
>> + }
>> +
>> + if (!dev->workers) {
>> + dev->workers = kcalloc(vq->dev->nvqs,
>> + sizeof(struct vhost_worker *),
>> + GFP_KERNEL);
>> + if (!dev->workers) {
>> + vhost_worker_put(worker);
>> + return -ENOMEM;
>> + }
>> + }
>> +
>> + vq->worker = worker;
>> +
>> + dev->workers[dev->num_workers] = worker;
>> + dev->num_workers++;
>> + return 0;
>> +}
>> +
>> /* Caller must have device mutex */
>> static int vhost_worker_try_create_def(struct vhost_dev *dev)
>> {
>> @@ -1680,6 +1761,7 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned
>> int ioctl, void __user *arg
>> struct eventfd_ctx *ctx = NULL;
>> u32 __user *idxp = argp;
>> struct vhost_virtqueue *vq;
>> + struct vhost_vring_worker w;
>> struct vhost_vring_state s;
>> struct vhost_vring_file f;
>> u32 idx;
>> @@ -1794,6 +1876,15 @@ long vhost_vring_ioctl(struct vhost_dev *d, unsigned
>> int ioctl, void __user *arg
>> if (copy_to_user(argp, &s, sizeof(s)))
>> r = -EFAULT;
>> break;
>> + case VHOST_SET_VRING_WORKER:
>> + if (copy_from_user(&w, argp, sizeof(w))) {
>> + r = -EFAULT;
>> + break;
>> + }
>> + r = vhost_vq_set_worker(vq, &w);
>> + if (!r && copy_to_user(argp, &w, sizeof(w)))
>> + r = -EFAULT;
>> + break;
>> default:
>> r = -ENOIOCTLCMD;
>> }
>> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
>> index 973889ec7d62..64dc00337389 100644
>> --- a/drivers/vhost/vhost.h
>> +++ b/drivers/vhost/vhost.h
>> @@ -14,6 +14,7 @@
>> #include <linux/atomic.h>
>> #include <linux/vhost_iotlb.h>
>> #include <linux/irqbypass.h>
>> +#include <linux/refcount.h>
>>
>> struct vhost_work;
>> typedef void (*vhost_work_fn_t)(struct vhost_work *work);
>> @@ -28,6 +29,8 @@ struct vhost_work {
>> struct vhost_worker {
>> struct task_struct *task;
>> struct llist_head work_list;
>> + struct list_head list;
>> + refcount_t refcount;
>> struct vhost_dev *dev;
>> int id;
>> };
>> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
>> index c998860d7bbc..61a57f5366ee 100644
>> --- a/include/uapi/linux/vhost.h
>> +++ b/include/uapi/linux/vhost.h
>> @@ -70,6 +70,12 @@
>> #define VHOST_VRING_BIG_ENDIAN 1
>> #define VHOST_SET_VRING_ENDIAN _IOW(VHOST_VIRTIO, 0x13, struct
>> vhost_vring_state)
>> #define VHOST_GET_VRING_ENDIAN _IOW(VHOST_VIRTIO, 0x14, struct
>> vhost_vring_state)
>> +/* Create/bind a vhost worker to a virtqueue. If pid > 0 and matches an
>> existing
>> + * vhost_worker thread it will be bound to the vq. If pid is -1, then a new
>
> What about adding a macro for -1? (e.g. VHOST_VRING_NEW_WORKER)
Yeah, that is nicer than a magic number. Will do.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
