Re: [PATCH 10/10] virtio: console: Add support for nonblocking write()s

[Amit Shah <amit.shah@redhat.com>]

On (Tue) Apr 06 2010 [12:42:58], Rusty Russell wrote:
> On Mon, 5 Apr 2010 11:24:14 pm Amit Shah wrote:
> > If the host port is not open, a write() should either just return if the
> > file is opened in non-blocking mode, or block till the host port is
> > opened.
> > 
> > Also, don't spin till host consumes data for nonblocking ports. For
> > non-blocking ports, we can do away with the spinning and reclaim the
> > buffers consumed by the host on the next write call or on the condition
> > that'll make poll return.
> 
> I'm only reading the patch so I might have missed it, but what's the
> locking going on here?
> 
> Can we race thinking we're full or not full incorrectly?

Turns out just this much on top of this patch should be sufficient.


diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 0c632ee..6541f76 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -158,6 +158,9 @@ struct port {
 	 */
 	spinlock_t inbuf_lock;
 
+	/* Protect the operations on the out_vq. */
+	spinlock_t outbuf_lock;
+
 	/* The IO vqs for this port */
 	struct virtqueue *in_vq, *out_vq;
 
@@ -409,12 +412,15 @@ static ssize_t send_control_msg(struct port *port, unsigned int event,
 static void reclaim_consumed_buffers(struct port *port)
 {
 	void *buf;
+	unsigned long flags;
 	unsigned int len;
 
+	spin_lock_irqsave(&port->outbuf_lock, flags);
 	while ((buf = port->out_vq->vq_ops->get_buf(port->out_vq, &len))) {
 		kfree(buf);
 		port->outvq_full = false;
 	}
+	spin_unlock_irqrestore(&port->outbuf_lock, flags);
 }
 
 static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
@@ -423,6 +429,7 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
 	struct scatterlist sg[1];
 	struct virtqueue *out_vq;
 	ssize_t ret;
+	unsigned long flags;
 	unsigned int len;
 
 	out_vq = port->out_vq;
@@ -430,6 +437,9 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
 	reclaim_consumed_buffers(port);
 
 	sg_init_one(sg, in_buf, in_count);
+
+	spin_lock_irqsave(&port->outbuf_lock, flags);
+
 	ret = out_vq->vq_ops->add_buf(out_vq, sg, 1, 0, in_buf);
 
 	/* Tell Host to go! */
@@ -437,14 +447,14 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
 
 	if (ret < 0) {
 		in_count = 0;
-		goto fail;
+		goto done;
 	}
 
 	if (ret == 0)
 		port->outvq_full = true;
 
 	if (nonblock)
-		return in_count;
+		goto done;
 
 	/*
 	 * Wait till the host acknowledges it pushed out the data we
@@ -454,7 +464,8 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count,
 	 */
 	while (!out_vq->vq_ops->get_buf(out_vq, &len))
 		cpu_relax();
-fail:
+done:
+	spin_unlock_irqrestore(&port->outbuf_lock, flags);
 	/*
 	 * We're expected to return the amount of data we wrote -- all
 	 * of it
@@ -1009,6 +1020,7 @@ static int add_port(struct ports_device *portdev, u32 id)
 	}
 
 	spin_lock_init(&port->inbuf_lock);
+	spin_lock_init(&port->outbuf_lock);
 	init_waitqueue_head(&port->waitqueue);
 
 	/* Fill the in_vq with buffers so the host can send us data. */