|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [UNIKRAFT PATCH v2 8/9] lib/uk9p: Recycle uk_9preq allocations
Reviewed-by: Costin Lupu <costin.lupu@xxxxxxxxx>
On 5/9/20 7:44 PM, Cristian Banu wrote:
> Each 9p device keeps a free-list of 9p requests. This allows
> allocations to take much less time, as usually the workflow of a 9p
> device is synchronous: create request, wait for reply, process it,
> repeat. This approach avoids the allocation performance hit on
> subsequent request creations.
>
> Results below indicate that request_allocated takes < 100ns,
> as opposed to 1us (previous commit in this patch series).
>
> In the sample below, the read() now spends most of the time (~99.8%)
> either notifying the host or waiting for a reply. At this scale,
> even uk_traces are probably noisy and the "real" time might be lower
> by a relatively large amount.
>
> Excerpt from the uk traces on a read():
>
> 500281499 uk_9p_trace_request_create
> 500281570 uk_9p_trace_request_allocated
> 500281603 uk_9p_trace_ready tag 0
> 500283423 uk_9p_trace_sent tag 0
> 500347243 uk_9p_trace_received tag 0
>
> Signed-off-by: Cristian Banu <cristb@xxxxxxxxx>
> ---
> lib/uk9p/9pdev.c | 80 +++++++++++++++++++++++++++++---
> lib/uk9p/9preq.c | 14 ++----
> lib/uk9p/include/uk/9pdev.h | 10 ++++
> lib/uk9p/include/uk/9pdev_core.h | 2 +
> lib/uk9p/include/uk/9preq.h | 15 +++---
> 5 files changed, 95 insertions(+), 26 deletions(-)
>
> diff --git a/lib/uk9p/9pdev.c b/lib/uk9p/9pdev.c
> index 40a4daa..bdd6a84 100644
> --- a/lib/uk9p/9pdev.c
> +++ b/lib/uk9p/9pdev.c
> @@ -132,6 +132,7 @@ static void _req_mgmt_init(struct uk_9pdev_req_mgmt
> *req_mgmt)
> ukarch_spin_lock_init(&req_mgmt->spinlock);
> uk_bitmap_zero(req_mgmt->tag_bm, UK_9P_NUMTAGS);
> UK_INIT_LIST_HEAD(&req_mgmt->req_list);
> + UK_INIT_LIST_HEAD(&req_mgmt->req_free_list);
> }
>
> static void _req_mgmt_add_req_locked(struct uk_9pdev_req_mgmt *req_mgmt,
> @@ -141,6 +142,21 @@ static void _req_mgmt_add_req_locked(struct
> uk_9pdev_req_mgmt *req_mgmt,
> uk_list_add(&req->_list, &req_mgmt->req_list);
> }
>
> +static struct uk_9preq *
> +_req_mgmt_from_freelist_locked(struct uk_9pdev_req_mgmt *req_mgmt)
> +{
> + struct uk_9preq *req;
> +
> + if (uk_list_empty(&req_mgmt->req_free_list))
> + return NULL;
> +
> + req = uk_list_first_entry(&req_mgmt->req_free_list,
> + struct uk_9preq, _list);
> + uk_list_del(&req->_list);
> +
> + return req;
> +}
> +
> static void _req_mgmt_del_req_locked(struct uk_9pdev_req_mgmt *req_mgmt,
> struct uk_9preq *req)
> {
> @@ -148,6 +164,12 @@ static void _req_mgmt_del_req_locked(struct
> uk_9pdev_req_mgmt *req_mgmt,
> uk_list_del(&req->_list);
> }
>
> +static void _req_mgmt_req_to_freelist_locked(struct uk_9pdev_req_mgmt
> *req_mgmt,
> + struct uk_9preq *req)
> +{
> + uk_list_add(&req->_list, &req_mgmt->req_free_list);
> +}
> +
> static uint16_t _req_mgmt_next_tag_locked(struct uk_9pdev_req_mgmt *req_mgmt)
> {
> return uk_find_next_zero_bit(req_mgmt->tag_bm, UK_9P_NUMTAGS, 0);
> @@ -164,10 +186,24 @@ static void _req_mgmt_cleanup(struct uk_9pdev_req_mgmt
> *req_mgmt __unused)
> tag = req->tag;
> _req_mgmt_del_req_locked(req_mgmt, req);
> if (!uk_9preq_put(req)) {
> - uk_pr_warn("Tag %d still has references on cleanup.\n",
> + /* If in the future these references get released, mark
> + * _dev as NULL so uk_9pdev_req_to_freelist doesn't
> + * attempt to place them in an invalid memory region.
> + *
> + * As _dev is not used for any other purpose, this
> + * doesn't impact any other logic related to 9p request
> + * processing.
> + */
> + req->_dev = NULL;
> + uk_pr_err("Tag %d still has references on cleanup.\n",
> tag);
> }
> }
> + uk_list_for_each_entry_safe(req, reqn, &req_mgmt->req_free_list,
> + _list) {
> + uk_list_del(&req->_list);
> + uk_free(req->_a, req);
> + }
> ukplat_spin_unlock_irqrestore(&req_mgmt->spinlock, flags);
> }
>
> @@ -297,17 +333,37 @@ struct uk_9preq *uk_9pdev_req_create(struct uk_9pdev
> *dev, uint8_t type)
>
> UK_ASSERT(dev);
>
> - req = uk_9preq_alloc(dev->a);
> - if (req == NULL) {
> - rc = -ENOMEM;
> - goto out;
> + ukplat_spin_lock_irqsave(&dev->_req_mgmt.spinlock, flags);
> + if (!(req = _req_mgmt_from_freelist_locked(&dev->_req_mgmt))) {
> + /* Don't allocate with the spinlock held. */
> + ukplat_spin_unlock_irqrestore(&dev->_req_mgmt.spinlock, flags);
> + req = uk_calloc(dev->a, 1, sizeof(*req));
> + if (req == NULL) {
> + rc = -ENOMEM;
> + goto out;
> + }
> + req->_dev = dev;
> + /*
> + * Duplicate this, instead of using req->_dev, as we can't rely
> + * on the value of _dev at time of free. Check comment in
> + * _req_mgmt_cleanup.
> + */
> + req->_a = dev->a;
> + ukplat_spin_lock_irqsave(&dev->_req_mgmt.spinlock, flags);
> }
>
> + uk_9preq_init(req);
> +
> + /*
> + * If request was from the free list, it should already belong to the
> + * dev.
> + */
> + UK_ASSERT(req->_dev == dev);
> +
> /* Shouldn't exceed the msize on non-zerocopy buffers, just in case. */
> req->recv.size = MIN(req->recv.size, dev->msize);
> req->xmit.size = MIN(req->xmit.size, dev->msize);
>
> - ukplat_spin_lock_irqsave(&dev->_req_mgmt.spinlock, flags);
> if (type == UK_9P_TVERSION)
> tag = UK_9P_NOTAG;
> else
> @@ -360,6 +416,18 @@ int uk_9pdev_req_remove(struct uk_9pdev *dev, struct
> uk_9preq *req)
> return uk_9preq_put(req);
> }
>
> +void uk_9pdev_req_to_freelist(struct uk_9pdev *dev, struct uk_9preq *req)
> +{
> + unsigned long flags;
> +
> + if (!dev)
> + return;
> +
> + ukplat_spin_lock_irqsave(&dev->_req_mgmt.spinlock, flags);
> + _req_mgmt_req_to_freelist_locked(&dev->_req_mgmt, req);
> + ukplat_spin_unlock_irqrestore(&dev->_req_mgmt.spinlock, flags);
> +}
> +
> struct uk_9pfid *uk_9pdev_fid_create(struct uk_9pdev *dev)
> {
> struct uk_9pfid *fid = NULL;
> diff --git a/lib/uk9p/9preq.c b/lib/uk9p/9preq.c
> index d44e684..edc462c 100644
> --- a/lib/uk9p/9preq.c
> +++ b/lib/uk9p/9preq.c
> @@ -35,6 +35,7 @@
> #include <string.h>
> #include <uk/config.h>
> #include <uk/9preq.h>
> +#include <uk/9pdev.h>
> #include <uk/9p_core.h>
> #include <uk/list.h>
> #include <uk/refcount.h>
> @@ -45,14 +46,8 @@
> #include <uk/wait.h>
> #endif
>
> -struct uk_9preq *uk_9preq_alloc(struct uk_alloc *a)
> +void uk_9preq_init(struct uk_9preq *req)
> {
> - struct uk_9preq *req;
> -
> - req = uk_calloc(a, 1, sizeof(*req));
> - if (req == NULL)
> - return NULL;
> -
> req->xmit.buf = req->xmit_buf;
> req->recv.buf = req->recv_buf;
> req->xmit.size = req->recv.size = UK_9P_BUFSIZE;
> @@ -69,13 +64,10 @@ struct uk_9preq *uk_9preq_alloc(struct uk_alloc *a)
> req->recv.offset = 0;
>
> UK_INIT_LIST_HEAD(&req->_list);
> - req->_a = a;
> uk_refcount_init(&req->refcount, 1);
> #if CONFIG_LIBUKSCHED
> uk_waitq_init(&req->wq);
> #endif
> -
> - return req;
> }
>
> void uk_9preq_get(struct uk_9preq *req)
> @@ -89,7 +81,7 @@ int uk_9preq_put(struct uk_9preq *req)
>
> last = uk_refcount_release(&req->refcount);
> if (last)
> - uk_free(req->_a, req);
> + uk_9pdev_req_to_freelist(req->_dev, req);
>
> return last;
> }
> diff --git a/lib/uk9p/include/uk/9pdev.h b/lib/uk9p/include/uk/9pdev.h
> index 1225336..560ba8f 100644
> --- a/lib/uk9p/include/uk/9pdev.h
> +++ b/lib/uk9p/include/uk/9pdev.h
> @@ -149,6 +149,16 @@ struct uk_9preq *uk_9pdev_req_lookup(struct uk_9pdev
> *dev, uint16_t tag);
> */
> int uk_9pdev_req_remove(struct uk_9pdev *dev, struct uk_9preq *req);
>
> +/**
> + * Places the given request on the 9p device's request freelist.
> + *
> + * @param dev
> + * The Unikraft 9P Device. If NULL, doesn't place the request on the
> freelist.
> + * @param req
> + * The request to be placed on the freelist.
> + */
> +void uk_9pdev_req_to_freelist(struct uk_9pdev *dev, struct uk_9preq *req);
> +
> /**
> * Creates a FID associated with the given 9P device.
> *
> diff --git a/lib/uk9p/include/uk/9pdev_core.h
> b/lib/uk9p/include/uk/9pdev_core.h
> index 38864ac..fcad1ef 100644
> --- a/lib/uk9p/include/uk/9pdev_core.h
> +++ b/lib/uk9p/include/uk/9pdev_core.h
> @@ -120,6 +120,8 @@ struct uk_9pdev_req_mgmt {
> unsigned long tag_bm[UK_BITS_TO_LONGS(UK_9P_NUMTAGS)];
> /* List of requests allocated and not yet removed. */
> struct uk_list_head req_list;
> + /* Free-list of requests. */
> + struct uk_list_head req_free_list;
> };
>
> /**
> diff --git a/lib/uk9p/include/uk/9preq.h b/lib/uk9p/include/uk/9preq.h
> index ed883d2..aad8d42 100644
> --- a/lib/uk9p/include/uk/9preq.h
> +++ b/lib/uk9p/include/uk/9preq.h
> @@ -162,6 +162,8 @@ struct uk_9preq {
> uint16_t tag;
> /* Entry into the list of requests (API-internal). */
> struct uk_list_head _list;
> + /* @internal 9P device this request belongs to. */
> + struct uk_9pdev *_dev;
> /* @internal Allocator used to allocate this request. */
> struct uk_alloc *_a;
> /* Tracks the number of references to this structure. */
> @@ -176,16 +178,10 @@ UK_CTASSERT(sizeof(struct uk_9preq) <= __PAGE_SIZE);
>
> /**
> * @internal
> - * Allocates a 9p request.
> + * Initializes a 9P request.
> * Should not be used directly, use uk_9pdev_req_create() instead.
> - *
> - * @param a
> - * Allocator to use.
> - * @return
> - * - (==NULL): Out of memory.
> - * - (!=NULL): Successful.
> */
> -struct uk_9preq *uk_9preq_alloc(struct uk_alloc *a);
> +void uk_9preq_init(struct uk_9preq *req);
>
> /**
> * Gets the 9p request, incrementing the reference count.
> @@ -197,7 +193,8 @@ void uk_9preq_get(struct uk_9preq *req);
>
> /**
> * Puts the 9p request, decrementing the reference count.
> - * If this was the last live reference, the memory will be freed.
> + * If this was the last live reference, it will be placed on the asociated
> + * device's request freelist.
> *
> * @param req
> * Reference to the 9p request.
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |