Re: [PATCH v2 1/1] Add timer to handle OOM situations

[Stefan Hajnoczi]

On Tue, Jul 26, 2016 at 04:28:21PM +0200, ggar...@abra.uab.cat wrote:
> @@ -493,6 +524,9 @@ static int vhost_vsock_dev_open(struct inode *inode, 
> struct file *file)
>   goto out;
>   }
>  
> + setup_timer(>tx_kick,
> + vhost_vsock_rehandle_tx_kick, (unsigned long) NULL);
> +
>   vqs[VSOCK_VQ_TX] = >vqs[VSOCK_VQ_TX];
>   vqs[VSOCK_VQ_RX] = >vqs[VSOCK_VQ_RX];
>   vsock->vqs[VSOCK_VQ_TX].handle_kick = vhost_vsock_handle_tx_kick;
> @@ -555,6 +589,7 @@ static int vhost_vsock_dev_release(struct inode *inode, 
> struct file *file)
>   vhost_dev_stop(>dev);
>   vhost_dev_cleanup(>dev, false);
>   kfree(vsock->dev.vqs);
> + del_timer(>tx_kick);

Please use del_timer_sync() so that we know the timer callback has
finished executing if it's racing with us.

Also please figure out the correct ordering of this call so
vhost_poll_queue() doesn't crash if the timer fires while we are
executing vhost_vsock_dev_release().  In other words, vq and vq->poll
must still be alive when we delete the timer, otherwise the callback
could fire an run on a vq that has already been cleaned up by
vhost_vsock_dev_release().


signature.asc
Description: PGP signature

[PATCH v2 1/1] Add timer to handle OOM situations

[ggarcia]

From: Gerard Garcia 

v2:
* Use of ERR_PTR/PTR_ERR/IS_ERR
* Timer cleaned on device release.
* Do not process more packets on error.

Signed-off-by: Gerard Garcia 
---
 drivers/vhost/vsock.c | 55 +--
 1 file changed, 45 insertions(+), 10 deletions(-)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index 17bfe4e..5c6879d 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -14,11 +14,13 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include "vhost.h"
 
 #define VHOST_VSOCK_DEFAULT_HOST_CID   2
+#define OOM_RETRY_MS   100
 
 enum {
VHOST_VSOCK_FEATURES = VHOST_FEATURES,
@@ -43,8 +45,12 @@ struct vhost_vsock {
u32 total_tx_buf;
 
u32 guest_cid;
+
+   struct timer_list tx_kick;
 };
 
+
+
 static u32 vhost_transport_get_local_cid(void)
 {
return VHOST_VSOCK_DEFAULT_HOST_CID;
@@ -282,12 +288,13 @@ vhost_vsock_alloc_pkt(struct vhost_virtqueue *vq,
 
if (in != 0) {
vq_err(vq, "Expected 0 input buffers, got %u\n", in);
-   return NULL;
+   return ERR_PTR(-EINVAL);
}
 
pkt = kzalloc(sizeof(*pkt), GFP_KERNEL);
-   if (!pkt)
-   return NULL;
+   if (!pkt){
+   return ERR_PTR(-ENOMEM);
+   }
 
len = iov_length(vq->iov, out);
iov_iter_init(_iter, WRITE, vq->iov, out, len);
@@ -297,7 +304,7 @@ vhost_vsock_alloc_pkt(struct vhost_virtqueue *vq,
vq_err(vq, "Expected %zu bytes for pkt->hdr, got %zu bytes\n",
   sizeof(pkt->hdr), nbytes);
kfree(pkt);
-   return NULL;
+   return ERR_PTR(-EINVAL);
}
 
if (le16_to_cpu(pkt->hdr.type) == VIRTIO_VSOCK_TYPE_STREAM)
@@ -310,13 +317,13 @@ vhost_vsock_alloc_pkt(struct vhost_virtqueue *vq,
/* The pkt is too big */
if (pkt->len > VIRTIO_VSOCK_MAX_PKT_BUF_SIZE) {
kfree(pkt);
-   return NULL;
+   return ERR_PTR(-EINVAL);
}
 
pkt->buf = kmalloc(pkt->len, GFP_KERNEL);
if (!pkt->buf) {
kfree(pkt);
-   return NULL;
+   return ERR_PTR(-EINVAL);
}
 
nbytes = copy_from_iter(pkt->buf, pkt->len, _iter);
@@ -324,7 +331,7 @@ vhost_vsock_alloc_pkt(struct vhost_virtqueue *vq,
vq_err(vq, "Expected %u byte payload, got %zu bytes\n",
   pkt->len, nbytes);
virtio_transport_free_pkt(pkt);
-   return NULL;
+   return ERR_PTR(-EINVAL);
}
 
return pkt;
@@ -362,9 +369,26 @@ static void vhost_vsock_handle_tx_kick(struct vhost_work 
*work)
}
 
pkt = vhost_vsock_alloc_pkt(vq, out, in);
-   if (!pkt) {
-   vq_err(vq, "Faulted on pkt\n");
-   continue;
+
+   if (IS_ERR(pkt)) {
+   if (PTR_ERR(pkt) == -ENOMEM) {
+   vhost_discard_vq_desc(vq, 1);
+
+   if (!timer_pending(>tx_kick)) {
+   vsock->tx_kick.data =
+   (unsigned long) vq;
+   vsock->tx_kick.expires =
+   jiffies + 
msecs_to_jiffies(OOM_RETRY_MS);
+   add_timer(>tx_kick);
+   }
+
+   break;
+   } else {
+   vq_err(vq, "Faulted on pkt\n");
+   break;
+   }
+   } else if (unlikely(timer_pending(>tx_kick))) {
+   del_timer(>tx_kick);
}
 
/* Only accept correctly addressed packets */
@@ -383,6 +407,13 @@ out:
mutex_unlock(>mutex);
 }
 
+static void vhost_vsock_rehandle_tx_kick(unsigned long data)
+{
+   struct vhost_virtqueue *vq = (struct vhost_virtqueue *) data;
+
+   vhost_poll_queue(>poll);
+}
+
 static void vhost_vsock_handle_rx_kick(struct vhost_work *work)
 {
struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
@@ -493,6 +524,9 @@ static int vhost_vsock_dev_open(struct inode *inode, struct 
file *file)
goto out;
}
 
+   setup_timer(>tx_kick,
+   vhost_vsock_rehandle_tx_kick, (unsigned long) NULL);
+
vqs[VSOCK_VQ_TX] = >vqs[VSOCK_VQ_TX];
vqs[VSOCK_VQ_RX] = >vqs[VSOCK_VQ_RX];
vsock->vqs[VSOCK_VQ_TX].handle_kick = vhost_vsock_handle_tx_kick;
@@ -555,6 +589,7 @@ static int vhost_vsock_dev_release(struct inode *inode, 
struct file *file)
vhost_dev_stop(>dev);
vhost_dev_cleanup(>dev, false);
kfree(vsock->dev.vqs);
+   del_timer(>tx_kick);