[PATCH 0 of 5] xen device driver updates [resend]

[PATCH 0 of 5] xen device driver updates [resend]

[Jeremy Fitzhardinge]

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(-)

[PATCH 1 of 5] xen/blkfront: Make sure we don't use bounce buffers, we don't need them

[Jeremy Fitzhardinge]

From: Ian Campbell <ian.campbell@xensource.com>

[ linux-2.6.18-xen changeset 667228bf8fc5 ]

Signed-off-by: Ian Campbell <ian.campbell@xensource.com>
Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 drivers/block/xen-blkfront.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -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;

Re: [PATCH 1 of 5] xen/blkfront: Make sure we don't use bounce buffers, we don't need them

[Jeff Garzik]

Jeremy Fitzhardinge wrote:
> From: Ian Campbell <ian.campbell@xensource.com>
> 
> [ linux-2.6.18-xen changeset 667228bf8fc5 ]
> 
> Signed-off-by: Ian Campbell <ian.campbell@xensource.com>
> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> ---
>  drivers/block/xen-blkfront.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -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;

ACK

[PATCH 2 of 5] xn/blkfront: Add the CDROM_GET_CAPABILITY ioctl to blkfront

[Jeremy Fitzhardinge]

From: Christian Limpach <Christian.Limpach@xensource.com>

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 ]

Signed-off-by: Christian Limpach <Christian.Limpach@xensource.com>
Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 drivers/block/xen-blkfront.c |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -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,
 };
 
 

Re: [PATCH 2 of 5] xn/blkfront: Add the CDROM_GET_CAPABILITY ioctl to blkfront

[Jeff Garzik]

Jeremy Fitzhardinge wrote:
> +	case CDROM_GET_CAPABILITY: {
> +		struct gendisk *gd = info->gd;
> +		if (gd->flags & GENHD_FL_CD)
> +			return 0;
> +		return -EINVAL;
> +	}


Surely you can come up with a better implementation than this?  It's 
just a bitmask returned to userspace...

	Jeff

Re: [PATCH 2 of 5] xn/blkfront: Add the CDROM_GET_CAPABILITY ioctl to blkfront

[Jeremy Fitzhardinge]

Jeff Garzik wrote:
> Jeremy Fitzhardinge wrote:
>> +    case CDROM_GET_CAPABILITY: {
>> +        struct gendisk *gd = info->gd;
>> +        if (gd->flags & GENHD_FL_CD)
>> +            return 0;
>> +        return -EINVAL;
>> +    }
>
>
> Surely you can come up with a better implementation than this?  It's 
> just a bitmask returned to userspace...

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

[PATCH 3 of 5] xen/blkfront: Make sure that the device is fully ready before allowing release

[Jeremy Fitzhardinge]

From: Wim Colgate <wim@xensource.com>

[ linux-2.6.18-xen changeset c1c57fea77e9 ]

Signed-off-by: Wim Colgate <wim@xensource.com>
Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

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

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -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;

[PATCH 4 of 5] xen/blkfront: add __exit to module_exit() handlers

[Jeremy Fitzhardinge]

From: Jan Beulich <jbeulich@novell.com>

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
---
 drivers/block/xen-blkfront.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1045,7 +1045,7 @@
 module_init(xlblk_init);
 
 
-static void xlblk_exit(void)
+static void __exit xlblk_exit(void)
 {
 	return xenbus_unregister_driver(&blkfront);
 }

Re: [PATCH 4 of 5] xen/blkfront: add __exit to module_exit() handlers

[Jeff Garzik]

Jeremy Fitzhardinge wrote:
> From: Jan Beulich <jbeulich@novell.com>
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> ---
>  drivers/block/xen-blkfront.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -1045,7 +1045,7 @@
>  module_init(xlblk_init);
>  
>  
> -static void xlblk_exit(void)
> +static void __exit xlblk_exit(void)
>  {
>  	return xenbus_unregister_driver(&blkfront);
>  }

ACK

[PATCH 5 of 5] xen: Avoid allocations causing swap activity on the resume path

[Jeremy Fitzhardinge]

From: Ian Campbell <ian.campbell@citrix.com>

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 ]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

---
 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(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -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));
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -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");
diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -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) {
diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -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;

Re: [PATCH 5 of 5] xen: Avoid allocations causing swap activity on the resume path

[Jeff Garzik]

Jeremy Fitzhardinge wrote:
> From: Ian Campbell <ian.campbell@citrix.com>
> 
> 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 ]
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> 
> ---
>  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(-)

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.

Re: [PATCH 5 of 5] xen: Avoid allocations causing swap activity on the resume path

[Jeremy Fitzhardinge]

Jeff Garzik wrote:
> Jeremy Fitzhardinge wrote:
>> From: Ian Campbell <ian.campbell@citrix.com>
>>
>> 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 ]
>>
>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>>
>> ---
>>  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(-)
>
> IMO I am not qualified enough to ACK or NAK this VM-related patch...

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 <ian.campbell@citrix.com>

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 ]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

---
 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(-)

===================================================================
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -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));
===================================================================
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -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");
===================================================================
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -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) {
===================================================================
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -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;

Re: [PATCH 5 of 5] xen: Avoid allocations causing swap activity on the resume path

[Andrew Morton]

On Sat, 31 May 2008 10:50:40 +0100 Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Does __GFP_HIGH necessarily mean that it won't try to 
> do IO to push pages out?

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.

Re: [PATCH 5 of 5] xen: Avoid allocations causing swap activity on the resume path

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> On Sat, 31 May 2008 10:50:40 +0100 Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>   
>> Does __GFP_HIGH necessarily mean that it won't try to 
>> do IO to push pages out?
>>     
>
> 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.
>   

OK, I'll respin with GFP_NOIO.

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

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

Re: [PATCH 5 of 5] xen: Avoid allocations causing swap activity on the resume path

[Andrew Morton]

On Sat, 31 May 2008 11:10:18 +0100 Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> > Whether you should also add __GFP_HIGH to cause the page allocation to
> > bite harder into the page reserves is unclear to me, sorry.
> >   
> 
> 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.)

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?

Re: [PATCH 5 of 5] xen: Avoid allocations causing swap activity on the resume path

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> On Sat, 31 May 2008 11:10:18 +0100 Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>
>   
>>> Whether you should also add __GFP_HIGH to cause the page allocation to
>>> bite harder into the page reserves is unclear to me, sorry.
>>>   
>>>       
>> 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.)
>>     
>
> OK.
>
> I don't actually have a clue what you're doing here.  Restoring a
> virtual machine from disk/network or something like that?
>   

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).

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

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

Re: [PATCH 0 of 5] xen device driver updates [resend]

[Jeff Garzik]

Jeremy Fitzhardinge wrote:
> 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(-)

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

Re: [PATCH 0 of 5] xen device driver updates [resend]

[Jeremy Fitzhardinge]

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

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

[PATCH 0 of 5] xen device driver updates

[Jeremy Fitzhardinge]

Hi,

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(-)

[PATCH 5 of 5] xen: Avoid allocations causing swap activity on the resume path

[Jeremy Fitzhardinge]

From: Ian Campbell <ian.campbell@citrix.com>

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 ]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>

---
 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(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -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));
diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -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");
diff --git a/drivers/xen/xenbus/xenbus_client.c b/drivers/xen/xenbus/xenbus_client.c
--- a/drivers/xen/xenbus/xenbus_client.c
+++ b/drivers/xen/xenbus/xenbus_client.c
@@ -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) {
diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
--- a/drivers/xen/xenbus/xenbus_xs.c
+++ b/drivers/xen/xenbus/xenbus_xs.c
@@ -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;