[PATCH V3] virtio-fs: Improved request latencies when Virtio queue is full

[PATCH V3] virtio-fs: Improved request latencies when Virtio queue is full

[Peter-Jan Gootzen via Virtualization]

When the Virtio queue is full, a work item is scheduled
to execute in 1ms that retries adding the request to the queue.
This is a large amount of time on the scale on which a
virtio-fs device can operate. When using a DPU this is around
40us baseline without going to a remote server (4k, QD=1).
This patch queues requests when the Virtio queue is full,
and when a completed request is taken off, immediately fills
it back up with queued requests.

This reduces the 99.9th percentile latencies in our tests by
60x and slightly increases the overall throughput, when using a
queue depth 2x the size of the Virtio queue size, with a
DPU-powered virtio-fs device.

Furthermore, the virtio-fs driver now also always lets -ENOMEM
errors go to userspace instead of retrying the request in the
driver.

Signed-off-by: Peter-Jan Gootzen <peter-jan@gootzen.net>
---
V3: Fixed requests falling into the void when -ENOMEM and no new
incoming requests. Virtio-fs now always lets -ENOMEM bubble up to
userspace. Also made queue full condition more explicit with
-ENOSPC in `send_forget_request`.
V2: Not scheduling dispatch work anymore when not needed
and changed delayed_work structs to work_struct structs

 fs/fuse/virtio_fs.c | 46 ++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
index 4d8d4f16c727..3a3231ddb9e7 100644
--- a/fs/fuse/virtio_fs.c
+++ b/fs/fuse/virtio_fs.c
@@ -45,7 +45,7 @@ struct virtio_fs_vq {
 	struct work_struct done_work;
 	struct list_head queued_reqs;
 	struct list_head end_reqs;	/* End these requests */
-	struct delayed_work dispatch_work;
+	struct work_struct dispatch_work;
 	struct fuse_dev *fud;
 	bool connected;
 	long in_flight;
@@ -202,7 +202,7 @@ static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
 	}
 
 	flush_work(&fsvq->done_work);
-	flush_delayed_work(&fsvq->dispatch_work);
+	flush_work(&fsvq->dispatch_work);
 }
 
 static void virtio_fs_drain_all_queues_locked(struct virtio_fs *fs)
@@ -346,6 +346,9 @@ static void virtio_fs_hiprio_done_work(struct work_struct *work)
 			dec_in_flight_req(fsvq);
 		}
 	} while (!virtqueue_enable_cb(vq) && likely(!virtqueue_is_broken(vq)));
+
+	if (!list_empty(&fsvq->queued_reqs))
+		schedule_work(&fsvq->dispatch_work);
 	spin_unlock(&fsvq->lock);
 }
 
@@ -353,7 +356,7 @@ static void virtio_fs_request_dispatch_work(struct work_struct *work)
 {
 	struct fuse_req *req;
 	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
-						 dispatch_work.work);
+						 dispatch_work);
 	int ret;
 
 	pr_debug("virtio-fs: worker %s called.\n", __func__);
@@ -385,11 +388,9 @@ static void virtio_fs_request_dispatch_work(struct work_struct *work)
 
 		ret = virtio_fs_enqueue_req(fsvq, req, true);
 		if (ret < 0) {
-			if (ret == -ENOMEM || ret == -ENOSPC) {
+			if (ret == -ENOSPC) {
 				spin_lock(&fsvq->lock);
 				list_add_tail(&req->list, &fsvq->queued_reqs);
-				schedule_delayed_work(&fsvq->dispatch_work,
-						      msecs_to_jiffies(1));
 				spin_unlock(&fsvq->lock);
 				return;
 			}
@@ -405,8 +406,8 @@ static void virtio_fs_request_dispatch_work(struct work_struct *work)
 }
 
 /*
- * Returns 1 if queue is full and sender should wait a bit before sending
- * next request, 0 otherwise.
+ * Returns 0 if request has been successfully sent, otherwise -ENOSPC
+ * when the queue is full.
  */
 static int send_forget_request(struct virtio_fs_vq *fsvq,
 			       struct virtio_fs_forget *forget,
@@ -432,16 +433,12 @@ static int send_forget_request(struct virtio_fs_vq *fsvq,
 
 	ret = virtqueue_add_outbuf(vq, &sg, 1, forget, GFP_ATOMIC);
 	if (ret < 0) {
-		if (ret == -ENOMEM || ret == -ENOSPC) {
+		if (ret == -ENOSPC) {
 			pr_debug("virtio-fs: Could not queue FORGET: err=%d. Will try later\n",
 				 ret);
 			list_add_tail(&forget->list, &fsvq->queued_reqs);
-			schedule_delayed_work(&fsvq->dispatch_work,
-					      msecs_to_jiffies(1));
 			if (!in_flight)
 				inc_in_flight_req(fsvq);
-			/* Queue is full */
-			ret = 1;
 		} else {
 			pr_debug("virtio-fs: Could not queue FORGET: err=%d. Dropping it.\n",
 				 ret);
@@ -469,7 +466,7 @@ static void virtio_fs_hiprio_dispatch_work(struct work_struct *work)
 {
 	struct virtio_fs_forget *forget;
 	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
-						 dispatch_work.work);
+						 dispatch_work);
 	pr_debug("virtio-fs: worker %s called.\n", __func__);
 	while (1) {
 		spin_lock(&fsvq->lock);
@@ -482,7 +479,7 @@ static void virtio_fs_hiprio_dispatch_work(struct work_struct *work)
 
 		list_del(&forget->list);
 		spin_unlock(&fsvq->lock);
-		if (send_forget_request(fsvq, forget, true))
+		if (send_forget_request(fsvq, forget, true) == -ENOSPC)
 			return;
 	}
 }
@@ -647,6 +644,11 @@ static void virtio_fs_requests_done_work(struct work_struct *work)
 			virtio_fs_request_complete(req, fsvq);
 		}
 	}
+
+	spin_lock(&fsvq->lock);
+	if (!list_empty(&fsvq->queued_reqs))
+		schedule_work(&fsvq->dispatch_work);
+	spin_unlock(&fsvq->lock);
 }
 
 /* Virtqueue interrupt handler */
@@ -670,12 +672,12 @@ static void virtio_fs_init_vq(struct virtio_fs_vq *fsvq, char *name,
 
 	if (vq_type == VQ_REQUEST) {
 		INIT_WORK(&fsvq->done_work, virtio_fs_requests_done_work);
-		INIT_DELAYED_WORK(&fsvq->dispatch_work,
-				  virtio_fs_request_dispatch_work);
+		INIT_WORK(&fsvq->dispatch_work,
+			  virtio_fs_request_dispatch_work);
 	} else {
 		INIT_WORK(&fsvq->done_work, virtio_fs_hiprio_done_work);
-		INIT_DELAYED_WORK(&fsvq->dispatch_work,
-				  virtio_fs_hiprio_dispatch_work);
+		INIT_WORK(&fsvq->dispatch_work,
+			  virtio_fs_hiprio_dispatch_work);
 	}
 }
 
@@ -1246,7 +1248,7 @@ __releases(fiq->lock)
 	fsvq = &fs->vqs[queue_id];
 	ret = virtio_fs_enqueue_req(fsvq, req, false);
 	if (ret < 0) {
-		if (ret == -ENOMEM || ret == -ENOSPC) {
+		if (ret == -ENOSPC) {
 			/*
 			 * Virtqueue full. Retry submission from worker
 			 * context as we might be holding fc->bg_lock.
@@ -1254,8 +1256,6 @@ __releases(fiq->lock)
 			spin_lock(&fsvq->lock);
 			list_add_tail(&req->list, &fsvq->queued_reqs);
 			inc_in_flight_req(fsvq);
-			schedule_delayed_work(&fsvq->dispatch_work,
-						msecs_to_jiffies(1));
 			spin_unlock(&fsvq->lock);
 			return;
 		}
@@ -1265,7 +1265,7 @@ __releases(fiq->lock)
 		/* Can't end request in submission context. Use a worker */
 		spin_lock(&fsvq->lock);
 		list_add_tail(&req->list, &fsvq->end_reqs);
-		schedule_delayed_work(&fsvq->dispatch_work, 0);
+		schedule_work(&fsvq->dispatch_work);
 		spin_unlock(&fsvq->lock);
 		return;
 	}
-- 
2.34.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3] virtio-fs: Improved request latencies when Virtio queue is full

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 1516 bytes --]

On Fri, Jun 02, 2023 at 03:32:26PM +0200, Peter-Jan Gootzen wrote:
> When the Virtio queue is full, a work item is scheduled
> to execute in 1ms that retries adding the request to the queue.
> This is a large amount of time on the scale on which a
> virtio-fs device can operate. When using a DPU this is around
> 40us baseline without going to a remote server (4k, QD=1).
> This patch queues requests when the Virtio queue is full,
> and when a completed request is taken off, immediately fills
> it back up with queued requests.
> 
> This reduces the 99.9th percentile latencies in our tests by
> 60x and slightly increases the overall throughput, when using a
> queue depth 2x the size of the Virtio queue size, with a
> DPU-powered virtio-fs device.
> 
> Furthermore, the virtio-fs driver now also always lets -ENOMEM
> errors go to userspace instead of retrying the request in the
> driver.
> 
> Signed-off-by: Peter-Jan Gootzen <peter-jan@gootzen.net>
> ---
> V3: Fixed requests falling into the void when -ENOMEM and no new
> incoming requests. Virtio-fs now always lets -ENOMEM bubble up to
> userspace. Also made queue full condition more explicit with
> -ENOSPC in `send_forget_request`.
> V2: Not scheduling dispatch work anymore when not needed
> and changed delayed_work structs to work_struct structs

Hi Peter-Jan,
I will be traveling and will try to look at this patch as soon as
possible.

If Vivek and others are happy, please don't wait for me.

Thanks,
Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V3] virtio-fs: Improved request latencies when Virtio queue is full

[Vivek Goyal]

On Fri, Jun 02, 2023 at 03:32:26PM +0200, Peter-Jan Gootzen wrote:
> When the Virtio queue is full, a work item is scheduled
> to execute in 1ms that retries adding the request to the queue.
> This is a large amount of time on the scale on which a
> virtio-fs device can operate. When using a DPU this is around
> 40us baseline without going to a remote server (4k, QD=1).
> This patch queues requests when the Virtio queue is full,
> and when a completed request is taken off, immediately fills
> it back up with queued requests.
> 
> This reduces the 99.9th percentile latencies in our tests by
> 60x and slightly increases the overall throughput, when using a
> queue depth 2x the size of the Virtio queue size, with a
> DPU-powered virtio-fs device.
> 
> Furthermore, the virtio-fs driver now also always lets -ENOMEM
> errors go to userspace instead of retrying the request in the
> driver.
> 
> Signed-off-by: Peter-Jan Gootzen <peter-jan@gootzen.net>
> ---
> V3: Fixed requests falling into the void when -ENOMEM and no new
> incoming requests. Virtio-fs now always lets -ENOMEM bubble up to
> userspace. Also made queue full condition more explicit with
> -ENOSPC in `send_forget_request`.
> V2: Not scheduling dispatch work anymore when not needed
> and changed delayed_work structs to work_struct structs
> 
>  fs/fuse/virtio_fs.c | 46 ++++++++++++++++++++++-----------------------
>  1 file changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 4d8d4f16c727..3a3231ddb9e7 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -45,7 +45,7 @@ struct virtio_fs_vq {
>  	struct work_struct done_work;
>  	struct list_head queued_reqs;
>  	struct list_head end_reqs;	/* End these requests */
> -	struct delayed_work dispatch_work;
> +	struct work_struct dispatch_work;
>  	struct fuse_dev *fud;
>  	bool connected;
>  	long in_flight;
> @@ -202,7 +202,7 @@ static void virtio_fs_drain_queue(struct virtio_fs_vq *fsvq)
>  	}
>  
>  	flush_work(&fsvq->done_work);
> -	flush_delayed_work(&fsvq->dispatch_work);
> +	flush_work(&fsvq->dispatch_work);
>  }
>  
>  static void virtio_fs_drain_all_queues_locked(struct virtio_fs *fs)
> @@ -346,6 +346,9 @@ static void virtio_fs_hiprio_done_work(struct work_struct *work)
>  			dec_in_flight_req(fsvq);
>  		}
>  	} while (!virtqueue_enable_cb(vq) && likely(!virtqueue_is_broken(vq)));
> +
> +	if (!list_empty(&fsvq->queued_reqs))
> +		schedule_work(&fsvq->dispatch_work);
>  	spin_unlock(&fsvq->lock);
>  }
>  
> @@ -353,7 +356,7 @@ static void virtio_fs_request_dispatch_work(struct work_struct *work)
>  {
>  	struct fuse_req *req;
>  	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
> -						 dispatch_work.work);
> +						 dispatch_work);
>  	int ret;
>  
>  	pr_debug("virtio-fs: worker %s called.\n", __func__);
> @@ -385,11 +388,9 @@ static void virtio_fs_request_dispatch_work(struct work_struct *work)
>  
>  		ret = virtio_fs_enqueue_req(fsvq, req, true);
>  		if (ret < 0) {
> -			if (ret == -ENOMEM || ret == -ENOSPC) {
> +			if (ret == -ENOSPC) {

I think this change should be a separate patch with proper justification.
This change has nothing to do with reducing the latency of submitting
the queued request.

Thanks
Vivek

>  				spin_lock(&fsvq->lock);
>  				list_add_tail(&req->list, &fsvq->queued_reqs);
> -				schedule_delayed_work(&fsvq->dispatch_work,
> -						      msecs_to_jiffies(1));
>  				spin_unlock(&fsvq->lock);
>  				return;
>  			}
> @@ -405,8 +406,8 @@ static void virtio_fs_request_dispatch_work(struct work_struct *work)
>  }
>  
>  /*
> - * Returns 1 if queue is full and sender should wait a bit before sending
> - * next request, 0 otherwise.
> + * Returns 0 if request has been successfully sent, otherwise -ENOSPC
> + * when the queue is full.
>   */
>  static int send_forget_request(struct virtio_fs_vq *fsvq,
>  			       struct virtio_fs_forget *forget,
> @@ -432,16 +433,12 @@ static int send_forget_request(struct virtio_fs_vq *fsvq,
>  
>  	ret = virtqueue_add_outbuf(vq, &sg, 1, forget, GFP_ATOMIC);
>  	if (ret < 0) {
> -		if (ret == -ENOMEM || ret == -ENOSPC) {
> +		if (ret == -ENOSPC) {
>  			pr_debug("virtio-fs: Could not queue FORGET: err=%d. Will try later\n",
>  				 ret);
>  			list_add_tail(&forget->list, &fsvq->queued_reqs);
> -			schedule_delayed_work(&fsvq->dispatch_work,
> -					      msecs_to_jiffies(1));
>  			if (!in_flight)
>  				inc_in_flight_req(fsvq);
> -			/* Queue is full */
> -			ret = 1;
>  		} else {
>  			pr_debug("virtio-fs: Could not queue FORGET: err=%d. Dropping it.\n",
>  				 ret);
> @@ -469,7 +466,7 @@ static void virtio_fs_hiprio_dispatch_work(struct work_struct *work)
>  {
>  	struct virtio_fs_forget *forget;
>  	struct virtio_fs_vq *fsvq = container_of(work, struct virtio_fs_vq,
> -						 dispatch_work.work);
> +						 dispatch_work);
>  	pr_debug("virtio-fs: worker %s called.\n", __func__);
>  	while (1) {
>  		spin_lock(&fsvq->lock);
> @@ -482,7 +479,7 @@ static void virtio_fs_hiprio_dispatch_work(struct work_struct *work)
>  
>  		list_del(&forget->list);
>  		spin_unlock(&fsvq->lock);
> -		if (send_forget_request(fsvq, forget, true))
> +		if (send_forget_request(fsvq, forget, true) == -ENOSPC)
>  			return;
>  	}
>  }
> @@ -647,6 +644,11 @@ static void virtio_fs_requests_done_work(struct work_struct *work)
>  			virtio_fs_request_complete(req, fsvq);
>  		}
>  	}
> +
> +	spin_lock(&fsvq->lock);
> +	if (!list_empty(&fsvq->queued_reqs))
> +		schedule_work(&fsvq->dispatch_work);
> +	spin_unlock(&fsvq->lock);
>  }
>  
>  /* Virtqueue interrupt handler */
> @@ -670,12 +672,12 @@ static void virtio_fs_init_vq(struct virtio_fs_vq *fsvq, char *name,
>  
>  	if (vq_type == VQ_REQUEST) {
>  		INIT_WORK(&fsvq->done_work, virtio_fs_requests_done_work);
> -		INIT_DELAYED_WORK(&fsvq->dispatch_work,
> -				  virtio_fs_request_dispatch_work);
> +		INIT_WORK(&fsvq->dispatch_work,
> +			  virtio_fs_request_dispatch_work);
>  	} else {
>  		INIT_WORK(&fsvq->done_work, virtio_fs_hiprio_done_work);
> -		INIT_DELAYED_WORK(&fsvq->dispatch_work,
> -				  virtio_fs_hiprio_dispatch_work);
> +		INIT_WORK(&fsvq->dispatch_work,
> +			  virtio_fs_hiprio_dispatch_work);
>  	}
>  }
>  
> @@ -1246,7 +1248,7 @@ __releases(fiq->lock)
>  	fsvq = &fs->vqs[queue_id];
>  	ret = virtio_fs_enqueue_req(fsvq, req, false);
>  	if (ret < 0) {
> -		if (ret == -ENOMEM || ret == -ENOSPC) {
> +		if (ret == -ENOSPC) {
>  			/*
>  			 * Virtqueue full. Retry submission from worker
>  			 * context as we might be holding fc->bg_lock.
> @@ -1254,8 +1256,6 @@ __releases(fiq->lock)
>  			spin_lock(&fsvq->lock);
>  			list_add_tail(&req->list, &fsvq->queued_reqs);
>  			inc_in_flight_req(fsvq);
> -			schedule_delayed_work(&fsvq->dispatch_work,
> -						msecs_to_jiffies(1));
>  			spin_unlock(&fsvq->lock);
>  			return;
>  		}
> @@ -1265,7 +1265,7 @@ __releases(fiq->lock)
>  		/* Can't end request in submission context. Use a worker */
>  		spin_lock(&fsvq->lock);
>  		list_add_tail(&req->list, &fsvq->end_reqs);
> -		schedule_delayed_work(&fsvq->dispatch_work, 0);
> +		schedule_work(&fsvq->dispatch_work);
>  		spin_unlock(&fsvq->lock);
>  		return;
>  	}
> -- 
> 2.34.1
> 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization