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

[Xen-changelog] [xen-unstable] Tmem: fix domain refcount leak by returni

To: xen-changelog@xxxxxxxxxxxxxxxxxxx
Subject: [Xen-changelog] [xen-unstable] Tmem: fix domain refcount leak by returning to simpler model
From: Xen patchbot-unstable <patchbot-unstable@xxxxxxxxxxxxxxxxxxx>
Date: Fri, 11 Jun 2010 07:25:16 -0700
Delivery-date: Fri, 11 Jun 2010 07:26:26 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
List-help: <mailto:xen-changelog-request@lists.xensource.com?subject=help>
List-id: BK change log <xen-changelog.lists.xensource.com>
List-post: <mailto:xen-changelog@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-changelog>, <mailto:xen-changelog-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-changelog>, <mailto:xen-changelog-request@lists.xensource.com?subject=unsubscribe>
Reply-to: xen-devel@xxxxxxxxxxxxxxxxxxx
Sender: xen-changelog-bounces@xxxxxxxxxxxxxxxxxxx
# HG changeset patch
# User Keir Fraser <keir.fraser@xxxxxxxxxx>
# Date 1276204356 -3600
# Node ID 39657b16806826f88947b9882884374f3ed15c30
# Parent  6d35ded36a79198efe8dcf0d7ea53dcf2324e365
Tmem: fix domain refcount leak by returning to simpler model
which claims a ref once when the tmem client is first associated
with the domain, and puts it once when the tmem client is
destroyed.

Signed-off-by: Dan Magenheimer <dan.magenheimer@xxxxxxxxxx>
---
 xen/common/tmem.c          |   50 ++++++++-------------------------------------
 xen/include/xen/tmem_xen.h |   19 +++++++++--------
 2 files changed, 20 insertions(+), 49 deletions(-)

diff -r 6d35ded36a79 -r 39657b168068 xen/common/tmem.c
--- a/xen/common/tmem.c Thu Jun 10 22:11:26 2010 +0100
+++ b/xen/common/tmem.c Thu Jun 10 22:12:36 2010 +0100
@@ -1916,8 +1916,6 @@ static NOINLINE int do_tmem_new_pool(cli
                     client->pools[d_poolid] = global_shared_pools[s_poolid];
                     shared_pool_join(global_shared_pools[s_poolid], client);
                     pool_free(pool);
-                    if ( this_cli_id != CLI_ID_NULL )
-                        tmh_client_put(client->tmh);
                     return d_poolid;
                 }
             }
@@ -1938,8 +1936,6 @@ static NOINLINE int do_tmem_new_pool(cli
         }
     }
     client->pools[d_poolid] = pool;
-    if ( this_cli_id != CLI_ID_NULL )
-        tmh_client_put(client->tmh);
     list_add_tail(&pool->pool_list, &global_pool_list);
     pool->pool_id = d_poolid;
     pool->persistent = persistent;
@@ -1949,8 +1945,6 @@ static NOINLINE int do_tmem_new_pool(cli
 
 fail:
     pool_free(pool);
-    if ( this_cli_id != CLI_ID_NULL )
-        tmh_client_put(client->tmh);
     return -EPERM;
 }
 
@@ -1976,7 +1970,6 @@ static int tmemc_freeze_pools(cli_id_t c
         if ( (client = tmh_client_from_cli_id(cli_id)) == NULL)
             return -1;
         client_freeze(client,freeze);
-        tmh_client_put(client->tmh);
         printk("tmem: all pools %s for %s=%d\n",s,cli_id_str,cli_id);
     }
     return 0;
@@ -2185,10 +2178,8 @@ static int tmemc_list(cli_id_t cli_id, t
     }
     else if ( (client = tmh_client_from_cli_id(cli_id)) == NULL)
         return -1;
-    else {
+    else
         off = tmemc_list_client(client, buf, 0, len, use_long);
-        tmh_client_put(client->tmh);
-    }
 
     return 0;
 }
@@ -2243,10 +2234,7 @@ static int tmemc_set_var(cli_id_t cli_id
     else if ( (client = tmh_client_from_cli_id(cli_id)) == NULL)
         return -1;
     else
-    {
         tmemc_set_var_one(client, subop, arg1);
-        tmh_client_put(client->tmh);
-    }
     return 0;
 }
 
@@ -2272,7 +2260,6 @@ static NOINLINE int tmemc_shared_pool_au
             if ( auth == 0 )
                 client->shared_auth_uuid[i][0] =
                     client->shared_auth_uuid[i][1] = -1L;
-            tmh_client_put(client->tmh);
             return 1;
         }
         if ( (auth == 1) && (client->shared_auth_uuid[i][0] == -1L) &&
@@ -2280,15 +2267,11 @@ static NOINLINE int tmemc_shared_pool_au
             free = i;
     }
     if ( auth == 0 )
-    {
-        tmh_client_put(client->tmh);
         return 0;
-    }
     if ( auth == 1 && free == -1 )
         return -ENOMEM;
     client->shared_auth_uuid[free][0] = uuid_lo;
     client->shared_auth_uuid[free][1] = uuid_hi;
-    tmh_client_put(client->tmh);
     return 1;
 }
 
@@ -2370,8 +2353,6 @@ static NOINLINE int tmemc_save_subop(int
         client->frozen = client->was_frozen;
         rc = 0;
     }
-    if ( client )
-        tmh_client_put(client->tmh);
     return rc;
 }
 
@@ -2387,15 +2368,9 @@ static NOINLINE int tmemc_save_get_next_
     unsigned int pagesize = 1 << (pool->pageshift+12);
 
     if ( pool == NULL || is_ephemeral(pool) )
-    {
-        tmh_client_put(client->tmh);
         return -1;
-    }
     if ( bufsize < pagesize + sizeof(struct tmem_handle) )
-    {
-        tmh_client_put(client->tmh);
         return -ENOMEM;
-    }
 
     tmem_spin_lock(&pers_lists_spinlock);
     if ( list_empty(&pool->persistent_page_list) )
@@ -2427,7 +2402,6 @@ static NOINLINE int tmemc_save_get_next_
 
 out:
     tmem_spin_unlock(&pers_lists_spinlock);
-    tmh_client_put(client->tmh);
     return ret;
 }
 
@@ -2442,10 +2416,7 @@ static NOINLINE int tmemc_save_get_next_
     if ( client == NULL )
         return 0;
     if ( bufsize < sizeof(struct tmem_handle) )
-    {
-        tmh_client_put(client->tmh);
         return 0;
-    }
     tmem_spin_lock(&pers_lists_spinlock);
     if ( list_empty(&client->persistent_invalidated_list) )
         goto out;
@@ -2472,7 +2443,6 @@ static NOINLINE int tmemc_save_get_next_
     ret = 1;
 out:
     tmem_spin_unlock(&pers_lists_spinlock);
-    tmh_client_put(client->tmh);
     return ret;
 }
 
@@ -2482,11 +2452,10 @@ static int tmemc_restore_put_page(int cl
     client_t *client = tmh_client_from_cli_id(cli_id);
     pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
                    ? NULL : client->pools[pool_id];
-    int rc = pool ? do_tmem_put(pool,oid,index,0,0,0,bufsize,buf.p) : -1;
-
-    if ( client )
-        tmh_client_put(client->tmh);
-    return rc;
+
+    if ( pool == NULL )
+        return -1;
+    return do_tmem_put(pool,oid,index,0,0,0,bufsize,buf.p);
 }
 
 static int tmemc_restore_flush_page(int cli_id, int pool_id, uint64_t oid,
@@ -2495,11 +2464,10 @@ static int tmemc_restore_flush_page(int 
     client_t *client = tmh_client_from_cli_id(cli_id);
     pool_t *pool = (client == NULL || pool_id >= MAX_POOLS_PER_DOMAIN)
                    ? NULL : client->pools[pool_id];
-    int rc = pool ? do_tmem_flush_page(pool, oid, index) : -1;
-
-    if ( client )
-        tmh_client_put(client->tmh);
-    return rc;
+
+    if ( pool == NULL )
+        return -1;
+    return do_tmem_flush_page(pool,oid,index);
 }
 
 static NOINLINE int do_tmem_control(struct tmem_op *op)
diff -r 6d35ded36a79 -r 39657b168068 xen/include/xen/tmem_xen.h
--- a/xen/include/xen/tmem_xen.h        Thu Jun 10 22:11:26 2010 +0100
+++ b/xen/include/xen/tmem_xen.h        Thu Jun 10 22:12:36 2010 +0100
@@ -302,18 +302,18 @@ extern tmh_client_t *tmh_client_init(cli
 extern tmh_client_t *tmh_client_init(cli_id_t);
 extern void tmh_client_destroy(tmh_client_t *);
 
-/* this appears to be unreliable when a domain is being shut down */
+/* we don't need to take a reference to the domain here as we hold
+ * one for the entire life of the client, so use rcu_lock_domain_by_id
+ * variant instead of get_domain_by_id() */
 static inline struct client *tmh_client_from_cli_id(cli_id_t cli_id)
 {
-    struct domain *d = get_domain_by_id(cli_id); /* incs d->refcnt! */
+    struct client *c;
+    struct domain *d = rcu_lock_domain_by_id(cli_id);
     if (d == NULL)
         return NULL;
-    return (struct client *)(d->tmem);
-}
-
-static inline void tmh_client_put(tmh_client_t *tmh)
-{
-    put_domain(tmh->domain);
+    c = (struct client *)(d->tmem);
+    rcu_unlock_domain(d);
+    return c;
 }
 
 static inline struct client *tmh_client_from_current(void)
@@ -336,6 +336,9 @@ static inline void tmh_set_client_from_i
 static inline void tmh_set_client_from_id(struct client *client,
                                           tmh_client_t *tmh, cli_id_t cli_id)
 {
+    /* here we DO want to take/hold a reference to the domain as
+     * this routine should be called exactly once when the client is created;
+     * the matching put_domain is in tmh_client_destroy */
     struct domain *d = get_domain_by_id(cli_id);
     d->tmem = client;
     tmh->domain = d;

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

<Prev in Thread] Current Thread [Next in Thread>
  • [Xen-changelog] [xen-unstable] Tmem: fix domain refcount leak by returning to simpler model, Xen patchbot-unstable <=