On (Wed) Dec 02 2009 [14:14:20], Rusty Russell wrote:

On Sat, 28 Nov 2009 05:20:35 pm Amit Shah wrote:

quoted

The console could be flooded with data from the host; handle
this situation by buffering the data.

All this complexity makes me really wonder if we should just
have the host say the max # ports it will ever use, and just do this
really dumbly.  Yes, it's a limitation, but it'd be much simpler.

As in make sure the max nr ports is less than 255 and have per-port vqs?
And then the buffering will be done inside the vqs themselves?

quoted

--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c

@@ -65,6 +65,23 @@ struct ports_device {
 	 * interrupt
 	 */
 	struct work_struct rx_work;
+
+	struct list_head unused_read_head;

You should name lists after plurals, rather than using "head" which is
an implementation detail.  eg. "queued_inbufs" and below "used_inbufs".

OK.

Though Shirly Ma was working on a "destroy_bufs" patch which would avoid
your need for this list at all, AFAICT.

quoted

+		/* Return the number of bytes actually copied */
+		ret = copy_size;
+		buf->offset += ret;
+		out_offset += ret;
+		out_count -= ret;

We don't actually use ret.

In a later patch, when copy_to_user is added, ret will be used. So I
kept it this way to reduce the noise in the diffs later.

quoted

+		if (buf->len - buf->offset == 0) {

I prefer the simpler "if (buf->offset == buf->len)" myself.

Will update.

quoted

+			spin_lock_irqsave(&port->readbuf_list_lock, flags);
+			list_del(&buf->list);
+			spin_unlock_irqrestore(&port->readbuf_list_lock, flags);
+			kfree(buf->buf);
+			kfree(buf);

Does it become cleaner later to have this in a separate function?  Usually
I prefer matching alloc and free fns.

Adding a function is easy, sure. I should've done that though; something
that got overlooked.

quoted

+static struct port_buffer *get_buf(size_t buf_size)
+{
+	struct port_buffer *buf;
+
+	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
+	if (!buf)
+		goto out;
+	buf->buf = kzalloc(buf_size, GFP_KERNEL);
+	if (!buf->buf) {
+		kfree(buf);
+		goto out;

No, that would return non-NULL.  I'd stick with the standard multi-part exit:

	if (!buf)
		goto fail;
	buf->buf = kzalloc(buf_size, GFP_KERNEL);
	if (!buf->buf)
		goto fail_free_buf;
	buf->len = buf_size;
	return buf;

fail_free_buf:
	kfree(buf);
fail:
	return NULL;

Ow, indeed.

Thanks! 

		Amit