|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [UNIKRAFT PATCH v2 7/9] lib/uk9p: Use fixed-size request buffers
Reviewed-by: Costin Lupu <costin.lupu@xxxxxxxxx>
On 5/9/20 7:44 PM, Cristian Banu wrote:
> Use fixed-size request buffers instead of dynamically allocating new
> receive and transmit buffers for each new request. This reduces the
> number of calls to uk_alloc from 3 to 1 per each uk_9pdev_req_create()
> call.
>
> Allocating a request is faster by roughly 2.5 microseconds. The
> previous patch in this patch series shows a 3.7us delay between read
> and request_allocated. The excerpt below shows roughly 1us delay.
> This performance benefit is observed at a larger scale as well (in
> latency and throughput benchmarks).
>
> Excerpt from the output traces for a read():
>
> 1350682319 uk_9p_trace_request_create
> 1350683329 uk_9p_trace_request_allocated
> 1350683375 uk_9p_trace_ready tag 0
> 1350686009 uk_9p_trace_sent tag 0
> 1350748124 uk_9p_trace_received tag 0
>
> Signed-off-by: Cristian Banu <cristb@xxxxxxxxx>
> ---
> lib/uk9p/9p.c | 29 +++++++-------
> lib/uk9p/9pdev.c | 11 +++---
> lib/uk9p/9preq.c | 76 ++++++++-----------------------------
> lib/uk9p/include/uk/9pdev.h | 5 +--
> lib/uk9p/include/uk/9preq.h | 31 +++++++++++++--
> 5 files changed, 65 insertions(+), 87 deletions(-)
>
> diff --git a/lib/uk9p/9p.c b/lib/uk9p/9p.c
> index 4acb0c6..9ac7e8a 100644
> --- a/lib/uk9p/9p.c
> +++ b/lib/uk9p/9p.c
> @@ -74,13 +74,12 @@ static inline int send_and_wait_no_zc(struct uk_9pdev
> *dev,
> return send_and_wait_zc(dev, req, UK_9PREQ_ZCDIR_NONE, NULL, 0, 0);
> }
>
> -static struct uk_9preq *request_create(struct uk_9pdev *dev,
> - uint8_t type, uint32_t size)
> +static struct uk_9preq *request_create(struct uk_9pdev *dev, uint8_t type)
> {
> struct uk_9preq *req;
>
> uk_9p_trace_request_create();
> - req = uk_9pdev_req_create(dev, type, size);
> + req = uk_9pdev_req_create(dev, type);
> if (!PTRISERR(req))
> uk_9p_trace_request_allocated();
>
> @@ -97,7 +96,7 @@ struct uk_9preq *uk_9p_version(struct uk_9pdev *dev,
>
> uk_9p_str_init(&requested_str, requested);
>
> - req = request_create(dev, UK_9P_TVERSION, __PAGE_SIZE);
> + req = request_create(dev, UK_9P_TVERSION);
> if (PTRISERR(req))
> return req;
>
> @@ -146,7 +145,7 @@ struct uk_9pfid *uk_9p_attach(struct uk_9pdev *dev,
> uint32_t afid,
> if (PTRISERR(fid))
> return fid;
>
> - req = request_create(dev, UK_9P_TATTACH, __PAGE_SIZE);
> + req = request_create(dev, UK_9P_TATTACH);
> if (PTRISERR(req)) {
> uk_9pdev_fid_release(fid);
> return (void *)req;
> @@ -183,7 +182,7 @@ int uk_9p_flush(struct uk_9pdev *dev, uint16_t oldtag)
> struct uk_9preq *req;
> int rc = 0;
>
> - req = request_create(dev, UK_9P_TFLUSH, __PAGE_SIZE);
> + req = request_create(dev, UK_9P_TFLUSH);
> if (PTRISERR(req))
> return PTR2ERR(req);
>
> @@ -216,7 +215,7 @@ struct uk_9pfid *uk_9p_walk(struct uk_9pdev *dev, struct
> uk_9pfid *fid,
>
> nwname = name ? 1 : 0;
>
> - req = request_create(dev, UK_9P_TWALK, __PAGE_SIZE);
> + req = request_create(dev, UK_9P_TWALK);
> if (PTRISERR(req)) {
> rc = PTR2ERR(req);
> goto out;
> @@ -272,7 +271,7 @@ int uk_9p_open(struct uk_9pdev *dev, struct uk_9pfid
> *fid, uint8_t mode)
> struct uk_9preq *req;
> int rc = 0;
>
> - req = request_create(dev, UK_9P_TOPEN, __PAGE_SIZE);
> + req = request_create(dev, UK_9P_TOPEN);
> if (PTRISERR(req))
> return PTR2ERR(req);
>
> @@ -306,7 +305,7 @@ int uk_9p_create(struct uk_9pdev *dev, struct uk_9pfid
> *fid,
> uk_9p_str_init(&name_str, name);
> uk_9p_str_init(&extension_str, extension);
>
> - req = request_create(dev, UK_9P_TCREATE, __PAGE_SIZE);
> + req = request_create(dev, UK_9P_TCREATE);
> if (PTRISERR(req))
> return PTR2ERR(req);
>
> @@ -339,7 +338,7 @@ int uk_9p_remove(struct uk_9pdev *dev, struct uk_9pfid
> *fid)
> /* The fid is considered invalid even if the remove fails. */
> fid->was_removed = 1;
>
> - req = request_create(dev, UK_9P_TREMOVE, __PAGE_SIZE);
> + req = request_create(dev, UK_9P_TREMOVE);
> if (PTRISERR(req))
> return PTR2ERR(req);
>
> @@ -362,7 +361,7 @@ int uk_9p_clunk(struct uk_9pdev *dev, struct uk_9pfid
> *fid)
> if (fid->was_removed)
> return 0;
>
> - req = request_create(dev, UK_9P_TCLUNK, __PAGE_SIZE);
> + req = request_create(dev, UK_9P_TCLUNK);
> if (PTRISERR(req))
> return PTR2ERR(req);
>
> @@ -390,7 +389,7 @@ int64_t uk_9p_read(struct uk_9pdev *dev, struct uk_9pfid
> *fid,
> uk_pr_debug("TREAD fid %u offset %lu count %u\n", fid->fid,
> offset, count);
>
> - req = request_create(dev, UK_9P_TREAD, __PAGE_SIZE);
> + req = request_create(dev, UK_9P_TREAD);
> if (PTRISERR(req))
> return PTR2ERR(req);
>
> @@ -422,7 +421,7 @@ int64_t uk_9p_write(struct uk_9pdev *dev, struct uk_9pfid
> *fid,
>
> uk_pr_debug("TWRITE fid %u offset %lu count %u\n", fid->fid,
> offset, count);
> - req = request_create(dev, UK_9P_TWRITE, __PAGE_SIZE);
> + req = request_create(dev, UK_9P_TWRITE);
> if (PTRISERR(req))
> return PTR2ERR(req);
>
> @@ -450,7 +449,7 @@ struct uk_9preq *uk_9p_stat(struct uk_9pdev *dev, struct
> uk_9pfid *fid,
> int rc = 0;
> uint16_t dummy;
>
> - req = request_create(dev, UK_9P_TSTAT, __PAGE_SIZE);
> + req = request_create(dev, UK_9P_TSTAT);
> if (PTRISERR(req))
> return req;
>
> @@ -478,7 +477,7 @@ int uk_9p_wstat(struct uk_9pdev *dev, struct uk_9pfid
> *fid,
> int rc = 0;
> uint16_t *dummy;
>
> - req = request_create(dev, UK_9P_TWSTAT, __PAGE_SIZE);
> + req = request_create(dev, UK_9P_TWSTAT);
> if (PTRISERR(req))
> return PTR2ERR(req);
>
> diff --git a/lib/uk9p/9pdev.c b/lib/uk9p/9pdev.c
> index 1d7e09b..40a4daa 100644
> --- a/lib/uk9p/9pdev.c
> +++ b/lib/uk9p/9pdev.c
> @@ -288,8 +288,7 @@ void uk_9pdev_xmit_notify(struct uk_9pdev *dev)
> #endif
> }
>
> -struct uk_9preq *uk_9pdev_req_create(struct uk_9pdev *dev, uint8_t type,
> - uint32_t size)
> +struct uk_9preq *uk_9pdev_req_create(struct uk_9pdev *dev, uint8_t type)
> {
> struct uk_9preq *req;
> int rc = 0;
> @@ -298,14 +297,16 @@ struct uk_9preq *uk_9pdev_req_create(struct uk_9pdev
> *dev, uint8_t type,
>
> UK_ASSERT(dev);
>
> - size = MIN(size, dev->msize);
> -
> - req = uk_9preq_alloc(dev->a, size);
> + req = uk_9preq_alloc(dev->a);
> if (req == NULL) {
> rc = -ENOMEM;
> goto out;
> }
>
> + /* 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;
> diff --git a/lib/uk9p/9preq.c b/lib/uk9p/9preq.c
> index 997f772..d44e684 100644
> --- a/lib/uk9p/9preq.c
> +++ b/lib/uk9p/9preq.c
> @@ -45,51 +45,28 @@
> #include <uk/wait.h>
> #endif
>
> -static int _fcall_alloc(struct uk_alloc *a, struct uk_9preq_fcall *f,
> - uint32_t size)
> -{
> - UK_ASSERT(a);
> - UK_ASSERT(f);
> - UK_ASSERT(size > 0);
> -
> - f->buf = uk_calloc(a, size, sizeof(char));
> - if (f->buf == NULL)
> - return -ENOMEM;
> -
> - f->size = size;
> - f->offset = 0;
> - f->zc_buf = NULL;
> - f->zc_size = 0;
> - f->zc_offset = 0;
> -
> - return 0;
> -}
> -
> -static void _fcall_free(struct uk_alloc *a, struct uk_9preq_fcall *f)
> -{
> - UK_ASSERT(a);
> - UK_ASSERT(f);
> -
> - if (f->buf)
> - uk_free(a, f->buf);
> -}
> -
> -struct uk_9preq *uk_9preq_alloc(struct uk_alloc *a, uint32_t size)
> +struct uk_9preq *uk_9preq_alloc(struct uk_alloc *a)
> {
> struct uk_9preq *req;
> - int rc;
>
> req = uk_calloc(a, 1, sizeof(*req));
> if (req == NULL)
> - goto out;
> + return NULL;
>
> - rc = _fcall_alloc(a, &req->xmit, size);
> - if (rc < 0)
> - goto out_free;
> + req->xmit.buf = req->xmit_buf;
> + req->recv.buf = req->recv_buf;
> + req->xmit.size = req->recv.size = UK_9P_BUFSIZE;
> + req->xmit.zc_buf = req->recv.zc_buf = NULL;
> + req->xmit.zc_size = req->recv.zc_size = 0;
> + req->xmit.zc_offset = req->recv.zc_offset = 0;
>
> - rc = _fcall_alloc(a, &req->recv, MAX(size, UK_9P_RERROR_MAXSIZE));
> - if (rc < 0)
> - goto out_free;
> + /*
> + * Assume the header has already been written.
> + * The header itself will be written on uk_9preq_ready(), when the
> + * actual message size is known.
> + */
> + req->xmit.offset = UK_9P_HEADER_SIZE;
> + req->recv.offset = 0;
>
> UK_INIT_LIST_HEAD(&req->_list);
> req->_a = a;
> @@ -98,28 +75,7 @@ struct uk_9preq *uk_9preq_alloc(struct uk_alloc *a,
> uint32_t size)
> uk_waitq_init(&req->wq);
> #endif
>
> - /*
> - * Assume the header has already been written.
> - * The header itself will be written on uk_9preq_ready(), when the
> - * actual message size is known.
> - */
> - req->xmit.offset = UK_9P_HEADER_SIZE;
> -
> return req;
> -
> -out_free:
> - _fcall_free(a, &req->recv);
> - _fcall_free(a, &req->xmit);
> - uk_free(a, req);
> -out:
> - return NULL;
> -}
> -
> -static void _req_free(struct uk_9preq *req)
> -{
> - _fcall_free(req->_a, &req->recv);
> - _fcall_free(req->_a, &req->xmit);
> - uk_free(req->_a, req);
> }
>
> void uk_9preq_get(struct uk_9preq *req)
> @@ -133,7 +89,7 @@ int uk_9preq_put(struct uk_9preq *req)
>
> last = uk_refcount_release(&req->refcount);
> if (last)
> - _req_free(req);
> + uk_free(req->_a, req);
>
> return last;
> }
> diff --git a/lib/uk9p/include/uk/9pdev.h b/lib/uk9p/include/uk/9pdev.h
> index 04ff523..1225336 100644
> --- a/lib/uk9p/include/uk/9pdev.h
> +++ b/lib/uk9p/include/uk/9pdev.h
> @@ -114,15 +114,12 @@ void uk_9pdev_xmit_notify(struct uk_9pdev *dev);
> * The Unikraft 9P Device.
> * @param type
> * Transmit type of the request, e.g. Tversion, Tread, and so on.
> - * @param size
> - * The maximum size for the receive and send buffers.
> * @return
> * If not an error pointer, the created request.
> * Otherwise, the error in creating the request:
> * - ENOMEM: No memory for the request or no available tags.
> */
> -struct uk_9preq *uk_9pdev_req_create(struct uk_9pdev *dev, uint8_t type,
> - uint32_t size);
> +struct uk_9preq *uk_9pdev_req_create(struct uk_9pdev *dev, uint8_t type);
>
> /**
> * Looks up a request based on the given tag. This is generally used by
> diff --git a/lib/uk9p/include/uk/9preq.h b/lib/uk9p/include/uk/9preq.h
> index b9713e2..ed883d2 100644
> --- a/lib/uk9p/include/uk/9preq.h
> +++ b/lib/uk9p/include/uk/9preq.h
> @@ -63,6 +63,16 @@ extern "C" {
> */
> #define UK_9P_RERROR_MAXSIZE 141U
>
> +/*
> + * The transmit and receive buffer size.
> + *
> + * The buffer size is not expected to exceed 1K: reads and writes are
> + * zero-copy; on-the-wire size of a stat structure should not exceed
> + * 1K -- its base size is 61, along with the lengths for name, uid,
> + * gid, muid and extension strings.
> + */
> +#define UK_9P_BUFSIZE 1024U
> +
> /**
> * @internal
> *
> @@ -125,8 +135,23 @@ enum uk_9preq_state {
> * referenced anymore. A call to uk_9pdev_req_remove() is mandatory to
> * correctly free this and remove it from the list of requests managed
> * by the 9p device.
> + *
> + * Should fit within one page.
> */
> struct uk_9preq {
> + /*
> + * Fixed-size buffer for transmit, used for most messages.
> + * Large messages will always zero-copy from the user-provided
> + * buffer (on Twrite).
> + */
> + uint8_t xmit_buf[UK_9P_BUFSIZE];
> + /*
> + * Fixed-size buffer for receive, used for most messages.
> + * Large messages will always zero-copy into the user-provided
> + * buffer (on Tread).
> + */
> + uint8_t recv_buf[UK_9P_BUFSIZE];
> + /* 2 KB offset in the structure here. */
> /* Transmit fcall. */
> struct uk_9preq_fcall xmit;
> /* Receive fcall. */
> @@ -147,6 +172,8 @@ struct uk_9preq {
> #endif
> };
>
> +UK_CTASSERT(sizeof(struct uk_9preq) <= __PAGE_SIZE);
> +
> /**
> * @internal
> * Allocates a 9p request.
> @@ -154,13 +181,11 @@ struct uk_9preq {
> *
> * @param a
> * Allocator to use.
> - * @param size
> - * Minimum size of the receive and transmit buffers.
> * @return
> * - (==NULL): Out of memory.
> * - (!=NULL): Successful.
> */
> -struct uk_9preq *uk_9preq_alloc(struct uk_alloc *a, uint32_t size);
> +struct uk_9preq *uk_9preq_alloc(struct uk_alloc *a);
>
> /**
> * Gets the 9p request, incrementing the reference count.
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |