Jim Fehlig
2011-May-20 22:04 UTC
[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 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-May-23 09:16 UTC
Re: [Xen-devel] [PATCH] libxc: use correct macro when unmapping memory after save operation
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); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-May-24 13:50 UTC
Re: [Xen-devel] [PATCH] libxc: use correct macro when unmapping memory after save operation
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2011-May-24 13:58 UTC
Re: [Xen-devel] [PATCH] libxc: use correct macro when unmapping memory after save operation
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Jim Fehlig
2011-May-24 14:25 UTC
Re: [Xen-devel] [PATCH] libxc: use correct macro when unmapping memory after save operation
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2011-May-24 15:52 UTC
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). -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-May-24 16:02 UTC
Re: [Xen-devel] [PATCH] libxc: use correct macro when unmapping memory after save operation
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Ian Jackson
2011-May-24 16:18 UTC
Re: [Xen-devel] [PATCH] libxc: use correct macro when unmapping memory after save operation
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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel