Xen devel - Apr 2007 - [Patch] [libxc] add missing free in xc_finish_mmu_updates to avoid memory leak

Hi,

the following patch adds a missing free to xc_finish_mmu_updates,
otherwise the memory allocated by xc_init_mmu_updates gets never freed.

Stephan



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

On 6/4/07 14:45, "Stephan Creutz"
<stephan.creutz@inf.tu-dresden.de> wrote:
> the following patch adds a missing free to xc_finish_mmu_updates,
> otherwise the memory allocated by xc_init_mmu_updates gets never freed.
The only user of this interface calls free() on that memory itself. It also
calls finish_mmu_update() more than once on the same mmu structure, so
freeing it in finish_mmu_update() would not work. Probably best we rename
xc_init_mmu_updates() to xc_alloc_mmu_updates(), finish_mmu_update to
flush_mmu_updates(), and document the need for explicit free() in the header
file. I''ll do this.

 -- Keir



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

On Fri, 06 Apr 2007 22:42:39 +0100
Keir Fraser <Keir.Fraser@cl.cam.ac.uk> wrote:
> On 6/4/07 14:45, "Stephan Creutz"
<stephan.creutz@inf.tu-dresden.de>
> wrote:
> 
> > the following patch adds a missing free to xc_finish_mmu_updates,
> > otherwise the memory allocated by xc_init_mmu_updates gets never
> > freed.
> 
> The only user of this interface calls free() on that memory itself. It
> also calls finish_mmu_update() more than once on the same mmu
> structure, so freeing it in finish_mmu_update() would not work.
> Probably best we rename xc_init_mmu_updates() to
> xc_alloc_mmu_updates(), finish_mmu_update to flush_mmu_updates(), and
> document the need for explicit free() in the header file. I''ll do
> this.
Oops, overlooked that, but xc_finish_mmu_updates sounded like a
destructor. The new function names you propose and the comment should
clear things up. But I would disagree to make the whole interface
private (read that in the staging changelog) because otherwise I will
have to duplicate the code for a program I''m working on (see my post
recently before). On the other hand it would be a minimal effort to
recode that for my needs.

Stephan


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

On 7/4/07 15:03, "Stephan Creutz"
<stephan.creutz@inf.tu-dresden.de> wrote:
>> The only user of this interface calls free() on that memory itself. It
>> also calls finish_mmu_update() more than once on the same mmu
>> structure, so freeing it in finish_mmu_update() would not work.
>> Probably best we rename xc_init_mmu_updates() to
>> xc_alloc_mmu_updates(), finish_mmu_update to flush_mmu_updates(), and
>> document the need for explicit free() in the header file. I''ll
do
>> this.
> 
> Oops, overlooked that, but xc_finish_mmu_updates sounded like a
> destructor. The new function names you propose and the comment should
> clear things up. But I would disagree to make the whole interface
> private (read that in the staging changelog) because otherwise I will
> have to duplicate the code for a program I''m working on (see my
post
> recently before). On the other hand it would be a minimal effort to
> recode that for my needs.
What do you need it for? It''s not all that useful to the toolstack any
more.

 -- Keir


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

On Sun, 8 Apr 2007 00:53:09 -0700
"Keir Fraser" <Keir.Fraser@xensource.com> wrote:
> On 7/4/07 15:03, "Stephan Creutz"
<stephan.creutz@inf.tu-dresden.de>
> wrote:
> 
> >> The only user of this interface calls free() on that memory
itself.
> >It > also calls finish_mmu_update() more than once on the same mmu
> >> structure, so freeing it in finish_mmu_update() would not work.
> >> Probably best we rename xc_init_mmu_updates() to
> >> xc_alloc_mmu_updates(), finish_mmu_update to flush_mmu_updates(),
> >and > document the need for explicit free() in the header file.
I''ll
> >do > this.
> > 
> > Oops, overlooked that, but xc_finish_mmu_updates sounded like a
> > destructor. The new function names you propose and the comment
> > should clear things up. But I would disagree to make the whole
> > interface private (read that in the staging changelog) because
> > otherwise I will have to duplicate the code for a program I''m
> > working on (see my post recently before). On the other hand it would
> > be a minimal effort to recode that for my needs.
> 
> What do you need it for? It''s not all that useful to the toolstack
any
> more.
I''m trying to develop a fast rollback for domains without recreating
the whole domain. In some cases I need to update pagetables to restore
an older memory state from a checkpoint. Maybe I should integrate it in
libxc itself to avoid code duplication. But I thought it would be better
to start with an external tool because my rollback is very special. It
uses the shadow pagetables all the time to track dirty pages.
Shadow pagetables are slow, so it might not be useful for other users.

Stephan


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

On 8/4/07 13:14, "Stephan Creutz"
<stephan.creutz@inf.tu-dresden.de> wrote:
>> What do you need it for? It''s not all that useful to the
toolstack any
>> more.
> 
> I''m trying to develop a fast rollback for domains without
recreating
> the whole domain. In some cases I need to update pagetables to restore
> an older memory state from a checkpoint. Maybe I should integrate it in
> libxc itself to avoid code duplication. But I thought it would be better
> to start with an external tool because my rollback is very special. It
> uses the shadow pagetables all the time to track dirty pages.
> Shadow pagetables are slow, so it might not be useful for other users.
I''m not sure whether this will work. It''s not possible to
modify another
domain''s page tables if Xen is currently type-tracking the page tables
as
such. You''d have to modify Xen to be able to do that.

 -- Keir



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

On Sun, 08 Apr 2007 15:05:37 +0100
Keir Fraser <Keir.Fraser@cl.cam.ac.uk> wrote:
> On 8/4/07 13:14, "Stephan Creutz"
<stephan.creutz@inf.tu-dresden.de>
> wrote:
> > I''m trying to develop a fast rollback for domains without
recreating
> > the whole domain. In some cases I need to update pagetables to
> > restore an older memory state from a checkpoint. Maybe I should
> > integrate it in libxc itself to avoid code duplication. But I
> > thought it would be better to start with an external tool because my
> > rollback is very special. It uses the shadow pagetables all the time
> > to track dirty pages. Shadow pagetables are slow, so it might not be
> > useful for other users.
> 
> I''m not sure whether this will work. It''s not possible to
modify
> another domain''s page tables if Xen is currently type-tracking the
> page tables as such. You''d have to modify Xen to be able to do
that.
Ok, thought a little bit about that. If I modify Xen in way that foreign
pagetable updates from Dom0 are possible, would this change will ever
have a chance to be accepted for Xen or is it against the general design
principles of Xen? Or in other words: is it not implemented because no
one needs that feature for now or because it is not wanted for any
reason? All I try to prevent are external patches, which I have to adapt
every time the Xen internals are changing.

Stephan

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

On 11/4/07 15:55, "Stephan Creutz"
<stephan.creutz@inf.tu-dresden.de> wrote:
>> I''m not sure whether this will work. It''s not
possible to modify
>> another domain''s page tables if Xen is currently type-tracking
the
>> page tables as such. You''d have to modify Xen to be able to do
that.
> 
> Ok, thought a little bit about that. If I modify Xen in way that foreign
> pagetable updates from Dom0 are possible, would this change will ever
> have a chance to be accepted for Xen or is it against the general design
> principles of Xen? Or in other words: is it not implemented because no
> one needs that feature for now or because it is not wanted for any
> reason? All I try to prevent are external patches, which I have to adapt
> every time the Xen internals are changing.
We''d prefer not to have to think about the implications for page type
tracking that this might cause. If your rollback support is suitable for
inclusion then we''d probably consider necessary Xen changes at the same
time. 

You could work around by stripping all pagetable-type pinning, then make
updates to all memory pages, then reapply pagetable-type pinning. This would
mean that your memory rollback code would not need to take special account
of pagetables.

 -- Keir


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

On Wed, 11 Apr 2007 16:34:07 +0100
Keir Fraser <keir@xensource.com> wrote:
> You could work around by stripping all pagetable-type pinning, then
> make updates to all memory pages, then reapply pagetable-type pinning.
> This would mean that your memory rollback code would not need to take
> special account of pagetables.
That was my first approach to solve the problem, but it failed. As far
as I understand it is not that easy because to unpin a frame its
typecount have to be 0. What is the general approach to decrease the
typecount to 0?

Stephan

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

On 13/4/07 15:31, "Stephan Creutz"
<stephan.creutz@inf.tu-dresden.de> wrote:
>> You could work around by stripping all pagetable-type pinning, then
>> make updates to all memory pages, then reapply pagetable-type pinning.
>> This would mean that your memory rollback code would not need to take
>> special account of pagetables.
> 
> That was my first approach to solve the problem, but it failed. As far
> as I understand it is not that easy because to unpin a frame its
> typecount have to be 0. What is the general approach to decrease the
> typecount to 0?
If you reset all VCPU states (setvcpucontext(NULL)) to clear all %cr3
references, and all pinnings are got rid of, then the type counts should
naturally fall to zero. One exception is linear pagetables (tables mapping
themselves) because these introduce circular references, which would need to
be handled somehow. Linux at least doesn''t use linear page tables
though.

I don''t think you can avoid this type-count issue by the way. If you
want to
do arbitrary rollback you have to be able to handle modifying the guest
state arbitrarily, including any kinds of updates to guest page types etc.
To me, that would suggest that just zapping all the existing metadata and
rebuilding from scratch is at least a good initial approach, which you might
then measure and improve upon.

 -- Keir



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