Re: [PATCH 1/3] virtio_console:Merge struct buffer_token into struct port_buffer

(2012/09/25 22:47), sjur.brandeland@stericsson.com wrote:

From: Sjur Brændeland <redacted>

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.

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.pt@hitachi.com


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

quoted

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.

quoted

 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.

quoted

+
+	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);

quoted

+	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. 

quoted

 	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

(2012/09/26 16:48), Sjur BRENDELAND wrote:

quoted

quoted

-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.pt@hitachi.com

Sjur BRENDELAND [off-list ref] writes:

quoted

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.

On (Wed) 26 Sep 2012 [09:48:12], Sjur BRENDELAND wrote:

quoted

quoted

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.

quoted

quoted

 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.

quoted

quoted

-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);

quoted

quoted

+	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.

quoted

quoted

 	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