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

[Keir Fraser <keir.fraser@eu.citrix.com>]

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