WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [RFC][PATCH] Basic support for page offline

To: "Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx>
Subject: [Xen-devel] Re: [RFC][PATCH] Basic support for page offline
From: Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Date: Fri, 13 Feb 2009 17:03:41 +0000
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Fri, 13 Feb 2009 09:04:29 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <E2263E4A5B2284449EEBD0AAB751098401C781605D@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <E2263E4A5B2284449EEBD0AAB751098401C781605D@xxxxxxxxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.17 (2007-11-01)
Hi, 

So a few more comments on the detail of those patches.  

I had imagined that you would suspend the domain, then update the p2m
and pagetables in the guest memory from the _tools_.  That would involve
less code (possibly none) in Xen, and is how I'd prefer it.  But your
current approach probably catches more of the corner cases (grant tables
&c) than the tools could, so that's OK.

update_pgtable_entry() needs a more descriptive name!  It updates
potentially very many pagetable entries, and in a particular way. 
Also it probably ought to be static.

The reference counting in update_pgtable_entry() is confusing -- it
should probably always do reference counting for both the old and new
entries; that seems more robust than only doing the decrements there and
manually setting count_info and type_info on the new page in
replace_page.

In replace_page(), your error paths are confused: the ENOMEM error case
drops a ref that wasn't taken and if get_page() fails you don't free the
allocated page.

Both of those functions need comments describing what they do and what
their arguments are.

memory_page_offline(): again, check your error and exit paths; I'm
pretty sure you leak references to the domain.  Why does this take a
domain, by the way?  can't it just take a range of MFNs and figure out
the owning domain for each one as it goes?

Also, isn't the returned nr_offlined value always one less than was
requested?  You write back the _index_ of the highest-numbered frame
that you _attempted_ to offline, which is a pretty confusing number.

Other than that, the xen mm patch just needs a good scattering of
comments.

The tools patch is enormous, and seems to copy big chunks of
xc_domain_save into a new file.  And since Xen is now doing the hard
work of pagetable manipulation, I don't think you even need to suspend
the guest -- just pausing it should be enough and is much easier.

If you do need to use the suspend/resume code in later stages of
development, please don't copy it out; just make a libxc function that
calls the existing functions appropriately.

I'll leave page_offline_xen.patch to Keir since he's said he'll do it,
but 700 new lines of code seems like quite a lot -- surely some subsets
of he existing buddy splitting and merging code could be split out and
reused?

Cheers,

Tim.

-- 
Tim Deegan <Tim.Deegan@xxxxxxxxxx>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]

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

<Prev in Thread] Current Thread [Next in Thread>