Re: [PATCH v4 2/3] virtio_balloon: introduce migration primitives to balloon pages

2012-07-18 Thread Rafael Aquini 
Howdy Andrew,

Thanks for taking the time to go through this work and provide me with such good
feedback.

On Wed, Jul 18, 2012 at 03:49:08PM -0700, Andrew Morton wrote:
> On Tue, 17 Jul 2012 13:50:42 -0300
> Rafael Aquini  wrote:
> 
> > Besides making balloon pages movable at allocation time and introducing
> > the necessary primitives to perform balloon page migration/compaction,
> > this patch also introduces the following locking scheme to provide the
> > proper synchronization and protection for struct virtio_balloon elements
> > against concurrent accesses due to parallel operations introduced by
> > memory compaction / page migration.
> >  - balloon_lock (mutex) : synchronizes the access demand to elements of
> >   struct virtio_balloon and its queue operations;
> >  - pages_lock (spinlock): special protection to balloon pages list against
> >   concurrent list handling operations;
> > 
> > ...
> >
> > +   balloon_mapping->a_ops = &virtio_balloon_aops;
> > +   balloon_mapping->backing_dev_info = (void *)vb;
> 
> hoo boy.  We're making page->mapping->backing_dev_info point at a
> struct which does not have type `struct backing_dev_info'.  And then we
> are exposing that page to core MM functions.  So we're hoping that core
> MM will never walk down page->mapping->backing_dev_info and explode.
> 
> That's nasty, hacky and fragile.

Shame on me, on this one.

Mea culpa: I took this approach, originally, because I stuck the spinlock within
the struct virtio_balloon and this was the easiest way to recover it just by
having the page pointer. I did this stupidity because on earlier stages of this
patch some functions that demanded access to that list spinlock were placed
outside the balloon driver's code -- this is a left-over
(I know, it's a total lame excuse, but it's the truth)

This is easily fixable, however, as the balloon page list spinlock is now only
being accessed within driver's code and it can be declared outside the struct.


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 2/3] virtio_balloon: introduce migration primitives to balloon pages

2012-07-18 Thread Andrew Morton 
On Tue, 17 Jul 2012 13:50:42 -0300
Rafael Aquini  wrote:

> Besides making balloon pages movable at allocation time and introducing
> the necessary primitives to perform balloon page migration/compaction,
> this patch also introduces the following locking scheme to provide the
> proper synchronization and protection for struct virtio_balloon elements
> against concurrent accesses due to parallel operations introduced by
> memory compaction / page migration.
>  - balloon_lock (mutex) : synchronizes the access demand to elements of
> struct virtio_balloon and its queue operations;
>  - pages_lock (spinlock): special protection to balloon pages list against
> concurrent list handling operations;
> 
> ...
>
> + balloon_mapping->a_ops = &virtio_balloon_aops;
> + balloon_mapping->backing_dev_info = (void *)vb;

hoo boy.  We're making page->mapping->backing_dev_info point at a
struct which does not have type `struct backing_dev_info'.  And then we
are exposing that page to core MM functions.  So we're hoping that core
MM will never walk down page->mapping->backing_dev_info and explode.

That's nasty, hacky and fragile.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH v4 2/3] virtio_balloon: introduce migration primitives to balloon pages

2012-07-17 Thread Rafael Aquini 
Besides making balloon pages movable at allocation time and introducing
the necessary primitives to perform balloon page migration/compaction,
this patch also introduces the following locking scheme to provide the
proper synchronization and protection for struct virtio_balloon elements
against concurrent accesses due to parallel operations introduced by
memory compaction / page migration.
 - balloon_lock (mutex) : synchronizes the access demand to elements of
  struct virtio_balloon and its queue operations;
 - pages_lock (spinlock): special protection to balloon pages list against
  concurrent list handling operations;

Signed-off-by: Rafael Aquini 
---
 drivers/virtio/virtio_balloon.c |  145 ---
 include/linux/virtio_balloon.h  |4 ++
 2 files changed, 141 insertions(+), 8 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 0908e60..7669bc8 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 
 /*
  * Balloon device works in 4K page units.  So each page is pointed to by
@@ -35,6 +36,9 @@
  */
 #define VIRTIO_BALLOON_PAGES_PER_PAGE (PAGE_SIZE >> VIRTIO_BALLOON_PFN_SHIFT)
 
+/* Synchronize accesses/updates to the struct virtio_balloon elements */
+DEFINE_MUTEX(balloon_lock);
+
 struct virtio_balloon
 {
struct virtio_device *vdev;
@@ -51,6 +55,10 @@ struct virtio_balloon
 
/* Number of balloon pages we've told the Host we're not using. */
unsigned int num_pages;
+
+   /* Protect 'pages' list against concurrent handling */
+   spinlock_t pages_lock;
+
/*
 * The pages we've told the Host we're not using.
 * Each page on this list adds VIRTIO_BALLOON_PAGES_PER_PAGE
@@ -125,10 +133,12 @@ static void fill_balloon(struct virtio_balloon *vb, 
size_t num)
/* We can only do one array worth at a time. */
num = min(num, ARRAY_SIZE(vb->pfns));
 
+   mutex_lock(&balloon_lock);
for (vb->num_pfns = 0; vb->num_pfns < num;
 vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
-   struct page *page = alloc_page(GFP_HIGHUSER | __GFP_NORETRY |
-   __GFP_NOMEMALLOC | __GFP_NOWARN);
+   struct page *page = alloc_page(GFP_HIGHUSER_MOVABLE |
+   __GFP_NORETRY | __GFP_NOWARN |
+   __GFP_NOMEMALLOC);
if (!page) {
if (printk_ratelimit())
dev_printk(KERN_INFO, &vb->vdev->dev,
@@ -141,7 +151,10 @@ static void fill_balloon(struct virtio_balloon *vb, size_t 
num)
set_page_pfns(vb->pfns + vb->num_pfns, page);
vb->num_pages += VIRTIO_BALLOON_PAGES_PER_PAGE;
totalram_pages--;
+   spin_lock(&vb->pages_lock);
list_add(&page->lru, &vb->pages);
+   page->mapping = balloon_mapping;
+   spin_unlock(&vb->pages_lock);
}
 
/* Didn't get any?  Oh well. */
@@ -149,6 +162,7 @@ static void fill_balloon(struct virtio_balloon *vb, size_t 
num)
return;
 
tell_host(vb, vb->inflate_vq);
+   mutex_unlock(&balloon_lock);
 }
 
 static void release_pages_by_pfn(const u32 pfns[], unsigned int num)
@@ -169,10 +183,22 @@ static void leak_balloon(struct virtio_balloon *vb, 
size_t num)
/* We can only do one array worth at a time. */
num = min(num, ARRAY_SIZE(vb->pfns));
 
+   mutex_lock(&balloon_lock);
for (vb->num_pfns = 0; vb->num_pfns < num;
 vb->num_pfns += VIRTIO_BALLOON_PAGES_PER_PAGE) {
+   /*
+* We can race against virtballoon_isolatepage() and end up
+* stumbling across a _temporarily_ empty 'pages' list.
+*/
+   spin_lock(&vb->pages_lock);
+   if (unlikely(list_empty(&vb->pages))) {
+   spin_unlock(&vb->pages_lock);
+   break;
+   }
page = list_first_entry(&vb->pages, struct page, lru);
+   page->mapping = NULL;
list_del(&page->lru);
+   spin_unlock(&vb->pages_lock);
set_page_pfns(vb->pfns + vb->num_pfns, page);
vb->num_pages -= VIRTIO_BALLOON_PAGES_PER_PAGE;
}
@@ -182,8 +208,11 @@ static void leak_balloon(struct virtio_balloon *vb, size_t 
num)
 * virtio_has_feature(vdev, VIRTIO_BALLOON_F_MUST_TELL_HOST);
 * is true, we *have* to do it in this order
 */
-   tell_host(vb, vb->deflate_vq);
-   release_pages_by_pfn(vb->pfns, vb->num_pfns);
+   if (vb->num_pfns > 0) {
+   tell_host(vb, vb->deflate_vq);
+   release_pages_by_pfn(vb->pfns, vb->num_pfns);
+   }
+