vhost: simplify use of eventfds

PATCH In review Eric Biggers 2 participants 5 messages Jan 6, 2018 – Jan 9, 2018

Subsystems (3)

VFS KVM virtio host (vhost)

lore

Share & tools

lore

Eric Biggers

author

2018-01-06 22:58 UTC #0 lore raw

cover vhost: simplify use of eventfds

This series updates some of the vhost ioctls to grab a reference to just the eventfd_ctx instead of to both the eventfd_ctx and eventfd file, since only the eventfd_ctx is needed. This simplifies the code and eliminates all but one user of eventfd_fget().

Eric Biggers (3):
vhost: don't hold onto file pointer for VHOST_SET_VRING_CALL
vhost: don't hold onto file pointer for VHOST_SET_VRING_ERR
vhost: don't hold onto file pointer for VHOST_SET_LOG_FD

 drivers/vhost/vhost.c | 62 ++++++++++++---------------------------------------
 drivers/vhost/vhost.h |  3 ---
 2 files changed, 14 insertions(+), 51 deletions(-)

Signature · 1 line

2.15.1

Eric Biggers

author

2018-01-06 22:58 UTC #1 lore raw

PATCH 1/3 vhost: don't hold onto file pointer for VHOST_SET_VRING_CALL

From: Eric Biggers <[email protected]>

We already hold a reference to the eventfd_ctx, which is sufficient; there's no need to hold a reference to the struct file as well. So get rid of vhost_virtqueue->call.

Signed-off-by Eric Biggers <[email protected]>

---
 drivers/vhost/vhost.c | 20 +++++---------------
 drivers/vhost/vhost.h |  1 -
 2 files changed, 5 insertions(+), 16 deletions(-)

drivers/vhost/vhost.c +5 ~~-15~~

index 33ac2b186b85..ba589e87372e 100644
@@ -322,7 +322,6 @@ static void vhost_vq_reset(struct vhost_dev *dev,
 	vq->error = NULL;
 	vq->kick = NULL;
 	vq->call_ctx = NULL;
-	vq->call = NULL;
 	vq->log_ctx = NULL;
 	vhost_reset_is_le(vq);
 	vhost_disable_cross_endian(vq);

@@ -625,8 +624,6 @@ void vhost_dev_cleanup(struct vhost_dev *dev, bool locked)
 			fput(dev->vqs[i]->kick);
 		if (dev->vqs[i]->call_ctx)
 			eventfd_ctx_put(dev->vqs[i]->call_ctx);
-		if (dev->vqs[i]->call)
-			fput(dev->vqs[i]->call);
 		vhost_vq_reset(dev, dev->vqs[i]);
 	}
 	vhost_dev_free_iovecs(dev);

@@ -1488,19 +1485,12 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
 			r = -EFAULT;
 			break;
 		}
-		eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
-		if (IS_ERR(eventfp)) {
-			r = PTR_ERR(eventfp);
+		ctx = f.fd == -1 ? NULL : eventfd_ctx_fdget(f.fd);
+		if (IS_ERR(ctx)) {
+			r = PTR_ERR(ctx);
 			break;
 		}
-		if (eventfp != vq->call) {
-			filep = vq->call;
-			ctx = vq->call_ctx;
-			vq->call = eventfp;
-			vq->call_ctx = eventfp ?
-				eventfd_ctx_fileget(eventfp) : NULL;
-		} else
-			filep = eventfp;
+		swap(ctx, vq->call_ctx);
 		break;
 	case VHOST_SET_VRING_ERR:
 		if (copy_from_user(&f, argp, sizeof f)) {

@@ -1547,7 +1537,7 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
 	if (pollstop && vq->handle_kick)
 		vhost_poll_stop(&vq->poll);
 
-	if (ctx)
+	if (!IS_ERR_OR_NULL(ctx))
 		eventfd_ctx_put(ctx);
 	if (filep)
 		fput(filep);

drivers/vhost/vhost.h +0 -1

index 79c6e7a60a5e..41026c7d6842 100644
@@ -96,7 +96,6 @@ struct vhost_virtqueue {
 	struct vring_used __user *used;
 	const struct vhost_umem_node *meta_iotlb[VHOST_NUM_ADDRS];
 	struct file *kick;
-	struct file *call;
 	struct file *error;
 	struct eventfd_ctx *call_ctx;
 	struct eventfd_ctx *error_ctx;

Signature · 1 line

2.15.1

Eric Biggers

author

2018-01-06 22:58 UTC #2 lore raw

PATCH 2/3 vhost: don't hold onto file pointer for VHOST_SET_VRING_ERR

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

We already hold a reference to the eventfd_ctx, which is sufficient; there's no need to hold a reference to the struct file as well. So get rid of vhost_virtqueue->error.

Signed-off-by Eric Biggers <[email protected]>

---
 drivers/vhost/vhost.c | 18 ++++--------------
 drivers/vhost/vhost.h |  1 -
 2 files changed, 4 insertions(+), 15 deletions(-)

drivers/vhost/vhost.c +4 ~~-14~~

index ba589e87372e..39e184b168ce 100644
@@ -319,7 +319,6 @@ static void vhost_vq_reset(struct vhost_dev *dev,
 	vq->acked_features = 0;
 	vq->log_base = NULL;
 	vq->error_ctx = NULL;
-	vq->error = NULL;
 	vq->kick = NULL;
 	vq->call_ctx = NULL;
 	vq->log_ctx = NULL;

@@ -618,8 +617,6 @@ void vhost_dev_cleanup(struct vhost_dev *dev, bool locked)
 	for (i = 0; i < dev->nvqs; ++i) {
 		if (dev->vqs[i]->error_ctx)
 			eventfd_ctx_put(dev->vqs[i]->error_ctx);
-		if (dev->vqs[i]->error)
-			fput(dev->vqs[i]->error);
 		if (dev->vqs[i]->kick)
 			fput(dev->vqs[i]->kick);
 		if (dev->vqs[i]->call_ctx)

@@ -1497,19 +1494,12 @@ long vhost_vring_ioctl(struct vhost_dev *d, int ioctl, void __user *argp)
 			r = -EFAULT;
 			break;
 		}
-		eventfp = f.fd == -1 ? NULL : eventfd_fget(f.fd);
-		if (IS_ERR(eventfp)) {
-			r = PTR_ERR(eventfp);
+		ctx = f.fd == -1 ? NULL : eventfd_ctx_fdget(f.fd);
+		if (IS_ERR(ctx)) {
+			r = PTR_ERR(ctx);
 			break;
 		}
-		if (eventfp != vq->error) {
-			filep = vq->error;
-			vq->error = eventfp;
-			ctx = vq->error_ctx;
-			vq->error_ctx = eventfp ?
-				eventfd_ctx_fileget(eventfp) : NULL;
-		} else
-			filep = eventfp;
+		swap(ctx, vq->error_ctx);
 		break;
 	case VHOST_SET_VRING_ENDIAN:
 		r = vhost_set_vring_endian(vq, argp);

drivers/vhost/vhost.h +0 -1

index 41026c7d6842..3fe82323f667 100644
@@ -96,7 +96,6 @@ struct vhost_virtqueue {
 	struct vring_used __user *used;
 	const struct vhost_umem_node *meta_iotlb[VHOST_NUM_ADDRS];
 	struct file *kick;
-	struct file *error;
 	struct eventfd_ctx *call_ctx;
 	struct eventfd_ctx *error_ctx;
 	struct eventfd_ctx *log_ctx;

Signature · 1 line

2.15.1

Eric Biggers

author

2018-01-06 22:52 UTC #3 lore raw

PATCH 3/3 vhost: don't hold onto file pointer for VHOST_SET_LOG_FD

In reply to Eric Biggers

From: Eric Biggers <[email protected]>

We already hold a reference to the eventfd_ctx, which is sufficient; there's no need to hold a reference to the struct file as well. So get rid of vhost_dev->log_file.

Signed-off-by Eric Biggers <[email protected]>

---
 drivers/vhost/vhost.c | 24 +++++-------------------
 drivers/vhost/vhost.h |  1 -
 2 files changed, 5 insertions(+), 20 deletions(-)

drivers/vhost/vhost.c +5 ~~-19~~

index 39e184b168ce..67ea87a4a526 100644
@@ -420,7 +420,6 @@ void vhost_dev_init(struct vhost_dev *dev,
 	dev->nvqs = nvqs;
 	mutex_init(&dev->mutex);
 	dev->log_ctx = NULL;
-	dev->log_file = NULL;
 	dev->umem = NULL;
 	dev->iotlb = NULL;
 	dev->mm = NULL;

@@ -627,9 +626,6 @@ void vhost_dev_cleanup(struct vhost_dev *dev, bool locked)
 	if (dev->log_ctx)
 		eventfd_ctx_put(dev->log_ctx);
 	dev->log_ctx = NULL;
-	if (dev->log_file)
-		fput(dev->log_file);
-	dev->log_file = NULL;
 	/* No one will access memory at this point */
 	vhost_umem_clean(dev->umem);
 	dev->umem = NULL;

@@ -1570,8 +1566,7 @@ EXPORT_SYMBOL_GPL(vhost_init_device_iotlb);
 /* Caller must have device mutex */
 long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
 {
-	struct file *eventfp, *filep = NULL;
-	struct eventfd_ctx *ctx = NULL;
+	struct eventfd_ctx *ctx;
 	u64 p;
 	long r;
 	int i, fd;

@@ -1617,19 +1612,12 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
 		r = get_user(fd, (int __user *)argp);
 		if (r < 0)
 			break;
-		eventfp = fd == -1 ? NULL : eventfd_fget(fd);
-		if (IS_ERR(eventfp)) {
-			r = PTR_ERR(eventfp);
+		ctx = fd == -1 ? NULL : eventfd_ctx_fdget(fd);
+		if (IS_ERR(ctx)) {
+			r = PTR_ERR(ctx);
 			break;
 		}
-		if (eventfp != d->log_file) {
-			filep = d->log_file;
-			d->log_file = eventfp;
-			ctx = d->log_ctx;
-			d->log_ctx = eventfp ?
-				eventfd_ctx_fileget(eventfp) : NULL;
-		} else
-			filep = eventfp;
+		swap(ctx, d->log_ctx);
 		for (i = 0; i < d->nvqs; ++i) {
 			mutex_lock(&d->vqs[i]->mutex);
 			d->vqs[i]->log_ctx = d->log_ctx;

@@ -1637,8 +1625,6 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
 		}
 		if (ctx)
 			eventfd_ctx_put(ctx);
-		if (filep)
-			fput(filep);
 		break;
 	default:
 		r = -ENOIOCTLCMD;

drivers/vhost/vhost.h +0 -1

index 3fe82323f667..da86623871d9 100644
@@ -161,7 +161,6 @@ struct vhost_dev {
 	struct mutex mutex;
 	struct vhost_virtqueue **vqs;
 	int nvqs;
-	struct file *log_file;
 	struct eventfd_ctx *log_ctx;
 	struct llist_head work_list;
 	struct task_struct *worker;

Signature · 1 line

2.15.1

Jason Wang

Reviewed-by on the v1 series

2018-01-09 03:20 UTC #4 lore raw

In reply to Eric Biggers

Quoted 5 lines · Eric Biggers

This series updates some of the vhost ioctls to grab a reference to just the eventfd_ctx instead of to both the eventfd_ctx and eventfd file, since only the eventfd_ctx is needed. This simplifies the code and eliminates all but one user of eventfd_fget().

Quoted 9 lines

Eric Biggers (3):
vhost: don't hold onto file pointer for VHOST_SET_VRING_CALL
vhost: don't hold onto file pointer for VHOST_SET_VRING_ERR
vhost: don't hold onto file pointer for VHOST_SET_LOG_FD
  drivers/vhost/vhost.c | 62 ++++++++++++---------------------------------------
  drivers/vhost/vhost.h |  3 ---
  2 files changed, 14 insertions(+), 51 deletions(-)

Reviewed-by Jason Wang <[email protected]>