xen device driver updates [resend]

Hi,

[ Repost spelling Jeff's email addr properly. ]

This is a series of cleanups to xen-blkfront, xen-netfront and xenbus. They're mostly changes imported from the linux-2.6.18-xen repo because they looked like they would be useful.

Thanks,
J

 drivers/block/xen-blkfront.c       |   48 ++++++++++++++++++++++++++---
 drivers/net/xen-netfront.c         |    4 +-
 drivers/xen/xenbus/xenbus_client.c |    2 -
 drivers/xen/xenbus/xenbus_xs.c     |   10 +++---
 4 files changed, 52 insertions(+), 12 deletions(-)

From: Christian Limpach <[email protected]>

Return 0 instead of -EINVAL if the blkfront device is a cdrom, i.e. had the VDISK_CDROM attribute. This allows udev's cdrom_id to correctly detect the device as a cdrom device.

[ Add blkif_ioctl, and CDROMMULTISESSION ]

[ linux-2.6.18-xen changeset d2bd9af846b5 ]

---
 drivers/block/xen-blkfront.c |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

@@ -38,6 +38,7 @@
 #include <linux/interrupt.h>
 #include <linux/blkdev.h>
 #include <linux/hdreg.h>
+#include <linux/cdrom.h>
 #include <linux/module.h>
 
 #include <xen/xenbus.h>

@@ -150,6 +151,40 @@
 	hg->cylinders = cylinders;
 	if ((sector_t)(hg->cylinders + 1) * hg->heads * hg->sectors < nsect)
 		hg->cylinders = 0xffff;
+	return 0;
+}
+
+int blkif_ioctl(struct inode *inode, struct file *filep,
+		unsigned command, unsigned long argument)
+{
+	struct blkfront_info *info =
+		inode->i_bdev->bd_disk->private_data;
+	int i;
+
+	dev_dbg(&info->xbdev->dev, "command: 0x%x, argument: 0x%lx\n",
+		command, (long)argument);
+
+	switch (command) {
+	case CDROMMULTISESSION:
+		dev_dbg(&info->xbdev->dev, "FIXME: support multisession CDs later\n");
+		for (i = 0; i < sizeof(struct cdrom_multisession); i++)
+			if (put_user(0, (char __user *)(argument + i)))
+				return -EFAULT;
+		return 0;
+
+	case CDROM_GET_CAPABILITY: {
+		struct gendisk *gd = info->gd;
+		if (gd->flags & GENHD_FL_CD)
+			return 0;
+		return -EINVAL;
+	}
+
+	default:
+		/*printk(KERN_ALERT "ioctl %08x not supported by Xen blkdev\n",
+		  command);*/
+		return -EINVAL; /* same return as native Linux */
+	}
+
 	return 0;
 }
 

@@ -974,6 +1009,7 @@
 	.open = blkif_open,
 	.release = blkif_release,
 	.getgeo = blkif_getgeo,
+	.ioctl = blkif_ioctl,
 };
 
 

Unfortunately the Xen block driver interface doesn't seem to pass through any capabilities of the underlying CDROM device, so there's not much to return. I suspect this is primarily a hack to make installers a bit happier installing off a Xen VBD rather than because real programs will use this in earnest.

J

From: Ian Campbell <[email protected]>

[ linux-2.6.18-xen changeset 667228bf8fc5 ]

---
 drivers/block/xen-blkfront.c |    3 +++
 1 file changed, 3 insertions(+)

@@ -324,6 +324,9 @@
 	/* Make sure buffer addresses are sector-aligned. */
 	blk_queue_dma_alignment(rq, 511);
 
+	/* Make sure we don't use bounce buffers. */
+	blk_queue_bounce_limit(rq, BLK_BOUNCE_ANY);
+
 	gd->queue = rq;
 
 	return 0;

From: Wim Colgate <[email protected]>

[ linux-2.6.18-xen changeset c1c57fea77e9 ]

---
 drivers/block/xen-blkfront.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

@@ -997,7 +997,7 @@
 		struct xenbus_device *dev = info->xbdev;
 		enum xenbus_state state = xenbus_read_driver_state(dev->otherend);
 
-		if (state == XenbusStateClosing)
+		if (state == XenbusStateClosing && info->is_ready)
 			blkfront_closing(dev);
 	}
 	return 0;

From: Jan Beulich <[email protected]>

---
 drivers/block/xen-blkfront.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

@@ -1045,7 +1045,7 @@
 module_init(xlblk_init);
 
 
-static void xlblk_exit(void)
+static void __exit xlblk_exit(void)
 {
 	return xenbus_unregister_driver(&blkfront);
 }

From: Ian Campbell <[email protected]>

Avoid allocations causing swap activity on the resume path by allowing such allocations to access the emergency pools otherwise a save/restore/migration of a guest which is low on memory can deadlock.

[ linux-2.6.18-xen changesets e8b49cfbdac, fdb998e79aba ]

---
 drivers/block/xen-blkfront.c       |    5 +++--
 drivers/net/xen-netfront.c         |    4 ++--
 drivers/xen/xenbus/xenbus_client.c |    2 +-
 drivers/xen/xenbus/xenbus_xs.c     |   10 +++++-----
 4 files changed, 11 insertions(+), 10 deletions(-)

@@ -584,7 +584,7 @@
 
 	info->ring_ref = GRANT_INVALID_REF;
 
-	sring = (struct blkif_sring *)__get_free_page(GFP_KERNEL);
+	sring = (struct blkif_sring *)__get_free_page(GFP_KERNEL | __GFP_HIGH);
 	if (!sring) {
 		xenbus_dev_fatal(dev, -ENOMEM, "allocating shared ring");
 		return -ENOMEM;

@@ -741,7 +741,8 @@
 	int j;
 
 	/* Stage 1: Make a safe copy of the shadow state. */
-	copy = kmalloc(sizeof(info->shadow), GFP_KERNEL);
+	copy = kmalloc(sizeof(info->shadow),
+		       GFP_KERNEL | __GFP_NOFAIL | __GFP_HIGH);
 	if (!copy)
 		return -ENOMEM;
 	memcpy(copy, info->shadow, sizeof(info->shadow));

@@ -1324,7 +1324,7 @@
 		goto fail;
 	}
 
-	txs = (struct xen_netif_tx_sring *)get_zeroed_page(GFP_KERNEL);
+	txs = (struct xen_netif_tx_sring *)get_zeroed_page(GFP_KERNEL | __GFP_HIGH);
 	if (!txs) {
 		err = -ENOMEM;
 		xenbus_dev_fatal(dev, err, "allocating tx ring page");

@@ -1340,7 +1340,7 @@
 	}
 
 	info->tx_ring_ref = err;
-	rxs = (struct xen_netif_rx_sring *)get_zeroed_page(GFP_KERNEL);
+	rxs = (struct xen_netif_rx_sring *)get_zeroed_page(GFP_KERNEL | __GFP_HIGH);
 	if (!rxs) {
 		err = -ENOMEM;
 		xenbus_dev_fatal(dev, err, "allocating rx ring page");

@@ -117,7 +117,7 @@
 	char *path;
 
 	va_start(ap, pathfmt);
-	path = kvasprintf(GFP_KERNEL, pathfmt, ap);
+	path = kvasprintf(GFP_KERNEL | __GFP_HIGH, pathfmt, ap);
 	va_end(ap);
 
 	if (!path) {

@@ -283,9 +283,9 @@
 	char *buffer;
 
 	if (strlen(name) == 0)
-		buffer = kasprintf(GFP_KERNEL, "%s", dir);
+		buffer = kasprintf(GFP_KERNEL | __GFP_HIGH, "%s", dir);
 	else
-		buffer = kasprintf(GFP_KERNEL, "%s/%s", dir, name);
+		buffer = kasprintf(GFP_KERNEL | __GFP_HIGH, "%s/%s", dir, name);
 	return (!buffer) ? ERR_PTR(-ENOMEM) : buffer;
 }
 

@@ -297,7 +297,7 @@
 	*num = count_strings(strings, len);
 
 	/* Transfer to one big alloc for easy freeing. */
-	ret = kmalloc(*num * sizeof(char *) + len, GFP_KERNEL);
+	ret = kmalloc(*num * sizeof(char *) + len, GFP_KERNEL | __GFP_HIGH);
 	if (!ret) {
 		kfree(strings);
 		return ERR_PTR(-ENOMEM);

@@ -751,7 +751,7 @@
 	}
 
 
-	msg = kmalloc(sizeof(*msg), GFP_KERNEL);
+	msg = kmalloc(sizeof(*msg), GFP_KERNEL | __GFP_HIGH);
 	if (msg == NULL) {
 		err = -ENOMEM;
 		goto out;

@@ -763,7 +763,7 @@
 		goto out;
 	}
 
-	body = kmalloc(msg->hdr.len + 1, GFP_KERNEL);
+	body = kmalloc(msg->hdr.len + 1, GFP_KERNEL | __GFP_HIGH);
 	if (body == NULL) {
 		kfree(msg);
 		err = -ENOMEM;

IMO I am not qualified enough to ACK or NAK this VM-related patch... If others are happy with it, go ahead and merge the xen-netfront.c with that batch.

Yeah, I'm not quite sure about which GFP_ flags to use in what circumstance. In this case the potential for deadlock exists because this is the code which is reconnecting the virtual devices after a resume, which will probably include its storage (either block device, or some kind of network storage). Should GFP_NOFS also be in there? Something else? Does __GFP_HIGH necessarily mean that it won't try to do IO to push pages out?

Thanks,
J

Patch again for reference:

Subject: xen: Avoid allocations causing swap activity on the resume path

From: Ian Campbell <[email protected]>

Avoid allocations causing swap activity on the resume path by allowing such allocations to access the emergency pools otherwise a save/restore/migration of a guest which is low on memory can deadlock.

[ linux-2.6.18-xen changesets e8b49cfbdac, fdb998e79aba ]

---

drivers/block/xen-blkfront.c       |    5 +++--
drivers/net/xen-netfront.c         |    4 ++--
drivers/xen/xenbus/xenbus_client.c |    2 +-
drivers/xen/xenbus/xenbus_xs.c     |   10 +++++-----
4 files changed, 11 insertions(+), 10 deletions(-)

===================================================================

@@ -584,7 +584,7 @@
 

	info->ring_ref = GRANT_INVALID_REF;

- sring = (struct blkif_sring *)__get_free_page(GFP_KERNEL);

+	sring = (struct blkif_sring *)__get_free_page(GFP_KERNEL | __GFP_HIGH);
	if (!sring) {
		xenbus_dev_fatal(dev, -ENOMEM, "allocating shared ring");
		return -ENOMEM;
@@ -741,7 +741,8 @@
	int j;

	/* Stage 1: Make a safe copy of the shadow state. */
-	copy = kmalloc(sizeof(info->shadow), GFP_KERNEL);
+	copy = kmalloc(sizeof(info->shadow),
+		       GFP_KERNEL | __GFP_NOFAIL | __GFP_HIGH);
	if (!copy)
		return -ENOMEM;
	memcpy(copy, info->shadow, sizeof(info->shadow));

===================================================================

@@ -1324,7 +1324,7 @@

		goto fail;
	}

- txs = (struct xen_netif_tx_sring *)get_zeroed_page(GFP_KERNEL);

+	txs = (struct xen_netif_tx_sring *)get_zeroed_page(GFP_KERNEL | __GFP_HIGH);
	if (!txs) {
		err = -ENOMEM;
		xenbus_dev_fatal(dev, err, "allocating tx ring page");
@@ -1340,7 +1340,7 @@
	}

	info->tx_ring_ref = err;
-	rxs = (struct xen_netif_rx_sring *)get_zeroed_page(GFP_KERNEL);
+	rxs = (struct xen_netif_rx_sring *)get_zeroed_page(GFP_KERNEL | __GFP_HIGH);
	if (!rxs) {
		err = -ENOMEM;
		xenbus_dev_fatal(dev, err, "allocating rx ring page");

===================================================================

@@ -117,7 +117,7 @@

	char *path;

	va_start(ap, pathfmt);
-	path = kvasprintf(GFP_KERNEL, pathfmt, ap);
+	path = kvasprintf(GFP_KERNEL | __GFP_HIGH, pathfmt, ap);
	va_end(ap);

	if (!path) {

===================================================================

@@ -283,9 +283,9 @@

	char *buffer;

	if (strlen(name) == 0)

- buffer = kasprintf(GFP_KERNEL, "%s", dir);
+ buffer = kasprintf(GFP_KERNEL | __GFP_HIGH, "%s", dir);
else
- buffer = kasprintf(GFP_KERNEL, "%s/%s", dir, name);

+		buffer = kasprintf(GFP_KERNEL | __GFP_HIGH, "%s/%s", dir, name);
	return (!buffer) ? ERR_PTR(-ENOMEM) : buffer;
}

@@ -297,7 +297,7 @@

	*num = count_strings(strings, len);

	/* Transfer to one big alloc for easy freeing. */
-	ret = kmalloc(*num * sizeof(char *) + len, GFP_KERNEL);
+	ret = kmalloc(*num * sizeof(char *) + len, GFP_KERNEL | __GFP_HIGH);
	if (!ret) {
		kfree(strings);
		return ERR_PTR(-ENOMEM);
@@ -751,7 +751,7 @@
	}

- msg = kmalloc(sizeof(*msg), GFP_KERNEL);

+	msg = kmalloc(sizeof(*msg), GFP_KERNEL | __GFP_HIGH);
	if (msg == NULL) {
		err = -ENOMEM;
		goto out;
@@ -763,7 +763,7 @@
		goto out;
	}

- body = kmalloc(msg->hdr.len + 1, GFP_KERNEL);

+	body = kmalloc(msg->hdr.len + 1, GFP_KERNEL | __GFP_HIGH);
	if (body == NULL) {
		kfree(msg);
		err = -ENOMEM;

Nope.

__GFP_FS: may enter filesystems
__GFP_IO: may perform IO

__GFP_IO also means "may do swapout". Even when swap is on a regular file. This is because we do all the fs-related operations up-front during swapon. So at alloc_pages()-time we can go direct-to-disk-blocks.

So I assume for this application you'll need GFP_NOIO. That's still heaps better than GFP_ATOMIC, because it can sleep and wait for kswapd to do stuff, and it can reclaim clean pagecache and clean swapcache.

Whether you should also add __GFP_HIGH to cause the page allocation to bite harder into the page reserves is unclear to me, sorry.

OK, I'll respin with GFP_NOIO.

Well, if the allocation fails the machine is dead in the water, because it probably can't go on without its devices. I think it's worth eating to the reserves to avoid that. (I'll have a close look to only add it to allocations which are really unrecoverable if they fail.)

Thanks,
J

OK.

I don't actually have a clue what you're doing here. Restoring a virtual machine from disk/network or something like that?

It might be appropriate to do a big memory-reclaim before starting the operation, along the lines of suspend-to-disk - go off and allocate (and possible pin) sufficient memroy for the entire operation before actually starting it?

More or less. When you suspend the VM it gets disconnected from all its virtual devices. When you resume - which may be after a migration to another machine - it reconnects to all its devices again. These allocations are happening on the reconnection path, and so may happen before there's any underlying device to do IO to (in fact, they've been triggered by something trying to do IO, and the driver notices it has become disconnected and tries to reconnect itself).

Not really. Live migration - which is one instance of when this comes into play - is a very lightweight process from the guest VM's perspective, and may cause only ~100ms service interruption. Doing a mass of reclaim/IO would be expensive by comparison.

Also, in general the drivers have to be prepared to deal with a disconnection from their devices at any moment (ie, they may spontaneously disconnect for no apparent reason), so there's no opportunity to prepare for it.

Maybe it would be possible to scurry away some preallocated memory for use if the main allocation fails, but that seems logically equivalent to dipping into the global emergency pool.

J

I presume most of these should have been "To: Jens"? The drivers/net/ is really the only thing under my direct purview.

Comments to follow...

Jeff

Yep, sure. To be honest, I couldn't remember if you dealt with block patches or not. I knew that between you everything would find a home ;)

J