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-...@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
