[Xen-devel] [PATCH 04/22] Fix a long-standing memory leak in the grant tables implementation. 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.

[<steven.smith@xxxxxxxxxx>]

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/core/gnttab.c |  100 ++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 94 insertions(+), 6 deletions(-)

diff --git a/drivers/xen/core/gnttab.c b/drivers/xen/core/gnttab.c
index 9a8fc89..dd93f43 100644
--- a/drivers/xen/core/gnttab.c
+++ b/drivers/xen/core/gnttab.c
@@ -55,11 +55,17 @@
 #define GNTTAB_LIST_END 0xffffffff
 #define ENTRIES_PER_GRANT_FRAME (PAGE_SIZE / sizeof(grant_entry_t))
 
+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;
@@ -180,7 +186,7 @@ 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)
+static int _gnttab_end_foreign_access_ref(grant_ref_t ref)
 {
        u16 flags, nflags;
 
@@ -195,19 +201,101 @@ int gnttab_end_foreign_access_ref(grant_ref_t ref)
 
        return 1;
 }
+
+int gnttab_end_foreign_access_ref(grant_ref_t ref)
+{
+       int r;
+
+       r = _gnttab_end_foreign_access_ref(ref);
+       if (!r)
+               printk(KERN_DEBUG "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, unsigned long page)
 {
-       if (gnttab_end_foreign_access_ref(ref)) {
+       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_DEBUG
-                      "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