* [uml-devel] UBD Improvements (first 30%)
@ 2016-09-29 20:36 anton.ivanov
2016-09-29 20:36 ` [uml-devel] [PATCH] UBD Improvements Phase 1 anton.ivanov
0 siblings, 1 reply; 3+ messages in thread
From: anton.ivanov @ 2016-09-29 20:36 UTC (permalink / raw
To: user-mode-linux-devel
Hi Richard, hi list.
I had some difficulties evaluating the actual performance
difference because of the strange behaviour with the ubd speed
reduction over time (as per my earlier "There is something rotten"
email).
Overall, if I take the only the first few dd runs to read the
entire disk and compare like for like (first run without patch vs
first run with patch) I get 30% improvement on dd-ing a 16G sparse
ubd device.
The performance on find /usr -type f -exec cat {} > /dev/null \;
increases 17%.
The patch is called "Phase 1" for a reason - by bulking the requests
we set-up the scene for a later replacement of pread/pwrite with
preadv/pwritev in next series of patches which should increase the
speed further by at least the same amount (if not more).
------------------------------------------------------------------------------
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* [uml-devel] [PATCH] UBD Improvements Phase 1
2016-09-29 20:36 [uml-devel] UBD Improvements (first 30%) anton.ivanov
@ 2016-09-29 20:36 ` anton.ivanov
2016-09-30 6:53 ` Anton Ivanov
0 siblings, 1 reply; 3+ messages in thread
From: anton.ivanov @ 2016-09-29 20:36 UTC (permalink / raw
To: user-mode-linux-devel; +Cc: Anton Ivanov
From: Anton Ivanov <aivanov@brocade.com>
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 <aivanov@brocade.com>
---
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 (aivanov@brocade.com)
* Copyright (C) 2000 Jeff Dike (jdike@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,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,
+ 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 (aivanov@brocade.com)
* Copyright (C) 2000, 2001, 2002 Jeff Dike (jdike@karaya.com)
* Copyright (C) 2001 Ridgerun,Inc (glonnon@ridgerun.com)
* 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);
+}
+
--
2.1.4
------------------------------------------------------------------------------
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [uml-devel] [PATCH] UBD Improvements Phase 1
2016-09-29 20:36 ` [uml-devel] [PATCH] UBD Improvements Phase 1 anton.ivanov
@ 2016-09-30 6:53 ` Anton Ivanov
0 siblings, 0 replies; 3+ messages in thread
From: Anton Ivanov @ 2016-09-30 6:53 UTC (permalink / raw
To: user-mode-linux-devel
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, anton.ivanov@kot-begemot.co.uk wrote:
> From: Anton Ivanov <aivanov@brocade.com>
>
> 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 <aivanov@brocade.com>
> ---
> 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 (aivanov@brocade.com)
> * Copyright (C) 2000 Jeff Dike (jdike@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,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 (aivanov@brocade.com)
> * Copyright (C) 2000, 2001, 2002 Jeff Dike (jdike@karaya.com)
> * Copyright (C) 2001 Ridgerun,Inc (glonnon@ridgerun.com)
> * 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);
> +}
> +
------------------------------------------------------------------------------
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-09-30 6:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-29 20:36 [uml-devel] UBD Improvements (first 30%) anton.ivanov
2016-09-29 20:36 ` [uml-devel] [PATCH] UBD Improvements Phase 1 anton.ivanov
2016-09-30 6:53 ` Anton Ivanov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.