Re: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
On (Tue) 25 Sep 2012 [15:47:15], sjur.brandel...@stericsson.com wrote: From: Sjur Brændeland sjur.brandel...@stericsson.com This merge reduces code size by unifying the approach for sending scatter-lists and regular buffers. Any type of write operation (splice, write, put_chars) will now allocate a port_buffer and send_buf() and free_buf() can always be used. Thanks for this cleanup; I should've caught it at the review of the virtio-trace patchset itself -- sorry for that. Signed-off-by: Sjur Brændeland sjur.brandel...@stericsson.com cc: Rusty Russell ru...@rustcorp.com.au cc: Michael S. Tsirkin m...@redhat.com cc: Amit Shah amit.s...@redhat.com cc: Linus Walleij linus.wall...@linaro.org cc: Masami Hiramatsu masami.hiramatsu...@hitachi.com --- drivers/char/virtio_console.c | 141 ++--- 1 files changed, 62 insertions(+), 79 deletions(-) diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index 8ab9c3d..f4f7b04 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -111,6 +111,11 @@ struct port_buffer { size_t len; /* offset in the buf from which to consume data */ size_t offset; + + /* If sgpages == 0 then buf is used, else sg is used */ + unsigned int sgpages; + + struct scatterlist sg[1]; }; /* @@ -338,23 +343,46 @@ static inline bool use_multiport(struct ports_device *portdev) static void free_buf(struct port_buffer *buf) { + int i; unsigned int + kfree(buf-buf); buf-buf isn't set to NULL in case sgpages is 0. Please set buf-buf to NULL (and initialise other fields to default values) in alloc_buf() (and leave this as is). + + if (buf-sgpages) This 'if' is not necessary; just having the for loop will do the right thing. + for (i = 0; i buf-sgpages; i++) { + struct page *page = sg_page(buf-sg[i]); + if (!page) + break; + put_page(page); + } + kfree(buf); } -static struct port_buffer *alloc_buf(size_t buf_size) +static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size, + int nrbufs) Indentation is off. { struct port_buffer *buf; + size_t alloc_size; - buf = kmalloc(sizeof(*buf), GFP_KERNEL); + /* Allocate buffer and the scatter list */ + alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs; + buf = kmalloc(alloc_size, GFP_ATOMIC); This looks hacky, along with the 'struct scatterlist sg[1]' in port_buffer above. Use a pointer instead? At the least, please include a comment in struct port_buffer mentioning sg has to be the last element in the struct. if (!buf) goto fail; - buf-buf = kzalloc(buf_size, GFP_KERNEL); + + buf-sgpages = nrbufs; + if (nrbufs 0) + return buf; + + buf-buf = kmalloc(buf_size, GFP_ATOMIC); That's a lot of GFP_ATOMICS; even for the cases that don't need them. Maybe add a gfp param that only allocates GFP_ATOMIC memory from callers in interrupt context. All existing code got switched to using GFP_ATOMIC buffers as well, that's definitely not good. if (!buf-buf) goto free_buf; buf-len = 0; buf-offset = 0; buf-size = buf_size; + + /* Prepare scatter buffer for sending */ + sg_init_one(buf-sg, buf-buf, buf_size); return buf; free_buf: @@ -476,52 +504,25 @@ static ssize_t send_control_msg(struct port *port, unsigned int event, return 0; } -struct buffer_token { - union { - void *buf; - struct scatterlist *sg; - } u; - /* If sgpages == 0 then buf is used, else sg is used */ - unsigned int sgpages; -}; - -static void reclaim_sg_pages(struct scatterlist *sg, unsigned int nrpages) -{ - int i; - struct page *page; - - for (i = 0; i nrpages; i++) { - page = sg_page(sg[i]); - if (!page) - break; - put_page(page); - } - kfree(sg); -} /* Callers must take the port-outvq_lock */ static void reclaim_consumed_buffers(struct port *port) { - struct buffer_token *tok; + struct port_buffer *buf; unsigned int len; if (!port-portdev) { /* Device has been unplugged. vqs are already gone. */ return; } - while ((tok = virtqueue_get_buf(port-out_vq, len))) { - if (tok-sgpages) - reclaim_sg_pages(tok-u.sg, tok-sgpages); - else - kfree(tok-u.buf); - kfree(tok); + while ((buf = virtqueue_get_buf(port-out_vq, len))) { + free_buf(buf); port-outvq_full = false; } } -static ssize_t __send_to_port(struct port
RE: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
This merge reduces code size by unifying the approach for sending scatter-lists and regular buffers. Any type of write operation (splice, write, put_chars) will now allocate a port_buffer and send_buf() and free_buf() can always be used. Thanks! This looks much nicer and simpler. I just have some comments below. OK, good to hear that you agree to this kind of change. I'll do a respin of this patch fixing the issues you have pointed out. static void free_buf(struct port_buffer *buf) { + int i; + kfree(buf-buf); this should be done only when !buf-sgpages, or (see below) Agree, I'll put this statement in an else branch to the if-statement below. + + if (buf-sgpages) + for (i = 0; i buf-sgpages; i++) { + struct page *page = sg_page(buf-sg[i]); + if (!page) + break; + put_page(page); + } + kfree(buf); } -static struct port_buffer *alloc_buf(size_t buf_size) +static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size, +int nrbufs) { struct port_buffer *buf; + size_t alloc_size; - buf = kmalloc(sizeof(*buf), GFP_KERNEL); + /* Allocate buffer and the scatter list */ + alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs; This allocates one redundant sg entry when nrbuf 0, but I think it is OK. (just a comment) I did this on purpose for the sake of simplicity, but I can change this to something like: alloc_size = sizeof(*buf) + sizeof(buf-sg) * max(nrbufs - 1, 1); + buf = kmalloc(alloc_size, GFP_ATOMIC); This should be kzalloc(), or buf-buf and others are not initialized, which will cause unexpected kfree bug at kfree(buf-buf) in free_buf. Agree, kzalloc() is better in this case. if (!buf) goto fail; - buf-buf = kzalloc(buf_size, GFP_KERNEL); + + buf-sgpages = nrbufs; + if (nrbufs 0) + return buf; + + buf-buf = kmalloc(buf_size, GFP_ATOMIC); You can also use kzalloc here as previous code does. But if the reason why using kzalloc comes from the security, I think kmalloc is enough here, since the host can access all the guest pages anyway. With this new patch alloc_buf() is used both for both RX and TX. The out_vq did previously use malloc(). But I have preserved the legacy behavior for the in_vq by calling memset() in function fill_queue(). Thanks, Sjur ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
(2012/09/26 16:48), Sjur BRENDELAND wrote: -static struct port_buffer *alloc_buf(size_t buf_size) +static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size, +int nrbufs) { struct port_buffer *buf; + size_t alloc_size; - buf = kmalloc(sizeof(*buf), GFP_KERNEL); + /* Allocate buffer and the scatter list */ + alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs; This allocates one redundant sg entry when nrbuf 0, but I think it is OK. (just a comment) I did this on purpose for the sake of simplicity, but I can change this to something like: alloc_size = sizeof(*buf) + sizeof(buf-sg) * max(nrbufs - 1, 1); You wouldn't need to change that. I think current code is enough simple and reasonable. :) Thanks! -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
Sjur BRENDELAND sjur.brandel...@stericsson.com writes: This allocates one redundant sg entry when nrbuf 0, but I think it is OK. (just a comment) I did this on purpose for the sake of simplicity, but I can change this to something like: alloc_size = sizeof(*buf) + sizeof(buf-sg) * max(nrbufs - 1, 1); That's why we use [0] in the definition (a GCC extension). Cheers, Rusty. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
(2012/09/25 22:47), sjur.brandel...@stericsson.com wrote: From: Sjur Brændeland sjur.brandel...@stericsson.com This merge reduces code size by unifying the approach for sending scatter-lists and regular buffers. Any type of write operation (splice, write, put_chars) will now allocate a port_buffer and send_buf() and free_buf() can always be used. Thanks! This looks much nicer and simpler. I just have some comments below. Signed-off-by: Sjur Brændeland sjur.brandel...@stericsson.com cc: Rusty Russell ru...@rustcorp.com.au cc: Michael S. Tsirkin m...@redhat.com cc: Amit Shah amit.s...@redhat.com cc: Linus Walleij linus.wall...@linaro.org cc: Masami Hiramatsu masami.hiramatsu...@hitachi.com --- drivers/char/virtio_console.c | 141 ++--- 1 files changed, 62 insertions(+), 79 deletions(-) diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index 8ab9c3d..f4f7b04 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -111,6 +111,11 @@ struct port_buffer { size_t len; /* offset in the buf from which to consume data */ size_t offset; + + /* If sgpages == 0 then buf is used, else sg is used */ + unsigned int sgpages; + + struct scatterlist sg[1]; }; /* @@ -338,23 +343,46 @@ static inline bool use_multiport(struct ports_device *portdev) static void free_buf(struct port_buffer *buf) { + int i; + kfree(buf-buf); this should be done only when !buf-sgpages, or (see below) + + if (buf-sgpages) + for (i = 0; i buf-sgpages; i++) { + struct page *page = sg_page(buf-sg[i]); + if (!page) + break; + put_page(page); + } + kfree(buf); } -static struct port_buffer *alloc_buf(size_t buf_size) +static struct port_buffer *alloc_buf(struct virtqueue *vq, size_t buf_size, + int nrbufs) { struct port_buffer *buf; + size_t alloc_size; - buf = kmalloc(sizeof(*buf), GFP_KERNEL); + /* Allocate buffer and the scatter list */ + alloc_size = sizeof(*buf) + sizeof(struct scatterlist) * nrbufs; This allocates one redundant sg entry when nrbuf 0, but I think it is OK. (just a comment) + buf = kmalloc(alloc_size, GFP_ATOMIC); This should be kzalloc(), or buf-buf and others are not initialized, which will cause unexpected kfree bug at kfree(buf-buf) in free_buf. if (!buf) goto fail; - buf-buf = kzalloc(buf_size, GFP_KERNEL); + + buf-sgpages = nrbufs; + if (nrbufs 0) + return buf; + + buf-buf = kmalloc(buf_size, GFP_ATOMIC); You can also use kzalloc here as previous code does. But if the reason why using kzalloc comes from the security, I think kmalloc is enough here, since the host can access all the guest pages anyway. if (!buf-buf) goto free_buf; buf-len = 0; buf-offset = 0; buf-size = buf_size; + + /* Prepare scatter buffer for sending */ + sg_init_one(buf-sg, buf-buf, buf_size); return buf; free_buf: Thank you, -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu...@hitachi.com ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization