WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] [PATCH 03/17] Fix a long-standing memory leak in the grant t

To: <xen-devel@xxxxxxxxxxxxxxxxxxx>
Subject: [Xen-devel] [PATCH 03/17] Fix a long-standing memory leak in the grant tables implementation.
From: <steven.smith@xxxxxxxxxx>
Date: Sun, 4 Oct 2009 16:04:19 +0100
Cc: Steven Smith <steven.smith@xxxxxxxxxx>, keir.frasier@xxxxxxxxxx, jeremy@xxxxxxxx, joserenato.santos@xxxxxx
Delivery-date: Sun, 04 Oct 2009 08:22:02 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <cover.1254667618.git.ssmith@xxxxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <cover.1254667618.git.ssmith@xxxxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
According to the interface comments, gnttab_end_foreign_access() is
supposed to free the page once the grant is no longer in use, from a
polling timer, but that was never implemented.  Implement it.

This shouldn't make any real difference, because the existing drivers
all arrange that with well-behaved backends references are never ended
while they're still in use, but it tidies things up a bit.

Signed-off-by: Steven Smith <steven.smith@xxxxxxxxxx>
---
 drivers/xen/grant-table.c |  103 +++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 94 insertions(+), 9 deletions(-)

diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index cd82d22..52183aa 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -51,11 +51,17 @@
 #define GNTTAB_LIST_END 0xffffffff
 #define GREFS_PER_GRANT_FRAME (PAGE_SIZE / sizeof(struct grant_entry))
 
+static void pending_free_timer(unsigned long ignore);
+
 static grant_ref_t **gnttab_list;
 static unsigned int nr_grant_frames;
 static unsigned int boot_max_nr_grant_frames;
 static int gnttab_free_count;
 static grant_ref_t gnttab_free_head;
+static grant_ref_t gnttab_pending_free_gref_head = GNTTAB_LIST_END;
+static LIST_HEAD(gnttab_pending_free_pages);
+static DEFINE_TIMER(gnttab_delayed_free_timer, pending_free_timer, 0, 0);
+static DEFINE_SPINLOCK(gnttab_pending_free_lock);
 static DEFINE_SPINLOCK(gnttab_list_lock);
 
 static struct grant_entry *shared;
@@ -191,35 +197,114 @@ int gnttab_query_foreign_access(grant_ref_t ref)
 }
 EXPORT_SYMBOL_GPL(gnttab_query_foreign_access);
 
-int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly)
+static int _gnttab_end_foreign_access_ref(grant_ref_t ref)
 {
        u16 flags, nflags;
 
        nflags = shared[ref].flags;
        do {
                flags = nflags;
-               if (flags & (GTF_reading|GTF_writing)) {
-                       printk(KERN_ALERT "WARNING: g.e. still in use!\n");
+               if (flags & (GTF_reading|GTF_writing))
                        return 0;
-               }
        } while ((nflags = sync_cmpxchg(&shared[ref].flags, flags, 0)) != 
flags);
 
        return 1;
 }
+
+int gnttab_end_foreign_access_ref(grant_ref_t ref, int readonly)
+{
+       int r;
+
+       r = _gnttab_end_foreign_access_ref(ref);
+       if (!r)
+               printk(KERN_ALERT "WARNING: g.e. still in use!\n");
+       return r;
+}
 EXPORT_SYMBOL_GPL(gnttab_end_foreign_access_ref);
 
+static void pending_free_timer(unsigned long ignore)
+{
+       grant_ref_t gref, next_gref;
+       grant_ref_t prev; /* The last gref which we failed to release,
+                            or GNTTAB_LIST_END if there is no such
+                            gref. */
+       int need_mod_timer;
+       struct page *page, *next_page;
+
+       spin_lock(&gnttab_pending_free_lock);
+       prev = GNTTAB_LIST_END;
+       for (gref = gnttab_pending_free_gref_head;
+            gref != GNTTAB_LIST_END;
+            gref = next_gref) {
+               next_gref = gnttab_entry(gref);
+               if (_gnttab_end_foreign_access_ref(gref)) {
+                       put_free_entry(gref);
+                       if (prev != GNTTAB_LIST_END)
+                               gnttab_entry(prev) = next_gref;
+                       else
+                               gnttab_pending_free_gref_head = next_gref;
+               } else {
+                       prev = gref;
+               }
+       }
+       list_for_each_entry_safe(page, next_page,
+                                &gnttab_pending_free_pages, lru) {
+               gref = page->index;
+               if (_gnttab_end_foreign_access_ref(gref)) {
+                       list_del(&page->lru);
+                       put_free_entry(gref);
+                       /* The page hasn't been used in this domain
+                          for more than a second, so it's probably
+                          cold. */
+                       if (put_page_testzero(page)) {
+#ifdef MODULE
+                               __free_page(page);
+#else
+                               free_cold_page(page);
+#endif
+                       }
+               }
+       }
+
+       need_mod_timer =
+               (gnttab_pending_free_gref_head != GNTTAB_LIST_END) ||
+               !list_empty(&gnttab_pending_free_pages);
+       spin_unlock(&gnttab_pending_free_lock);
+
+       if (need_mod_timer)
+               mod_timer(&gnttab_delayed_free_timer, jiffies + HZ);
+}
+
 void gnttab_end_foreign_access(grant_ref_t ref, int readonly,
                               unsigned long page)
 {
-       if (gnttab_end_foreign_access_ref(ref, readonly)) {
+       int need_mod_timer;
+       struct page *page_struct;
+
+       if (_gnttab_end_foreign_access_ref(ref)) {
                put_free_entry(ref);
                if (page != 0)
                        free_page(page);
        } else {
-               /* XXX This needs to be fixed so that the ref and page are
-                  placed on a list to be freed up later. */
-               printk(KERN_WARNING
-                      "WARNING: leaking g.e. and page still in use!\n");
+               spin_lock_bh(&gnttab_pending_free_lock);
+               if (page == 0) {
+                       if (gnttab_pending_free_gref_head == GNTTAB_LIST_END)
+                               need_mod_timer = 1;
+                       else
+                               need_mod_timer = 0;
+                       gnttab_entry(ref) = gnttab_pending_free_gref_head;
+                       gnttab_pending_free_gref_head = ref;
+               } else {
+                       need_mod_timer =
+                               list_empty(&gnttab_pending_free_pages);
+                       page_struct = virt_to_page((void *)page);
+                       page_struct->index = ref;
+                       list_add_tail(&page_struct->lru,
+                                     &gnttab_pending_free_pages);
+               }
+               spin_unlock_bh(&gnttab_pending_free_lock);
+               if (need_mod_timer)
+                       mod_timer(&gnttab_delayed_free_timer, jiffies + HZ);
        }
 }
 EXPORT_SYMBOL_GPL(gnttab_end_foreign_access);
-- 
1.6.3.1


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel