Re: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
On (Wed) 26 Sep 2012 [09:48:12], Sjur BRENDELAND wrote:
> > > 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.
Not necessary; see my comments in another mail.
> > > -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.
Not really -- one thing I've picked up from Rusty is to use kmallocs
and explicitly initialise values. Then, missing such initialisations
(like in this case) will cause tools like valgrind to show the error
pretty quickly.
> > > 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().
But we're dropping the memset/kzalloc anyway.
Amit
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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
>
> 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
> cc: Rusty Russell
> cc: Michael S. Tsirkin
> cc: Amit Shah
> cc: Linus Walleij
> cc: Masami Hiramatsu
> ---
> 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(>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([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, ))) {
> - 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, ))) {
> + free_buf(buf);
> port->outvq_full = false;
> }
> }
>
> -static ssize_t __send_to_port(struct port *port, struct
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
On (Wed) 26 Sep 2012 [09:48:12], Sjur BRENDELAND wrote:
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.
Not necessary; see my comments in another mail.
-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.
Not really -- one thing I've picked up from Rusty is to use kmallocs
and explicitly initialise values. Then, missing such initialisations
(like in this case) will cause tools like valgrind to show the error
pretty quickly.
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().
But we're dropping the memset/kzalloc anyway.
Amit
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
Sjur BRENDELAND 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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(>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
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
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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
>
> 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
> cc: Rusty Russell
> cc: Michael S. Tsirkin
> cc: Amit Shah
> cc: Linus Walleij
> cc: Masami Hiramatsu
> ---
> 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(>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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
From: Sjur Brændeland
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.
Signed-off-by: Sjur Brændeland
cc: Rusty Russell
cc: Michael S. Tsirkin
cc: Amit Shah
cc: Linus Walleij
cc: Masami Hiramatsu
---
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);
+
+ if (buf->sgpages)
+ for (i = 0; i < buf->sgpages; i++) {
+ struct page *page = sg_page(>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;
+ buf = kmalloc(alloc_size, GFP_ATOMIC);
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);
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([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, ))) {
- 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, ))) {
+ free_buf(buf);
port->outvq_full = false;
}
}
-static ssize_t __send_to_port(struct port *port, struct scatterlist *sg,
- int nents, size_t in_count,
- struct buffer_token *tok, bool nonblock)
+static ssize_t send_buf(struct port *port, struct port_buffer *buf, int nents,
+ size_t in_count, bool nonblock)
{
struct virtqueue *out_vq;
ssize_t ret;
@@ -534,7 +535,7 @@ static ssize_t __send_to_port(struct port *port, struct
scatterlist *sg,
reclaim_consumed_buffers(port);
- ret = virtqueue_add_buf(out_vq, sg, nents, 0, tok, GFP_ATOMIC);
+ ret = virtqueue_add_buf(out_vq, buf->sg, nents, 0, buf, GFP_ATOMIC);
/* Tell Host to go! */
virtqueue_kick(out_vq);
@@ -559,8 +560,11 @@ static ssize_t __send_to_port(struct port *port, struct
scatterlist *sg,
* we need to kmalloc a GFP_ATOMIC buffer each time the
* console driver writes something out.
*/
- while (!virtqueue_get_buf(out_vq, ))
+ for (buf = virtqueue_get_buf(out_vq, ); !buf;
+buf = virtqueue_get_buf(out_vq, ))
cpu_relax();
+
+
[PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer
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.
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);
+
+ 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;
+ buf = kmalloc(alloc_size, GFP_ATOMIC);
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);
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 *port, struct scatterlist *sg,
- int nents, size_t in_count,
- struct buffer_token *tok, bool nonblock)
+static ssize_t send_buf(struct port *port, struct port_buffer *buf, int nents,
+ size_t in_count, bool nonblock)
{
struct virtqueue *out_vq;
ssize_t ret;
@@ -534,7 +535,7 @@ static ssize_t __send_to_port(struct port *port, struct
scatterlist *sg,
reclaim_consumed_buffers(port);
- ret = virtqueue_add_buf(out_vq, sg, nents, 0, tok, GFP_ATOMIC);
+ ret = virtqueue_add_buf(out_vq, buf-sg, nents, 0, buf, GFP_ATOMIC);
/* Tell Host to go! */
virtqueue_kick(out_vq);
@@ -559,8 +560,11 @@ static ssize_t __send_to_port(struct port *port, struct
scatterlist *sg,
* we need to kmalloc a GFP_ATOMIC buffer each time the
* console driver writes something out.
*/
- while
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
