Menu
▾
▴
Re: [uml-devel] [PATCH] UBD Improvements Phase 1
|
From: Anton I. <ant...@ko...> - 2016-09-30 06:53:20
|
I noticed a formatting error and a logical error in the short read
"safety net". It is never invoked, but none the less should be fixed.
A revised patch with fixed formatting and a fix for the logical error
will follow shortly. Performance figures are unchanged: 30% gain to
start off with on raw read, 17% on find /usr -exec cat {}
A.
On 29/09/16 21:36, ant...@ko... wrote:
> From: Anton Ivanov <ai...@br...>
>
> UBD at present is extremely slow because it handles only
> one request at a time in the IO thread and IRQ handler.
>
> The single request at a time is replaced by handling multiple
> requests as well as necessary workarounds for short reads/writes.
>
> Resulting performance improvement in disk IO - 30%
>
> Signed-off-by: Anton Ivanov <ai...@br...>
> ---
> arch/um/drivers/ubd.h | 5 ++
> arch/um/drivers/ubd_kern.c | 161 ++++++++++++++++++++++++++++++++++++++-------
> arch/um/drivers/ubd_user.c | 19 +++++-
> 3 files changed, 159 insertions(+), 26 deletions(-)
>
> diff --git a/arch/um/drivers/ubd.h b/arch/um/drivers/ubd.h
> index 3b48cd2..cc1cc85 100644
> --- a/arch/um/drivers/ubd.h
> +++ b/arch/um/drivers/ubd.h
> @@ -11,5 +11,10 @@ extern int start_io_thread(unsigned long sp, int *fds_out);
> extern int io_thread(void *arg);
> extern int kernel_fd;
>
> +extern int ubd_read_poll(int timeout);
> +extern int ubd_write_poll(int timeout);
> +
> +#define UBD_REQ_BUFFER_SIZE 64
> +
> #endif
>
> diff --git a/arch/um/drivers/ubd_kern.c b/arch/um/drivers/ubd_kern.c
> index f354027..91f3354 100644
> --- a/arch/um/drivers/ubd_kern.c
> +++ b/arch/um/drivers/ubd_kern.c
> @@ -1,4 +1,5 @@
> /*
> + * Copyright (C) 2015-2016 Anton Ivanov (ai...@br...)
> * Copyright (C) 2000 Jeff Dike (jd...@ka...)
> * Licensed under the GPL
> */
> @@ -58,6 +59,17 @@ struct io_thread_req {
> int error;
> };
>
> +
> +static struct io_thread_req* (*irq_req_buffer) [];
> +static struct io_thread_req * irq_remainder;
> +static int irq_remainder_size;
> +
> +static struct io_thread_req* (*io_req_buffer) [];
> +static struct io_thread_req * io_remainder;
> +static int io_remainder_size;
> +
> +
> +
> static inline int ubd_test_bit(__u64 bit, unsigned char *data)
> {
> __u64 n;
> @@ -442,29 +454,88 @@ static void do_ubd_request(struct request_queue * q);
> static int thread_fd = -1;
> static LIST_HEAD(restart);
>
> -/* XXX - move this inside ubd_intr. */
> +/* Function to read several request pointers at a time
> +* handling fractional reads if (and as) needed
> +*/
> +
> +static int bulk_req_safe_read(
> + int fd,
> + struct io_thread_req* (*request_buffer) [],
> + struct io_thread_req * remainder,
This should be ** and the code modified accordingly - you need the
pointer to the "short read" buffer, not where the short read buffer is
pointing to as that is not relevant for what it tries to do.
> + int * remainder_size,
> + int max_recs
> + )
> +{
> + int n = 0;
> + int res = 0;
> +
> + if (*remainder_size > 0) {
> + memmove(
> + (char *) request_buffer,
> + (char *) & remainder, * remainder_size
> + );
> + n = * remainder_size;
> + }
> +
> + res = os_read_file(
> + fd,
> + ((char *) request_buffer) + * remainder_size,
> + sizeof(struct io_thread_req *) * max_recs - *remainder_size
> + );
> + if (res > 0) {
> + n += res;
> + if ((n % sizeof(struct io_thread_req *)) > 0) {
> + /*
> + * Read somehow returned not a multiple of dword
> + * theoretically possible, but never observed in the
> + * wild, so read routine must be able to handle it
> + */
> + *remainder_size = n % sizeof(struct io_thread_req *);
> + memmove(
> + & remainder,
> + ((char *) request_buffer) + n/sizeof(struct io_thread_req *),
> + *remainder_size
> + );
> + n = n - *remainder_size;
> + }
> + } else {
> + n = res;
> + }
> + return n;
> +}
> +
> /* Called without dev->lock held, and only in interrupt context. */
> static void ubd_handler(void)
> {
> - struct io_thread_req *req;
> struct ubd *ubd;
> struct list_head *list, *next_ele;
> unsigned long flags;
> int n;
> + int count;
>
> while(1){
> - n = os_read_file(thread_fd, &req,
> - sizeof(struct io_thread_req *));
> - if(n != sizeof(req)){
> + n = bulk_req_safe_read(
> + thread_fd,
> + irq_req_buffer,
> + irq_remainder,
> + &irq_remainder_size,
> + UBD_REQ_BUFFER_SIZE
> + );
> + if(n < 0){
> if(n == -EAGAIN)
> break;
> printk(KERN_ERR "spurious interrupt in ubd_handler, "
> "err = %d\n", -n);
> return;
> }
> -
> - blk_end_request(req->req, 0, req->length);
> - kfree(req);
> + for (count=0;count < n /sizeof(struct io_thread_req *);count++) {
> + blk_end_request(
> + (* irq_req_buffer)[count]->req,
> + 0,
> + (* irq_req_buffer)[count]->length
> + );
> + kfree((* irq_req_buffer)[count]);
> + }
> }
> reactivate_fd(thread_fd, UBD_IRQ);
>
> @@ -1064,6 +1135,28 @@ static int __init ubd_init(void)
> if (register_blkdev(fake_major, "ubd"))
> return -1;
> }
> +
> + irq_req_buffer = kmalloc(
> + sizeof(struct io_thread_req *) * UBD_REQ_BUFFER_SIZE,
> + GFP_KERNEL
> + );
> + irq_remainder = 0;
> +
> + if(irq_req_buffer == NULL) {
> + printk(KERN_ERR "Failed to initialize ubd buffering\n");
> + return -1;
> + }
> + io_req_buffer = kmalloc(
> + sizeof(struct io_thread_req *) * UBD_REQ_BUFFER_SIZE,
> + GFP_KERNEL
> + );
> +
> + io_remainder = 0;
> +
> + if(io_req_buffer == NULL) {
> + printk(KERN_ERR "Failed to initialize ubd buffering\n");
> + return -1;
> + }
> platform_driver_register(&ubd_driver);
> mutex_lock(&ubd_lock);
> for (i = 0; i < MAX_DEV; i++){
> @@ -1458,31 +1551,49 @@ static int io_count = 0;
>
> int io_thread(void *arg)
> {
> - struct io_thread_req *req;
> - int n;
> + int n, count, written, res;
>
> os_fix_helper_signals();
>
> while(1){
> - n = os_read_file(kernel_fd, &req,
> - sizeof(struct io_thread_req *));
> - if(n != sizeof(struct io_thread_req *)){
> - if(n < 0)
> + n = bulk_req_safe_read(
> + kernel_fd,
> + io_req_buffer,
> + io_remainder,
> + &io_remainder_size,
> + UBD_REQ_BUFFER_SIZE
> + );
> + if(n < 0){
> + if(n == -EAGAIN) {
> + ubd_read_poll(-1);
> + continue;
> + } else {
> printk("io_thread - read failed, fd = %d, "
> "err = %d\n", kernel_fd, -n);
> - else {
> - printk("io_thread - short read, fd = %d, "
> - "length = %d\n", kernel_fd, n);
> }
> - continue;
> + }
> +
> + for (count=0;count < n /sizeof(struct io_thread_req *);count++) {
> + io_count++;
> + do_io((* io_req_buffer)[count]);
> }
> - io_count++;
> - do_io(req);
> - n = os_write_file(kernel_fd, &req,
> - sizeof(struct io_thread_req *));
> - if(n != sizeof(struct io_thread_req *))
> - printk("io_thread - write failed, fd = %d, err = %d\n",
> - kernel_fd, -n);
> +
> + written = 0;
> +
> + do {
> + res = os_write_file(kernel_fd, ((char *) io_req_buffer) + written, n);
> + if (res > 0) {
> + written += res;
> + } else {
> + if (res != -EAGAIN) {
> + printk("io_thread - read failed, fd = %d, "
> + "err = %d\n", kernel_fd, -n);
> + }
> + }
> + if (written < n) {
> + ubd_write_poll(-1);
> + }
> + } while (written < n);
> }
>
> return 0;
> diff --git a/arch/um/drivers/ubd_user.c b/arch/um/drivers/ubd_user.c
> index e376f9b..2a7d5b6 100644
> --- a/arch/um/drivers/ubd_user.c
> +++ b/arch/um/drivers/ubd_user.c
> @@ -1,4 +1,5 @@
> -/*
> +/*
> + * Copyright (C) 2016 Anton Ivanov (ai...@br...)
> * Copyright (C) 2000, 2001, 2002 Jeff Dike (jd...@ka...)
> * Copyright (C) 2001 Ridgerun,Inc (gl...@ri...)
> * Licensed under the GPL
> @@ -20,6 +21,9 @@
>
> #include "ubd.h"
> #include <os.h>
> +#include <poll.h>
> +
> +struct pollfd kernel_pollfd;
>
> int start_io_thread(unsigned long sp, int *fd_out)
> {
> @@ -32,9 +36,12 @@ int start_io_thread(unsigned long sp, int *fd_out)
> }
>
> kernel_fd = fds[0];
> + kernel_pollfd.fd = kernel_fd;
> + kernel_pollfd.events = POLLIN;
> *fd_out = fds[1];
>
> err = os_set_fd_block(*fd_out, 0);
> + err = os_set_fd_block(kernel_fd, 0);
> if (err) {
> printk("start_io_thread - failed to set nonblocking I/O.\n");
> goto out_close;
> @@ -57,3 +64,13 @@ int start_io_thread(unsigned long sp, int *fd_out)
> out:
> return err;
> }
> +
> +int ubd_read_poll(int timeout) {
> + kernel_pollfd.events = POLLIN;
> + return poll(&kernel_pollfd, 1, timeout);
> +}
> +int ubd_write_poll(int timeout) {
> + kernel_pollfd.events = POLLOUT;
> + return poll(&kernel_pollfd, 1, timeout);
> +}
> +
|
