[PATCH 0/2] virtio: console: control queue race fixes

[PATCH 0/2] virtio: console: control queue race fixes

[Amit Shah]

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

[PATCH 1/2] virtio: console: Serialise control work

[Amit Shah]

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@google.com>
Signed-off-by: Amit Shah <amit.shah@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

Re: [PATCH 1/2] virtio: console: Serialise control work

[Rusty Russell]

On Fri,  6 Jan 2012 16:19:07 +0530, Amit Shah <amit.shah@redhat.com> wrote:
> 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.

Wait a sec... This might *work*, but it's not correct.  Remember that
disabling callbacks is a hint, and it's cetainly not synchronous.

You need something else to explicitly prevent reentry.

Unapplied,
Rusty.

Re: [PATCH 1/2] virtio: console: Serialise control work

[Amit Shah]

On (Mon) 09 Jan 2012 [13:03:59], Rusty Russell wrote:
> On Fri,  6 Jan 2012 16:19:07 +0530, Amit Shah <amit.shah@redhat.com> wrote:
> > 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.
> 
> Wait a sec... This might *work*, but it's not correct.  Remember that
> disabling callbacks is a hint, and it's cetainly not synchronous.
> 
> You need something else to explicitly prevent reentry.

OK, right.  I'll use the nrt workqueue to synchronise then.

		Amit

[PATCH 2/2] virtio: console: Disable callbacks for virtqueues at start of S4 freeze

[Amit Shah]

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@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

Re: [PATCH 0/2] virtio: console: control queue race fixes

[Rusty Russell]

On Fri,  6 Jan 2012 16:19:06 +0530, Amit Shah <amit.shah@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.