2009-12-10 22:35:27

by Adam Litke

Subject: [PATCH] virtio: Fix scheduling while atomic in virtio_balloon stats

[PATCH] virtio: Fix scheduling while atomic in virtio_balloon stats

This is a fix for my earlier patch: "virtio: Add memory statistics reporting to
the balloon driver (V4)".

I discovered that all_vm_events() can sleep and therefore stats collection
cannot be done in interrupt context. One solution is to handle the interrupt
by noting that stats need to be collected and waking the existing vballoon
kthread which will complete the work via stats_handle_request(). Rusty, is
this a saner way of doing business?

There is one issue that I would like a broader opinion on. In stats_request, I
update vb->need_stats_update and then wake up the kthread. The kthread uses
vb->need_stats_update as a condition variable. Do I need a memory barrier
between the update and wake_up to ensure that my kthread sees the correct
value? My testing suggests that it is not needed but I would like some
confirmation from the experts.

Signed-off-by: Adam Litke <[email protected]>
To: Rusty Russell <[email protected]>
Cc: Anthony Liguori <[email protected]>
Cc: [email protected]

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index b45d946..c4a6437 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -52,6 +52,7 @@ struct virtio_balloon
u32 pfns[256];

/* Memory statistics */
+ int need_stats_update;
struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
};

@@ -194,20 +195,30 @@ static void update_balloon_stats(struct virtio_balloon *vb)
* the stats queue operates in reverse. The driver initializes the virtqueue
* with a single buffer. From that point forward, all conversations consist of
* a hypervisor request (a call to this function) which directs us to refill
- * the virtqueue with a fresh stats buffer.
+ * the virtqueue with a fresh stats buffer. Since stats collection can sleep,
+ * we notify our kthread which does the actual work via stats_handle_request().
*/
-static void stats_ack(struct virtqueue *vq)
+static void stats_request(struct virtqueue *vq)
{
struct virtio_balloon *vb;
unsigned int len;
- struct scatterlist sg;

vb = vq->vq_ops->get_buf(vq, &len);
if (!vb)
return;
+ vb->need_stats_update = 1;
+ wake_up(&vb->config_change);
+}
+
+static void stats_handle_request(struct virtio_balloon *vb)
+{
+ struct virtqueue *vq;
+ struct scatterlist sg;

+ vb->need_stats_update = 0;
update_balloon_stats(vb);

+ vq = vb->stats_vq;
sg_init_one(&sg, vb->stats, sizeof(vb->stats));
if (vq->vq_ops->add_buf(vq, &sg, 1, 0, vb) < 0)
BUG();
@@ -250,8 +261,11 @@ static int balloon(void *_vballoon)
try_to_freeze();
wait_event_interruptible(vb->config_change,
(diff = towards_target(vb)) != 0
+ || vb->need_stats_update
|| kthread_should_stop()
|| freezing(current));
+ if (vb->need_stats_update)
+ stats_handle_request(vb);
if (diff > 0)
fill_balloon(vb, diff);
else if (diff < 0)
@@ -265,7 +279,7 @@ static int virtballoon_probe(struct virtio_device *vdev)
{
struct virtio_balloon *vb;
struct virtqueue *vqs[3];
- vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_ack };
+ vq_callback_t *callbacks[] = { balloon_ack, balloon_ack, stats_request };
const char *names[] = { "inflate", "deflate", "stats" };
int err, nvqs;



--
Thanks,
Adam

2009-12-14 04:26:13

by Rusty Russell

Subject: Re: [PATCH] virtio: Fix scheduling while atomic in virtio_balloon stats

On Fri, 11 Dec 2009 09:05:15 am Adam Litke wrote:
> [PATCH] virtio: Fix scheduling while atomic in virtio_balloon stats
>
> This is a fix for my earlier patch: "virtio: Add memory statistics reporting to
> the balloon driver (V4)".
>
> I discovered that all_vm_events() can sleep and therefore stats collection
> cannot be done in interrupt context. One solution is to handle the interrupt
> by noting that stats need to be collected and waking the existing vballoon
> kthread which will complete the work via stats_handle_request(). Rusty, is
> this a saner way of doing business?

I think so.

> There is one issue that I would like a broader opinion on. In stats_request, I
> update vb->need_stats_update and then wake up the kthread. The kthread uses
> vb->need_stats_update as a condition variable. Do I need a memory barrier
> between the update and wake_up to ensure that my kthread sees the correct
> value? My testing suggests that it is not needed but I would like some
> confirmation from the experts.

Yep:

* It may be assumed that this function implies a write memory barrier before
* changing the task state if and only if any tasks are woken up.

> +static void stats_handle_request(struct virtio_balloon *vb)
> +{
> + struct virtqueue *vq;
> + struct scatterlist sg;
>
> + vb->need_stats_update = 0;
> update_balloon_stats(vb);

And this works without a barrier because they can't make another request
before we finish with this one.

Applied,
Rusty.