Bug in tmem: refcount leak leaves zombie saved domains

Bug in tmem: refcount leak leaves zombie saved domains

[Keir Fraser]

Dan,

Just doing some save/restore testing on xen-unstable tip, I noticed that:
# xm create ./pv_config
# xm save PV1

Would leave the saved guest as a zombie in the DOMDYING_dead state with no
pages, yet with refcnt=1. This happens absolutely consistently. Just as
consistently, it does not happen when I boot Xen with no-tmem. My conclusion
is that tmem is leaking a domain reference count during domain save. This
doesn't happen if I merely "xm create ...; xm destroy ...".

My pv_config file contains nothing exciting:
kernel = "/nfs/keir/xen/xen64.hg/dist/install/boot/vmlinuz-2.6.18.8-xenU"
memory = 750
name = "PV1"
vcpus = 2
vif = [ 'mac=00:1a:00:00:01:01' ]
disk = [ 'phy:/dev/VG/Suse10.1_64_1,sda1,w' ]
root = "/dev/sda1 ro xencons=tty"
extra = ""
tsc_native = 1
on_poweroff = 'destroy'
on_reboot   = 'restart'
on_crash    = 'preserve'

The dom{0,U} kernels are tip of linux-2.6.18-xen, default -xen{0,U} configs.

 -- Keir

RE: Bug in tmem: refcount leak leaves zombie saved domains

[Dan Magenheimer]

OK, will take a look.

IIRC, Jan's work to fix the domain reference stuff just
before 4.0 shipped was a heavy hammer but since it seemed
to work, I didn't want to mess with it so close to release...
really there's only a need to take a reference once on
first use and release it at shutdown, rather than
take/release frequently.  IIRC, I had used a macro that
took references when they weren't really needed and
Jan placed the matching macros that did the release.

Dan

> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: Thursday, June 10, 2010 3:47 AM
> To: xen-devel@lists.xensource.com
> Cc: Dan Magenheimer
> Subject: Bug in tmem: refcount leak leaves zombie saved domains
> 
> Dan,
> 
> Just doing some save/restore testing on xen-unstable tip, I noticed
> that:
> # xm create ./pv_config
> # xm save PV1
> 
> Would leave the saved guest as a zombie in the DOMDYING_dead state with
> no
> pages, yet with refcnt=1. This happens absolutely consistently. Just as
> consistently, it does not happen when I boot Xen with no-tmem. My
> conclusion
> is that tmem is leaking a domain reference count during domain save.
> This
> doesn't happen if I merely "xm create ...; xm destroy ...".
> 
> My pv_config file contains nothing exciting:
> kernel = "/nfs/keir/xen/xen64.hg/dist/install/boot/vmlinuz-2.6.18.8-
> xenU"
> memory = 750
> name = "PV1"
> vcpus = 2
> vif = [ 'mac=00:1a:00:00:01:01' ]
> disk = [ 'phy:/dev/VG/Suse10.1_64_1,sda1,w' ]
> root = "/dev/sda1 ro xencons=tty"
> extra = ""
> tsc_native = 1
> on_poweroff = 'destroy'
> on_reboot   = 'restart'
> on_crash    = 'preserve'
> 
> The dom{0,U} kernels are tip of linux-2.6.18-xen, default -xen{0,U}
> configs.
> 
>  -- Keir
> 
> 

RE: RE: Bug in tmem: refcount leak leaves zombie saved domains

[Dan Magenheimer]

Keir --

I'd like to do a get_domain_by_id() without doing a get_domain()
as the tmem code need only get_domain() once on first use
and put_domain() once when destroying, but frequently needs
to lookup a domain by id.

It looks like rcu_lock_domain_by_id() does what I need, but
I don't need any rcu critical sections (outside of the domain
lookup itself) and am fearful that if rcu locking ever is fully
implemented, my use of rcu_lock_domain_by_id() would become
incorrect and I may have a problem.  Should I create an equivalent
get_domain_by_id_no_ref()?  Or am I misunderstanding something?

Semi-related, rcu_lock_domain_by_id() has a "return d" inside
the for loop without an rcu_read_unlock().  I see that this
is irrelevant now because rcu_read_unlock() is a no-op anyway,
but maybe this should be cleaned up for the same reason --
in case rcu locking is ever fully implemented.

Thanks,
Dan

> -----Original Message-----
> From: Dan Magenheimer
> Sent: Thursday, June 10, 2010 7:08 AM
> To: Keir Fraser; xen-devel@lists.xensource.com
> Subject: [Xen-devel] RE: Bug in tmem: refcount leak leaves zombie saved
> domains
> 
> OK, will take a look.
> 
> IIRC, Jan's work to fix the domain reference stuff just
> before 4.0 shipped was a heavy hammer but since it seemed
> to work, I didn't want to mess with it so close to release...
> really there's only a need to take a reference once on
> first use and release it at shutdown, rather than
> take/release frequently.  IIRC, I had used a macro that
> took references when they weren't really needed and
> Jan placed the matching macros that did the release.
> 
> Dan
> 
> > -----Original Message-----
> > From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> > Sent: Thursday, June 10, 2010 3:47 AM
> > To: xen-devel@lists.xensource.com
> > Cc: Dan Magenheimer
> > Subject: Bug in tmem: refcount leak leaves zombie saved domains
> >
> > Dan,
> >
> > Just doing some save/restore testing on xen-unstable tip, I noticed
> > that:
> > # xm create ./pv_config
> > # xm save PV1
> >
> > Would leave the saved guest as a zombie in the DOMDYING_dead state
> with
> > no
> > pages, yet with refcnt=1. This happens absolutely consistently. Just
> as
> > consistently, it does not happen when I boot Xen with no-tmem. My
> > conclusion
> > is that tmem is leaking a domain reference count during domain save.
> > This
> > doesn't happen if I merely "xm create ...; xm destroy ...".
> >
> > My pv_config file contains nothing exciting:
> > kernel = "/nfs/keir/xen/xen64.hg/dist/install/boot/vmlinuz-2.6.18.8-
> > xenU"
> > memory = 750
> > name = "PV1"
> > vcpus = 2
> > vif = [ 'mac=00:1a:00:00:01:01' ]
> > disk = [ 'phy:/dev/VG/Suse10.1_64_1,sda1,w' ]
> > root = "/dev/sda1 ro xencons=tty"
> > extra = ""
> > tsc_native = 1
> > on_poweroff = 'destroy'
> > on_reboot   = 'restart'
> > on_crash    = 'preserve'
> >
> > The dom{0,U} kernels are tip of linux-2.6.18-xen, default -xen{0,U}
> > configs.
> >
> >  -- Keir
> >
> >
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: RE: Bug in tmem: refcount leak leaves zombie saved domains

[Keir Fraser]

On 10/06/2010 16:38, "Dan Magenheimer" <dan.magenheimer@oracle.com> wrote:

> Keir --
> 
> I'd like to do a get_domain_by_id() without doing a get_domain()
> as the tmem code need only get_domain() once on first use
> and put_domain() once when destroying, but frequently needs
> to lookup a domain by id.

If you have a handle on a domain already I wonder why you need to
continually look up by domid...

> It looks like rcu_lock_domain_by_id() does what I need, but
> I don't need any rcu critical sections (outside of the domain
> lookup itself) and am fearful that if rcu locking ever is fully
> implemented, my use of rcu_lock_domain_by_id() would become
> incorrect and I may have a problem.  Should I create an equivalent
> get_domain_by_id_no_ref()?  Or am I misunderstanding something?

If you really know what you're doing, I suggest just have your own
tmh_lookup_domain() macro mapping onto rcu_lock_domain_by_id(). RCU locking
is fully implemented already. It's highly unlikely to change in future and
we can work out something else for your case if that happens.

I'm not keen on providing an explicitly synchronisation-free version in
common code. It just encourages people not to think about synchronisation at
all.

 -- Keir

> Semi-related, rcu_lock_domain_by_id() has a "return d" inside
> the for loop without an rcu_read_unlock().  I see that this
> is irrelevant now because rcu_read_unlock() is a no-op anyway,
> but maybe this should be cleaned up for the same reason --
> in case rcu locking is ever fully implemented.
> 
> Thanks,
> Dan
> 
>> -----Original Message-----
>> From: Dan Magenheimer
>> Sent: Thursday, June 10, 2010 7:08 AM
>> To: Keir Fraser; xen-devel@lists.xensource.com
>> Subject: [Xen-devel] RE: Bug in tmem: refcount leak leaves zombie saved
>> domains
>> 
>> OK, will take a look.
>> 
>> IIRC, Jan's work to fix the domain reference stuff just
>> before 4.0 shipped was a heavy hammer but since it seemed
>> to work, I didn't want to mess with it so close to release...
>> really there's only a need to take a reference once on
>> first use and release it at shutdown, rather than
>> take/release frequently.  IIRC, I had used a macro that
>> took references when they weren't really needed and
>> Jan placed the matching macros that did the release.
>> 
>> Dan
>> 
>>> -----Original Message-----
>>> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>>> Sent: Thursday, June 10, 2010 3:47 AM
>>> To: xen-devel@lists.xensource.com
>>> Cc: Dan Magenheimer
>>> Subject: Bug in tmem: refcount leak leaves zombie saved domains
>>> 
>>> Dan,
>>> 
>>> Just doing some save/restore testing on xen-unstable tip, I noticed
>>> that:
>>> # xm create ./pv_config
>>> # xm save PV1
>>> 
>>> Would leave the saved guest as a zombie in the DOMDYING_dead state
>> with
>>> no
>>> pages, yet with refcnt=1. This happens absolutely consistently. Just
>> as
>>> consistently, it does not happen when I boot Xen with no-tmem. My
>>> conclusion
>>> is that tmem is leaking a domain reference count during domain save.
>>> This
>>> doesn't happen if I merely "xm create ...; xm destroy ...".
>>> 
>>> My pv_config file contains nothing exciting:
>>> kernel = "/nfs/keir/xen/xen64.hg/dist/install/boot/vmlinuz-2.6.18.8-
>>> xenU"
>>> memory = 750
>>> name = "PV1"
>>> vcpus = 2
>>> vif = [ 'mac=00:1a:00:00:01:01' ]
>>> disk = [ 'phy:/dev/VG/Suse10.1_64_1,sda1,w' ]
>>> root = "/dev/sda1 ro xencons=tty"
>>> extra = ""
>>> tsc_native = 1
>>> on_poweroff = 'destroy'
>>> on_reboot   = 'restart'
>>> on_crash    = 'preserve'
>>> 
>>> The dom{0,U} kernels are tip of linux-2.6.18-xen, default -xen{0,U}
>>> configs.
>>> 
>>>  -- Keir
>>> 
>>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xensource.com
>> http://lists.xensource.com/xen-devel

RE: RE: Bug in tmem: refcount leak leaves zombie saved domains

[Dan Magenheimer]

[-- Attachment #1: Type: text/plain, Size: 5311 bytes --]

Could you give the attached a try on your test case?  If
it passes and Jan thinks it is OK (as I backed out most of
his patch at cs 20918), then:

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@oracle.com>

> If you have a handle on a domain already I wonder why you need to
> continually look up by domid...

Nearly all tmem uses are via current->domain.  The remaining
(such as from the tools) are via a specified domid.  I don't
keep a domid->domain lookup table around as the frequency is
very low and the existing mechanism works fine (or it does
if I use it correctly anyway ;-)

> RCU locking
> is fully implemented already. It's highly unlikely to change in future
> and we can work out something else for your case if that happens.

I guess I was confused by the fact that the rcu_lock/unlock macros
are no-ops.

In any case, I think I understand the semantics well enough now
after your second reply pointing me to rcu_unlock_domain, so
I think the attached patch should avoid special cases in the
future.

> > I'd like to do a get_domain_by_id() without doing a get_domain()
> > as the tmem code need only get_domain() once on first use
> > and put_domain() once when destroying, but frequently needs
> > to lookup a domain by id.
> 
> If you have a handle on a domain already I wonder why you need to
> continually look up by domid...
> 
> > It looks like rcu_lock_domain_by_id() does what I need, but
> > I don't need any rcu critical sections (outside of the domain
> > lookup itself) and am fearful that if rcu locking ever is fully
> > implemented, my use of rcu_lock_domain_by_id() would become
> > incorrect and I may have a problem.  Should I create an equivalent
> > get_domain_by_id_no_ref()?  Or am I misunderstanding something?
> 
> If you really know what you're doing, I suggest just have your own
> tmh_lookup_domain() macro mapping onto rcu_lock_domain_by_id(). RCU
> locking
> is fully implemented already. It's highly unlikely to change in future
> and
> we can work out something else for your case if that happens.
> 
> I'm not keen on providing an explicitly synchronisation-free version in
> common code. It just encourages people not to think about
> synchronisation at
> all.
> 
>  -- Keir
> 
> > Semi-related, rcu_lock_domain_by_id() has a "return d" inside
> > the for loop without an rcu_read_unlock().  I see that this
> > is irrelevant now because rcu_read_unlock() is a no-op anyway,
> > but maybe this should be cleaned up for the same reason --
> > in case rcu locking is ever fully implemented.
> >
> > Thanks,
> > Dan
> >
> >> -----Original Message-----
> >> From: Dan Magenheimer
> >> Sent: Thursday, June 10, 2010 7:08 AM
> >> To: Keir Fraser; xen-devel@lists.xensource.com
> >> Subject: [Xen-devel] RE: Bug in tmem: refcount leak leaves zombie
> saved
> >> domains
> >>
> >> OK, will take a look.
> >>
> >> IIRC, Jan's work to fix the domain reference stuff just
> >> before 4.0 shipped was a heavy hammer but since it seemed
> >> to work, I didn't want to mess with it so close to release...
> >> really there's only a need to take a reference once on
> >> first use and release it at shutdown, rather than
> >> take/release frequently.  IIRC, I had used a macro that
> >> took references when they weren't really needed and
> >> Jan placed the matching macros that did the release.
> >>
> >> Dan
> >>
> >>> -----Original Message-----
> >>> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> >>> Sent: Thursday, June 10, 2010 3:47 AM
> >>> To: xen-devel@lists.xensource.com
> >>> Cc: Dan Magenheimer
> >>> Subject: Bug in tmem: refcount leak leaves zombie saved domains
> >>>
> >>> Dan,
> >>>
> >>> Just doing some save/restore testing on xen-unstable tip, I noticed
> >>> that:
> >>> # xm create ./pv_config
> >>> # xm save PV1
> >>>
> >>> Would leave the saved guest as a zombie in the DOMDYING_dead state
> >> with
> >>> no
> >>> pages, yet with refcnt=1. This happens absolutely consistently.
> Just
> >> as
> >>> consistently, it does not happen when I boot Xen with no-tmem. My
> >>> conclusion
> >>> is that tmem is leaking a domain reference count during domain
> save.
> >>> This
> >>> doesn't happen if I merely "xm create ...; xm destroy ...".
> >>>
> >>> My pv_config file contains nothing exciting:
> >>> kernel = "/nfs/keir/xen/xen64.hg/dist/install/boot/vmlinuz-
> 2.6.18.8-
> >>> xenU"
> >>> memory = 750
> >>> name = "PV1"
> >>> vcpus = 2
> >>> vif = [ 'mac=00:1a:00:00:01:01' ]
> >>> disk = [ 'phy:/dev/VG/Suse10.1_64_1,sda1,w' ]
> >>> root = "/dev/sda1 ro xencons=tty"
> >>> extra = ""
> >>> tsc_native = 1
> >>> on_poweroff = 'destroy'
> >>> on_reboot   = 'restart'
> >>> on_crash    = 'preserve'
> >>>
> >>> The dom{0,U} kernels are tip of linux-2.6.18-xen, default -xen{0,U}
> >>> configs.
> >>>
> >>>  -- Keir
> >>>
> >>>
> >>
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@lists.xensource.com
> >> http://lists.xensource.com/xen-devel
> 
> 

[-- Attachment #2: tmem-fix-refcount-leak.patch --]
[-- Type: application/octet-stream, Size: 6929 bytes --]

diff -r 1bfab8704f32 xen/common/tmem.c
--- a/xen/common/tmem.c	Thu Jun 10 10:18:00 2010 +0100
+++ b/xen/common/tmem.c	Thu Jun 10 11:35:52 2010 -0600
@@ -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 1bfab8704f32 xen/include/xen/tmem_xen.h
--- a/xen/include/xen/tmem_xen.h	Thu Jun 10 10:18:00 2010 +0100
+++ b/xen/include/xen/tmem_xen.h	Thu Jun 10 11:35:52 2010 -0600
@@ -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;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: RE: Bug in tmem: refcount leak leaves zombie saved domains

[Keir Fraser]

This patch looks good, I'll test it tomorrow. I think you don't need to mess
with reference counts *at all* however, since
domain_kill()->tmem_destroy()->client_flush()->client_free()->tmh_client_des
troy()->put_domain(). But domain_kill() itself holds a domain reference,
hence tmem_destroy's put_domain() is never the last domain reference. IOW,
since you have a hook on the domain destruction path, you basically get a
callback before a domain's reference count falls to zero, and that's all you
need.

What you *do* need to do when setting up a new tmem client is check that the
associated domain is not dying. Without that check the code is in fact
currently buggy: you can end up with a zombie domain that is a client of
tmem and will never stop being a client because it became a client after
tmem_destroy() was called on it.

Does that make sense?

 -- Keir

On 10/06/2010 18:54, "Dan Magenheimer" <dan.magenheimer@oracle.com> wrote:

> Could you give the attached a try on your test case?  If
> it passes and Jan thinks it is OK (as I backed out most of
> his patch at cs 20918), then:
> 
> 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@oracle.com>
> 
>> If you have a handle on a domain already I wonder why you need to
>> continually look up by domid...
> 
> Nearly all tmem uses are via current->domain.  The remaining
> (such as from the tools) are via a specified domid.  I don't
> keep a domid->domain lookup table around as the frequency is
> very low and the existing mechanism works fine (or it does
> if I use it correctly anyway ;-)
> 
>> RCU locking
>> is fully implemented already. It's highly unlikely to change in future
>> and we can work out something else for your case if that happens.
> 
> I guess I was confused by the fact that the rcu_lock/unlock macros
> are no-ops.
> 
> In any case, I think I understand the semantics well enough now
> after your second reply pointing me to rcu_unlock_domain, so
> I think the attached patch should avoid special cases in the
> future.
> 
>>> I'd like to do a get_domain_by_id() without doing a get_domain()
>>> as the tmem code need only get_domain() once on first use
>>> and put_domain() once when destroying, but frequently needs
>>> to lookup a domain by id.
>> 
>> If you have a handle on a domain already I wonder why you need to
>> continually look up by domid...
>> 
>>> It looks like rcu_lock_domain_by_id() does what I need, but
>>> I don't need any rcu critical sections (outside of the domain
>>> lookup itself) and am fearful that if rcu locking ever is fully
>>> implemented, my use of rcu_lock_domain_by_id() would become
>>> incorrect and I may have a problem.  Should I create an equivalent
>>> get_domain_by_id_no_ref()?  Or am I misunderstanding something?
>> 
>> If you really know what you're doing, I suggest just have your own
>> tmh_lookup_domain() macro mapping onto rcu_lock_domain_by_id(). RCU
>> locking
>> is fully implemented already. It's highly unlikely to change in future
>> and
>> we can work out something else for your case if that happens.
>> 
>> I'm not keen on providing an explicitly synchronisation-free version in
>> common code. It just encourages people not to think about
>> synchronisation at
>> all.
>> 
>>  -- Keir
>> 
>>> Semi-related, rcu_lock_domain_by_id() has a "return d" inside
>>> the for loop without an rcu_read_unlock().  I see that this
>>> is irrelevant now because rcu_read_unlock() is a no-op anyway,
>>> but maybe this should be cleaned up for the same reason --
>>> in case rcu locking is ever fully implemented.
>>> 
>>> Thanks,
>>> Dan
>>> 
>>>> -----Original Message-----
>>>> From: Dan Magenheimer
>>>> Sent: Thursday, June 10, 2010 7:08 AM
>>>> To: Keir Fraser; xen-devel@lists.xensource.com
>>>> Subject: [Xen-devel] RE: Bug in tmem: refcount leak leaves zombie
>> saved
>>>> domains
>>>> 
>>>> OK, will take a look.
>>>> 
>>>> IIRC, Jan's work to fix the domain reference stuff just
>>>> before 4.0 shipped was a heavy hammer but since it seemed
>>>> to work, I didn't want to mess with it so close to release...
>>>> really there's only a need to take a reference once on
>>>> first use and release it at shutdown, rather than
>>>> take/release frequently.  IIRC, I had used a macro that
>>>> took references when they weren't really needed and
>>>> Jan placed the matching macros that did the release.
>>>> 
>>>> Dan
>>>> 
>>>>> -----Original Message-----
>>>>> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>>>>> Sent: Thursday, June 10, 2010 3:47 AM
>>>>> To: xen-devel@lists.xensource.com
>>>>> Cc: Dan Magenheimer
>>>>> Subject: Bug in tmem: refcount leak leaves zombie saved domains
>>>>> 
>>>>> Dan,
>>>>> 
>>>>> Just doing some save/restore testing on xen-unstable tip, I noticed
>>>>> that:
>>>>> # xm create ./pv_config
>>>>> # xm save PV1
>>>>> 
>>>>> Would leave the saved guest as a zombie in the DOMDYING_dead state
>>>> with
>>>>> no
>>>>> pages, yet with refcnt=1. This happens absolutely consistently.
>> Just
>>>> as
>>>>> consistently, it does not happen when I boot Xen with no-tmem. My
>>>>> conclusion
>>>>> is that tmem is leaking a domain reference count during domain
>> save.
>>>>> This
>>>>> doesn't happen if I merely "xm create ...; xm destroy ...".
>>>>> 
>>>>> My pv_config file contains nothing exciting:
>>>>> kernel = "/nfs/keir/xen/xen64.hg/dist/install/boot/vmlinuz-
>> 2.6.18.8-
>>>>> xenU"
>>>>> memory = 750
>>>>> name = "PV1"
>>>>> vcpus = 2
>>>>> vif = [ 'mac=00:1a:00:00:01:01' ]
>>>>> disk = [ 'phy:/dev/VG/Suse10.1_64_1,sda1,w' ]
>>>>> root = "/dev/sda1 ro xencons=tty"
>>>>> extra = ""
>>>>> tsc_native = 1
>>>>> on_poweroff = 'destroy'
>>>>> on_reboot   = 'restart'
>>>>> on_crash    = 'preserve'
>>>>> 
>>>>> The dom{0,U} kernels are tip of linux-2.6.18-xen, default -xen{0,U}
>>>>> configs.
>>>>> 
>>>>>  -- Keir
>>>>> 
>>>>> 
>>>> 
>>>> _______________________________________________
>>>> Xen-devel mailing list
>>>> Xen-devel@lists.xensource.com
>>>> http://lists.xensource.com/xen-devel
>> 
>> 

Re: RE: Bug in tmem: refcount leak leaves zombie saved domains

[Keir Fraser]

On 10/06/2010 21:06, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

> This patch looks good, I'll test it tomorrow.

It tests okay so I applied it as xen-unstable:21595.

> What you *do* need to do when setting up a new tmem client is check that the
> associated domain is not dying. Without that check the code is in fact
> currently buggy: you can end up with a zombie domain that is a client of
> tmem and will never stop being a client because it became a client after
> tmem_destroy() was called on it.

I implemented this as xen-unstable:21596. Take a look. It's pretty
straightforward.

I think both of these should be backported for Xen 4.0.1?

 -- Keir

> Does that make sense?
> 
>  -- Keir
> 
> On 10/06/2010 18:54, "Dan Magenheimer" <dan.magenheimer@oracle.com> wrote:
> 
>> Could you give the attached a try on your test case?  If
>> it passes and Jan thinks it is OK (as I backed out most of
>> his patch at cs 20918), then:
>> 
>> 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@oracle.com>
>> 
>>> If you have a handle on a domain already I wonder why you need to
>>> continually look up by domid...
>> 
>> Nearly all tmem uses are via current->domain.  The remaining
>> (such as from the tools) are via a specified domid.  I don't
>> keep a domid->domain lookup table around as the frequency is
>> very low and the existing mechanism works fine (or it does
>> if I use it correctly anyway ;-)
>> 
>>> RCU locking
>>> is fully implemented already. It's highly unlikely to change in future
>>> and we can work out something else for your case if that happens.
>> 
>> I guess I was confused by the fact that the rcu_lock/unlock macros
>> are no-ops.
>> 
>> In any case, I think I understand the semantics well enough now
>> after your second reply pointing me to rcu_unlock_domain, so
>> I think the attached patch should avoid special cases in the
>> future.
>> 
>>>> I'd like to do a get_domain_by_id() without doing a get_domain()
>>>> as the tmem code need only get_domain() once on first use
>>>> and put_domain() once when destroying, but frequently needs
>>>> to lookup a domain by id.
>>> 
>>> If you have a handle on a domain already I wonder why you need to
>>> continually look up by domid...
>>> 
>>>> It looks like rcu_lock_domain_by_id() does what I need, but
>>>> I don't need any rcu critical sections (outside of the domain
>>>> lookup itself) and am fearful that if rcu locking ever is fully
>>>> implemented, my use of rcu_lock_domain_by_id() would become
>>>> incorrect and I may have a problem.  Should I create an equivalent
>>>> get_domain_by_id_no_ref()?  Or am I misunderstanding something?
>>> 
>>> If you really know what you're doing, I suggest just have your own
>>> tmh_lookup_domain() macro mapping onto rcu_lock_domain_by_id(). RCU
>>> locking
>>> is fully implemented already. It's highly unlikely to change in future
>>> and
>>> we can work out something else for your case if that happens.
>>> 
>>> I'm not keen on providing an explicitly synchronisation-free version in
>>> common code. It just encourages people not to think about
>>> synchronisation at
>>> all.
>>> 
>>>  -- Keir
>>> 
>>>> Semi-related, rcu_lock_domain_by_id() has a "return d" inside
>>>> the for loop without an rcu_read_unlock().  I see that this
>>>> is irrelevant now because rcu_read_unlock() is a no-op anyway,
>>>> but maybe this should be cleaned up for the same reason --
>>>> in case rcu locking is ever fully implemented.
>>>> 
>>>> Thanks,
>>>> Dan
>>>> 
>>>>> -----Original Message-----
>>>>> From: Dan Magenheimer
>>>>> Sent: Thursday, June 10, 2010 7:08 AM
>>>>> To: Keir Fraser; xen-devel@lists.xensource.com
>>>>> Subject: [Xen-devel] RE: Bug in tmem: refcount leak leaves zombie
>>> saved
>>>>> domains
>>>>> 
>>>>> OK, will take a look.
>>>>> 
>>>>> IIRC, Jan's work to fix the domain reference stuff just
>>>>> before 4.0 shipped was a heavy hammer but since it seemed
>>>>> to work, I didn't want to mess with it so close to release...
>>>>> really there's only a need to take a reference once on
>>>>> first use and release it at shutdown, rather than
>>>>> take/release frequently.  IIRC, I had used a macro that
>>>>> took references when they weren't really needed and
>>>>> Jan placed the matching macros that did the release.
>>>>> 
>>>>> Dan
>>>>> 
>>>>>> -----Original Message-----
>>>>>> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
>>>>>> Sent: Thursday, June 10, 2010 3:47 AM
>>>>>> To: xen-devel@lists.xensource.com
>>>>>> Cc: Dan Magenheimer
>>>>>> Subject: Bug in tmem: refcount leak leaves zombie saved domains
>>>>>> 
>>>>>> Dan,
>>>>>> 
>>>>>> Just doing some save/restore testing on xen-unstable tip, I noticed
>>>>>> that:
>>>>>> # xm create ./pv_config
>>>>>> # xm save PV1
>>>>>> 
>>>>>> Would leave the saved guest as a zombie in the DOMDYING_dead state
>>>>> with
>>>>>> no
>>>>>> pages, yet with refcnt=1. This happens absolutely consistently.
>>> Just
>>>>> as
>>>>>> consistently, it does not happen when I boot Xen with no-tmem. My
>>>>>> conclusion
>>>>>> is that tmem is leaking a domain reference count during domain
>>> save.
>>>>>> This
>>>>>> doesn't happen if I merely "xm create ...; xm destroy ...".
>>>>>> 
>>>>>> My pv_config file contains nothing exciting:
>>>>>> kernel = "/nfs/keir/xen/xen64.hg/dist/install/boot/vmlinuz-
>>> 2.6.18.8-
>>>>>> xenU"
>>>>>> memory = 750
>>>>>> name = "PV1"
>>>>>> vcpus = 2
>>>>>> vif = [ 'mac=00:1a:00:00:01:01' ]
>>>>>> disk = [ 'phy:/dev/VG/Suse10.1_64_1,sda1,w' ]
>>>>>> root = "/dev/sda1 ro xencons=tty"
>>>>>> extra = ""
>>>>>> tsc_native = 1
>>>>>> on_poweroff = 'destroy'
>>>>>> on_reboot   = 'restart'
>>>>>> on_crash    = 'preserve'
>>>>>> 
>>>>>> The dom{0,U} kernels are tip of linux-2.6.18-xen, default -xen{0,U}
>>>>>> configs.
>>>>>> 
>>>>>>  -- Keir
>>>>>> 
>>>>>> 
>>>>> 
>>>>> _______________________________________________
>>>>> Xen-devel mailing list
>>>>> Xen-devel@lists.xensource.com
>>>>> http://lists.xensource.com/xen-devel
>>> 
>>> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

RE: RE: Bug in tmem: refcount leak leaves zombie saved domains

[Dan Magenheimer]

> I think both of these should be backported for Xen 4.0.1?

Thanks for handling it.  I agree it should go into Xen 4.0.1,
but I won't be able to give it much testing before that
releases (unless it takes a few more RC's) so if you are
confident and Jan doesn't see any problems, go for it.

> -----Original Message-----
> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> Sent: Thursday, June 10, 2010 3:43 PM
> To: Dan Magenheimer; xen-devel@lists.xensource.com
> Cc: JBeulich@novell.com
> Subject: Re: [Xen-devel] RE: Bug in tmem: refcount leak leaves zombie
> saved domains
> 
> On 10/06/2010 21:06, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:
> 
> > This patch looks good, I'll test it tomorrow.
> 
> It tests okay so I applied it as xen-unstable:21595.
> 
> > What you *do* need to do when setting up a new tmem client is check
> that the
> > associated domain is not dying. Without that check the code is in
> fact
> > currently buggy: you can end up with a zombie domain that is a client
> of
> > tmem and will never stop being a client because it became a client
> after
> > tmem_destroy() was called on it.
> 
> I implemented this as xen-unstable:21596. Take a look. It's pretty
> straightforward.
> 
> I think both of these should be backported for Xen 4.0.1?
> 
>  -- Keir
> 
> > Does that make sense?
> >
> >  -- Keir
> >
> > On 10/06/2010 18:54, "Dan Magenheimer" <dan.magenheimer@oracle.com>
> wrote:
> >
> >> Could you give the attached a try on your test case?  If
> >> it passes and Jan thinks it is OK (as I backed out most of
> >> his patch at cs 20918), then:
> >>
> >> 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@oracle.com>
> >>
> >>> If you have a handle on a domain already I wonder why you need to
> >>> continually look up by domid...
> >>
> >> Nearly all tmem uses are via current->domain.  The remaining
> >> (such as from the tools) are via a specified domid.  I don't
> >> keep a domid->domain lookup table around as the frequency is
> >> very low and the existing mechanism works fine (or it does
> >> if I use it correctly anyway ;-)
> >>
> >>> RCU locking
> >>> is fully implemented already. It's highly unlikely to change in
> future
> >>> and we can work out something else for your case if that happens.
> >>
> >> I guess I was confused by the fact that the rcu_lock/unlock macros
> >> are no-ops.
> >>
> >> In any case, I think I understand the semantics well enough now
> >> after your second reply pointing me to rcu_unlock_domain, so
> >> I think the attached patch should avoid special cases in the
> >> future.
> >>
> >>>> I'd like to do a get_domain_by_id() without doing a get_domain()
> >>>> as the tmem code need only get_domain() once on first use
> >>>> and put_domain() once when destroying, but frequently needs
> >>>> to lookup a domain by id.
> >>>
> >>> If you have a handle on a domain already I wonder why you need to
> >>> continually look up by domid...
> >>>
> >>>> It looks like rcu_lock_domain_by_id() does what I need, but
> >>>> I don't need any rcu critical sections (outside of the domain
> >>>> lookup itself) and am fearful that if rcu locking ever is fully
> >>>> implemented, my use of rcu_lock_domain_by_id() would become
> >>>> incorrect and I may have a problem.  Should I create an equivalent
> >>>> get_domain_by_id_no_ref()?  Or am I misunderstanding something?
> >>>
> >>> If you really know what you're doing, I suggest just have your own
> >>> tmh_lookup_domain() macro mapping onto rcu_lock_domain_by_id(). RCU
> >>> locking
> >>> is fully implemented already. It's highly unlikely to change in
> future
> >>> and
> >>> we can work out something else for your case if that happens.
> >>>
> >>> I'm not keen on providing an explicitly synchronisation-free
> version in
> >>> common code. It just encourages people not to think about
> >>> synchronisation at
> >>> all.
> >>>
> >>>  -- Keir
> >>>
> >>>> Semi-related, rcu_lock_domain_by_id() has a "return d" inside
> >>>> the for loop without an rcu_read_unlock().  I see that this
> >>>> is irrelevant now because rcu_read_unlock() is a no-op anyway,
> >>>> but maybe this should be cleaned up for the same reason --
> >>>> in case rcu locking is ever fully implemented.
> >>>>
> >>>> Thanks,
> >>>> Dan
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: Dan Magenheimer
> >>>>> Sent: Thursday, June 10, 2010 7:08 AM
> >>>>> To: Keir Fraser; xen-devel@lists.xensource.com
> >>>>> Subject: [Xen-devel] RE: Bug in tmem: refcount leak leaves zombie
> >>> saved
> >>>>> domains
> >>>>>
> >>>>> OK, will take a look.
> >>>>>
> >>>>> IIRC, Jan's work to fix the domain reference stuff just
> >>>>> before 4.0 shipped was a heavy hammer but since it seemed
> >>>>> to work, I didn't want to mess with it so close to release...
> >>>>> really there's only a need to take a reference once on
> >>>>> first use and release it at shutdown, rather than
> >>>>> take/release frequently.  IIRC, I had used a macro that
> >>>>> took references when they weren't really needed and
> >>>>> Jan placed the matching macros that did the release.
> >>>>>
> >>>>> Dan
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Keir Fraser [mailto:keir.fraser@eu.citrix.com]
> >>>>>> Sent: Thursday, June 10, 2010 3:47 AM
> >>>>>> To: xen-devel@lists.xensource.com
> >>>>>> Cc: Dan Magenheimer
> >>>>>> Subject: Bug in tmem: refcount leak leaves zombie saved domains
> >>>>>>
> >>>>>> Dan,
> >>>>>>
> >>>>>> Just doing some save/restore testing on xen-unstable tip, I
> noticed
> >>>>>> that:
> >>>>>> # xm create ./pv_config
> >>>>>> # xm save PV1
> >>>>>>
> >>>>>> Would leave the saved guest as a zombie in the DOMDYING_dead
> state
> >>>>> with
> >>>>>> no
> >>>>>> pages, yet with refcnt=1. This happens absolutely consistently.
> >>> Just
> >>>>> as
> >>>>>> consistently, it does not happen when I boot Xen with no-tmem.
> My
> >>>>>> conclusion
> >>>>>> is that tmem is leaking a domain reference count during domain
> >>> save.
> >>>>>> This
> >>>>>> doesn't happen if I merely "xm create ...; xm destroy ...".
> >>>>>>
> >>>>>> My pv_config file contains nothing exciting:
> >>>>>> kernel = "/nfs/keir/xen/xen64.hg/dist/install/boot/vmlinuz-
> >>> 2.6.18.8-
> >>>>>> xenU"
> >>>>>> memory = 750
> >>>>>> name = "PV1"
> >>>>>> vcpus = 2
> >>>>>> vif = [ 'mac=00:1a:00:00:01:01' ]
> >>>>>> disk = [ 'phy:/dev/VG/Suse10.1_64_1,sda1,w' ]
> >>>>>> root = "/dev/sda1 ro xencons=tty"
> >>>>>> extra = ""
> >>>>>> tsc_native = 1
> >>>>>> on_poweroff = 'destroy'
> >>>>>> on_reboot   = 'restart'
> >>>>>> on_crash    = 'preserve'
> >>>>>>
> >>>>>> The dom{0,U} kernels are tip of linux-2.6.18-xen, default -
> xen{0,U}
> >>>>>> configs.
> >>>>>>
> >>>>>>  -- Keir
> >>>>>>
> >>>>>>
> >>>>>
> >>>>> _______________________________________________
> >>>>> Xen-devel mailing list
> >>>>> Xen-devel@lists.xensource.com
> >>>>> http://lists.xensource.com/xen-devel
> >>>
> >>>
> >
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel
> 
> 

RE: RE: Bug in tmem: refcount leak leaves zombie saved domains

[Jan Beulich]

>>> On 10.06.10 at 19:54, Dan Magenheimer <dan.magenheimer@oracle.com> wrote:
> Could you give the attached a try on your test case?  If
> it passes and Jan thinks it is OK (as I backed out most of
> his patch at cs 20918), then:

The combination of the two patches looks okay to me in any case;
I didn't bother checking whether 21595 alone would have been
okay too.

Jan

Re: RE: Bug in tmem: refcount leak leaves zombie saved domains

[Keir Fraser]

On 10/06/2010 16:38, "Dan Magenheimer" <dan.magenheimer@oracle.com> wrote:

> Semi-related, rcu_lock_domain_by_id() has a "return d" inside
> the for loop without an rcu_read_unlock().  I see that this
> is irrelevant now because rcu_read_unlock() is a no-op anyway,
> but maybe this should be cleaned up for the same reason --
> in case rcu locking is ever fully implemented.

The implementation is correct. The matching rcu_read_unlock() is in
rcu_unlock_domain().

 -- Keir