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

["Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx>]

Tim Deegan <mailto:Tim.Deegan@xxxxxxxxxx> wrote:
> 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.

Tim, thanks for your feedback very much. Yes, the update_pgtable_entry() will 
update potentially very much page table entries, I'm not sure that's the right 
method to achieve it.

> 
> 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.

Sure, I will do like this.

> 
> 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.

Sure, I will update it.

> 
> 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.

But I'm not sure if we can update the P2M table from Xen side, that's the 
reason I did the it in the user space.

-- Yunhong Jiang


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