Re: [RFC][PATCH] Basic support for page offline

[Tim Deegan <Tim.Deegan@citrix.com>]

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@citrix.com>
Principal Software Engineer, Citrix Systems (R&D) Ltd.
[Company #02300071, SL9 0DZ, UK.]