[PATCH] libxc: use correct macro when unmapping memory after save operation

[PATCH] libxc: use correct macro when unmapping memory after save operation

[Jim Fehlig]

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

With some help from Olaf, I've finally got to the bottom of an issue I
came across while trying to implement save/restore in the libvirt
libxenlight driver.  After issuing the save operation, the saved domain
was not being cleaned up properly and left in this state from xl's
perspective

xen33:# xl list
Name                   ID   Mem VCPUs      State   Time(s)
Domain-0                0  6821     8     r-----     122.5
(null)                  2     2     2     --pssd      10.8

Checking the libvirtd /proc/$pid/maps I found this

7f3798984000-7f3798b86000 r--s 00002000 00:03 4026532097 /proc/xen/privcmd

So not all all pages belonging to the domain were unmapped from
libvirtd.  In tools/libxc/xc_domain_save.c we found that P2M_FL_ENTRIES
were being mapped but only P2M_FLL_ENTRIES were being unmapped.  The
attached patch changes the unmapping to use the same P2M_FL_ENTRIES
macro.  I'm not too familiar with this code though so posting here for
review.

I suspect this was not noticed before since most (all?) processes doing
save terminate after the save and are not long-running like libvirtd.

Regards,
Jim


[-- Attachment #2: libxc_save.patch --]
[-- Type: text/x-patch, Size: 519 bytes --]

diff -r 5fb4c607049d tools/libxc/xc_domain_save.c
--- a/tools/libxc/xc_domain_save.c	Fri May 20 09:44:41 2011 +0100
+++ b/tools/libxc/xc_domain_save.c	Fri May 20 16:02:28 2011 -0600
@@ -1955,7 +1955,7 @@ int xc_domain_save(xc_interface *xch, in
         munmap(live_shinfo, PAGE_SIZE);
 
     if ( ctx->live_p2m )
-        munmap(ctx->live_p2m, P2M_FLL_ENTRIES * PAGE_SIZE);
+        munmap(ctx->live_p2m, P2M_FL_ENTRIES * PAGE_SIZE);
 
     if ( ctx->live_m2p )
         munmap(ctx->live_m2p, M2P_SIZE(ctx->max_mfn));

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

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

Re: [PATCH] libxc: use correct macro when unmapping memory after save operation

[Ian Campbell]

On Fri, 2011-05-20 at 23:04 +0100, Jim Fehlig wrote:
> With some help from Olaf, I've finally got to the bottom of an issue I
> came across while trying to implement save/restore in the libvirt
> libxenlight driver.  After issuing the save operation, the saved domain
> was not being cleaned up properly and left in this state from xl's
> perspective
> 
> xen33:# xl list
> Name                   ID   Mem VCPUs      State   Time(s)
> Domain-0                0  6821     8     r-----     122.5
> (null)                  2     2     2     --pssd      10.8
> 
> Checking the libvirtd /proc/$pid/maps I found this
> 
> 7f3798984000-7f3798b86000 r--s 00002000 00:03 4026532097 /proc/xen/privcmd
> 
> So not all all pages belonging to the domain were unmapped from
> libvirtd.  In tools/libxc/xc_domain_save.c we found that P2M_FL_ENTRIES
> were being mapped but only P2M_FLL_ENTRIES were being unmapped.  The
> attached patch changes the unmapping to use the same P2M_FL_ENTRIES
> macro.  I'm not too familiar with this code though so posting here for
> review.
> 
> I suspect this was not noticed before since most (all?) processes doing
> save terminate after the save and are not long-running like libvirtd.

Good catch!

Looks like I introduced this in 18558:ccf0205255e1, sorry!

I guess it is also wrong in the error path out of map_and_save_p2m_table
and so we also need:

diff -r 35ae855173fa tools/libxc/xc_domain_save.c
--- a/tools/libxc/xc_domain_save.c	Mon May 23 10:06:23 2011 +0100
+++ b/tools/libxc/xc_domain_save.c	Mon May 23 10:15:43 2011 +0100
@@ -861,7 +861,7 @@ static xen_pfn_t *map_and_save_p2m_table
  out:
     
     if ( !success && p2m )
-        munmap(p2m, P2M_FLL_ENTRIES * PAGE_SIZE);
+        munmap(p2m, P2M_FL_ENTRIES * PAGE_SIZE);
 
     if ( live_p2m_frame_list_list )
         munmap(live_p2m_frame_list_list, PAGE_SIZE);

Re: [PATCH] libxc: use correct macro when unmapping memory after save operation

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] libxc: use correct macro when unmapping memory after save operation"):
> I guess it is also wrong in the error path out of map_and_save_p2m_table
> and so we also need:

Yes, I agree, I have applied that too.

Ian.

Re: [PATCH] libxc: use correct macro when unmapping memory after save operation

[Ian Jackson]

Jim Fehlig writes ("[Xen-devel] [PATCH] libxc: use correct macro when unmapping memory after save operation"):
> With some help from Olaf, I've finally got to the bottom of an issue I
> came across while trying to implement save/restore in the libvirt
> libxenlight driver.  After issuing the save operation, the saved domain
> was not being cleaned up properly and left in this state from xl's
> perspective

Good catch, thanks.  I have applied this.

Next time, though, can you please be sure to add a Signed-off-by line,
to signify your certification in accordance with the Developer's
Certificate of Origin ?  In this case I'll go ahead as it's only one
line.

Thanks,
Ian.

>From Documentation/SubmittingPatches in the Linux kernel tree:
 
        Developer's Certificate of Origin 1.1
 
        By making a contribution to this project, I certify that:
 
        (a) The contribution was created in whole or in part by me and I
            have the right to submit it under the open source license
            indicated in the file; or
 
        (b) The contribution is based upon previous work that, to the best
            of my knowledge, is covered under an appropriate open source
            license and I have the right under that license to submit that
            work with modifications, whether created in whole or in part
            by me, under the same open source license (unless I am
            permitted to submit under a different license), as indicated
            in the file; or
 
        (c) The contribution was provided directly to me by some other
            person who certified (a), (b) or (c) and I have not modified
            it.
 
        (d) I understand and agree that this project and the contribution
            are public and that a record of the contribution (including all
            personal information I submit with it, including my sign-off) is
            maintained indefinitely and may be redistributed consistent with
            this project or the open source license(s) involved.

Re: [PATCH] libxc: use correct macro when unmapping memory after save operation

[Jim Fehlig]

Ian Jackson wrote:
> Jim Fehlig writes ("[Xen-devel] [PATCH] libxc: use correct macro when unmapping memory after save operation"):
>   
>> With some help from Olaf, I've finally got to the bottom of an issue I
>> came across while trying to implement save/restore in the libvirt
>> libxenlight driver.  After issuing the save operation, the saved domain
>> was not being cleaned up properly and left in this state from xl's
>> perspective
>>     
>
> Good catch, thanks.  I have applied this.
>
> Next time, though, can you please be sure to add a Signed-off-by line,
>   

Yes, apologies for the oversight.

BTW, thanks for the commit message note about "backporting to relevant
earlier trees".  I was going to ask that this be applied to
4.1-testing.  Should such a statement be included for fixes that apply
to multiple trees?  Is it helpful for scanning potential backports in
-unstable?

Regards,
Jim

Re: [PATCH] libxc: use correct macro when unmapping memory after save operation

[Keir Fraser]

On 24/05/2011 15:25, "Jim Fehlig" <jfehlig@novell.com> wrote:

> BTW, thanks for the commit message note about "backporting to relevant
> earlier trees".  I was going to ask that this be applied to
> 4.1-testing.  Should such a statement be included for fixes that apply
> to multiple trees?  Is it helpful for scanning potential backports in
> -unstable?

Worth pointing out that at the moment only I seem to be backporting to the
stable trees, and I only do that for tools patches that I am explicitly
requested to do by a toolstack maintainer. A comment in a changeset comment
is fine if it is a reminder for Ian, but it is useless in terms of getting
me to apply it to 4.1-testing (for example).

 -- Keir

Re: [PATCH] libxc: use correct macro when unmapping memory after save operation

[Ian Jackson]

Jim Fehlig writes ("Re: [Xen-devel] [PATCH] libxc: use correct macro when unmapping memory after save operation"):
> BTW, thanks for the commit message note about "backporting to relevant
> earlier trees".  I was going to ask that this be applied to
> 4.1-testing.  Should such a statement be included for fixes that apply
> to multiple trees?  Is it helpful for scanning potential backports in
> -unstable?

Yes, exactly.  I think it would be a good idea to put such a statement
(including at least the literal string "backport" in such changes).

That's why I put it in in that case.  We're about to do RCs point
releases for 4.0 and 4.1 but after that we should consider these
changes.

Ian.

Re: [PATCH] libxc: use correct macro when unmapping memory after save operation

[Ian Jackson]

Keir Fraser writes ("Re: [Xen-devel] [PATCH] libxc: use correct macro when unmapping memory after save operation"):
> On 24/05/2011 15:25, "Jim Fehlig" <jfehlig@novell.com> wrote:
> > BTW, thanks for the commit message note about "backporting to relevant
> > earlier trees".  I was going to ask that this be applied to
> > 4.1-testing.  Should such a statement be included for fixes that apply
> > to multiple trees?  Is it helpful for scanning potential backports in
> > -unstable?
> 
> Worth pointing out that at the moment only I seem to be backporting to the
> stable trees, and I only do that for tools patches that I am explicitly
> requested to do by a toolstack maintainer. A comment in a changeset comment
> is fine if it is a reminder for Ian, but it is useless in terms of getting
> me to apply it to 4.1-testing (for example).

Indeed.  It is a reminder for me.

Ian.