Linux Virtualization - Jan 2012 - [PATCH 0/2] virtio: console: control queue race fixes

Hello,

The first patch here fixes the race seen by Miche.  He hasn't yet
reported back if this fixes the races he saw, but Joy Pu from Red Hat
tested this patch with hot-plugging/unplugging ports in a loop.
Before this patch, he saw some freezes as well as sysfs warnings.
After applying the patch, all was well.

The second patch can be folded into the series fixing S4 for
virtio-console.  It ensures all callbacks are disabled in the freeze
function.  This patch has a dependency on the 1st one in the sense the
double disabling of vq won't make sense w/o patch 1.

Please review and apply.


Amit Shah (2):
  virtio: console: Serialise control work
  virtio: console: Disable callbacks for virtqueues at start of S4
    freeze

 drivers/char/virtio_console.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

-- 
1.7.7.4

We currently allow multiple instances of the control work handler to run
in parallel.  This isn't expected to work; serialise access by disabling
interrupts on new packets from the Host and enable them when all the
existing ones are consumed.

Testcase: hot-plug/unplug a port in a loop.  Without this patch, some
sysfs warnings are seen along with freezes.  With the patch, all works
fine.

Reported-by: Miche Baker-Harvey <miche at google.com>
Signed-off-by: Amit Shah <amit.shah at redhat.com>
---
 drivers/char/virtio_console.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index fd2fd6f..fc54a79 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1471,6 +1471,7 @@ static void control_work_handler(struct work_struct *work)
 	portdev = container_of(work, struct ports_device, control_work);
 	vq = portdev->c_ivq;
 
+ start:
 	spin_lock(&portdev->cvq_lock);
 	while ((buf = virtqueue_get_buf(vq, &len))) {
 		spin_unlock(&portdev->cvq_lock);
@@ -1488,6 +1489,10 @@ static void control_work_handler(struct work_struct
*work)
 		}
 	}
 	spin_unlock(&portdev->cvq_lock);
+	if (unlikely(!virtqueue_enable_cb(vq))) {
+		virtqueue_disable_cb(vq);
+		goto start;
+	}
 }
 
 static void out_intr(struct virtqueue *vq)
@@ -1538,6 +1543,7 @@ static void control_intr(struct virtqueue *vq)
 {
 	struct ports_device *portdev;
 
+	virtqueue_disable_cb(vq);
 	portdev = vq->vdev->priv;
 	schedule_work(&portdev->control_work);
 }
-- 
1.7.7.4

To ensure we don't receive any more interrupts from the host after we
enter the freeze function, disable all vq interrupts.

There wasn't any problem seen due to this in tests, but applying this
patch makes the freeze case more robust.

Signed-off-by: Amit Shah <amit.shah at redhat.com>
---
 drivers/char/virtio_console.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index fc54a79..c1c45d8 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1860,10 +1860,18 @@ static int virtcons_freeze(struct virtio_device *vdev)
 
 	vdev->config->reset(vdev);
 
+	virtqueue_disable_cb(portdev->c_ivq);
 	cancel_work_sync(&portdev->control_work);
+	/*
+	 * Once more: if control_work_handler() was running, it would
+	 * enable the cb as the last step.
+	 */
+	virtqueue_disable_cb(portdev->c_ivq);
 	remove_controlq_data(portdev);
 
 	list_for_each_entry(port, &portdev->ports, list) {
+		virtqueue_disable_cb(port->in_vq);
+		virtqueue_disable_cb(port->out_vq);
 		/*
 		 * We'll ask the host later if the new invocation has
 		 * the port opened or closed.
-- 
1.7.7.4

On Fri,  6 Jan 2012 16:19:06 +0530, Amit Shah <amit.shah at redhat.com>
wrote:
> Hello,
> 
> The first patch here fixes the race seen by Miche.  He hasn't yet
> reported back if this fixes the races he saw, but Joy Pu from Red Hat
> tested this patch with hot-plugging/unplugging ports in a loop.
> Before this patch, he saw some freezes as well as sysfs warnings.
> After applying the patch, all was well.

So, the first one should be cc: stable?
> The second patch can be folded into the series fixing S4 for
> virtio-console.  It ensures all callbacks are disabled in the freeze
> function.  This patch has a dependency on the 1st one in the sense the
> double disabling of vq won't make sense w/o patch 1.
> 
> Please review and apply.
Done, thanks.

Cheers,
Rusty.