[uml-devel] [PATCH v3] UBD Improvements Phase 1
From: Anton Ivanov
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
---
arch/um/drivers/ubd.h | 5 ++
arch/um/drivers/ubd_kern.c | 167 ++---
arch/um/drivers/ubd_user.c | 21 +-
3 files changed, 166 insertions(+), 27 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..7007ee3 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 (aiva...@brocade.com)
* Copyright (C) 2000 Jeff Dike (jd...@karaya.com)
* 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,90 @@ 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,
+ 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)[cou
Re: [uml-devel] [PATCH v3] UBD Improvements Phase 1
Hi
I am not clear on the remainder/remainder_size would not a single offset
parameter work? rather than copying it off and back?
also max_recs does not seem to used except to calculate max buffer size so
would not using a buffer size be easier?
something like bulk_req_read( int fd, struct io_thread_req *buf, size_t len,
size_t *offset)
+static int bulk_req_safe_read(
+ int fd,
+ struct io_thread_req * (*request_buffer)[],
+ struct io_thread_req **remainder,
+ 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
then here it would be
res = os_read_file(fd, buf+*offset,len-*offset)
Note that if this is ever multithreaded buf and offset or
request_buffer/remainder/remainder_size need to be kept separate
+ );
+ 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
+ */
maybe this should have a WARN_ON since it should never occur?
+ *remainder_size = n % sizeof(struct io_thread_req *);
+ memmove(
+ remainder,
+ ((char *) request_buffer) +
+ n/sizeof(struct io_thread_req *),
+ *remainder_size
I am pretty sure the 2nd parameter is off. I think it should be:
((char *) request_buffer) +(n/sizeof(struct io_thread_req *))*sizeof(struct
io_thread_req *)
also copying it off to a shared static buffer?
+ );
+ 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)){
if it is being rewritten n could just be the number of complete buffers for the
for loop below
+ 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 +1137,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;
+ }
Ok, I am really not tracking this, the same buffer allocated twice?
+ 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;
+
Re: [uml-devel] [PATCH v3] UBD Improvements Phase 1
On 09/11/16 16:08, James McMechan wrote:
> Hi
>
> I am not clear on the remainder/remainder_size would not a single offset
> parameter work? rather than copying it off and back?
Possible.
The other alternative is to copy the remainder (if it exists) to the
beginning of the buffer at the end of a read and keep only the offset
around instead of keeping two params around.
You can no longer reuse the whole read function in both the interrupt
handler and the io thread though.
The reason is that you can copy the remainder to the beginning of the
buffer (so you can use only offset as a single param) only after you
have "consumed" all the records.
> also max_recs does not seem to used except to calculate max buffer size so
> would not using a buffer size be easier?
> something like bulk_req_read( int fd, struct io_thread_req *buf, size_t len,
> size_t *offset)
>
> +static int bulk_req_safe_read(
> + int fd,
> + struct io_thread_req * (*request_buffer)[],
> + struct io_thread_req **remainder,
> + 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
>
> then here it would be
> res = os_read_file(fd, buf+*offset,len-*offset)
> Note that if this is ever multithreaded buf and offset or
> request_buffer/remainder/remainder_size need to be kept separate
This is executed only in the single IO thread.
>
> + );
> + 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
> + */
>
> maybe this should have a WARN_ON since it should never occur?
Fair point.
>
> + *remainder_size = n % sizeof(struct io_thread_req *);
> + memmove(
> + remainder,
> + ((char *) request_buffer) +
> + n/sizeof(struct io_thread_req *),
> + *remainder_size
>
> I am pretty sure the 2nd parameter is off. I think it should be:
> ((char *) request_buffer) +(n/sizeof(struct io_thread_req *))*sizeof(struct
> io_thread_req *)
> also copying it off to a shared static buffer?
Well spotted.
>
> + );
> + 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)){
>
> if it is being rewritten n could just be the number of complete buffers for
> the for loop below
That will make it more readable. Agree.
>
> + 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 +1137,
[uml-devel] [PATCH v4] UBD Improvements
From: Anton Ivanov
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
---
arch/um/drivers/ubd.h | 5 ++
arch/um/drivers/ubd_kern.c | 168 ++---
arch/um/drivers/ubd_user.c | 21 +-
3 files changed, 167 insertions(+), 27 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..8541027 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 (aiva...@brocade.com)
* Copyright (C) 2000 Jeff Dike (jd...@karaya.com)
* 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,91 @@ 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,
+ 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 *);
+ WARN(*remainder_size > 0, "UBD IPC read returned a
partial result");
+ memmove(
+ remainder,
+ ((char *) request_buffer) +
+ (n/sizeof(struct io_thread_req
*))*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(
+
