Re: [PATCH] GSoC 2010 - Memory hotplug support for Xen guests - second fully working version - once again

Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
From: Jeremy Fitzhardinge
Date: Friday, August 6, 2010 - 10:42 am

On 08/06/2010 04:11 AM, Daniel Kiper wrote:

Likely, depending on how they're used.  If you're using them as a 
sensible API (which looks like the case) then there should be no problem.


Good.  I noticed you have some specific tests for "xen_pv_domain()" - 
are there many differences between pv and hvm?


OK.  Thanks for posting a delta against your previous patch; it makes it 
much easier to see what changes you've made.


But surely they can be combined?  A system without 
XEN_BALLOON_MEMORY_HOTPLUG is identical to a system with 
XEN_BALLOON_MEMORY_HOTPLUG which hasn't yet added any memory.  Some 
variables may become constants (because memory can never be hot-added), 
but the logic of the code should be the same.


OK, see below.  I think you can pull all the common code out into a 
separate function rather than duplicating it.


Overall, this looks much better.  The next step is to split this into at 
least two patches: one for the core code, and one for the Xen bits.  
Each patch should do just one logical operation, so if you have several 
distinct changes to the core code, put them in separate patches.

More comments inline.


The trouble with making anything statically depend on Xen at config time 
is that you lose it even if you're not running under Xen.  A pvops 
kernel can run on bare hardware as well, and we don't want to lose 
functionality (assume that CONFIG_XEN is always set, since distros do 
always set it).

Can you find a clean way to prevent/disable ARCH_MEMORY_PROBE at runtime 
when in a Xen context?


Could this be __meminit too then?


What's this for?  I see all its other users are in the memory hotplug 
code, but presumably they're concerned about a real S3 suspend.  Do we 
care about that here?


Actually, this is nearly identical to mm/memory_hotplug.c:add_memory().  
It looks to me like you should:

    * pull the common core out into mm/memory_hotplug.c:__add_memory()
      (or a better name)
    * make add_memory() do its
      register_memory_resource()/firmware_map_add_hotplug() around that
      (assuming they're definitely unwanted in the Xen case)
    * make xen_add_memory() just call __add_memory() along with whatever
      else it needs (which is nothing?)

That way you can export a high-level __add_memory function from 
memory_hotplug.c rather than the two internal detail functions.


How about making it clear its Xen hotplug RAM?  Or do things care about 
the "System RAM" name?


Thanks,
     J
--
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
Re: [PATCH] GSoC 2010 - Memory hotplug support for Xen gue ..., Konrad Rzeszutek Wilk, (Fri Aug 6, 9:34 am)
Re: [PATCH] GSoC 2010 - Memory hotplug support for Xen gue ..., Jeremy Fitzhardinge, (Fri Aug 6, 10:42 am)
Re: [PATCH] GSoC 2010 - Memory hotplug support for Xen gue ..., Jeremy Fitzhardinge, (Fri Aug 6, 12:20 pm)
Re: [PATCH] GSoC 2010 - Memory hotplug support for Xen gue ..., Konrad Rzeszutek Wilk, (Fri Aug 6, 2:17 pm)