[Xen-devel] Re: [PATCH] GSoC 2010 - Memory hotplug support for Xen guests - third fully working version

[Jeremy Fitzhardinge <jeremy@xxxxxxxx>]

 On 08/11/2010 06:22 PM, Daniel Kiper wrote:
> > 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.
> I will do that if this patch will be accepted.
First step is to post it to lkml for discussion, cc:ing the relevant 
maintainers. (I'm not really sure who that is at the moment.  It will 
take some digging around in the history.)
> > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > > index 38434da..beb1aa7 100644
> > > --- a/arch/x86/Kconfig
> > > +++ b/arch/x86/Kconfig
> > > @@ -1273,7 +1273,7 @@ config ARCH_SELECT_MEMORY_MODEL
> > >         depends on ARCH_SPARSEMEM_ENABLE
> > >
> > >   config ARCH_MEMORY_PROBE
> > > -       def_bool y
> > > +       def_bool X86_64&&   !XEN
> > >         depends on MEMORY_HOTPLUG
> > 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?
> There is no simple way to do that. It requiers to do some
> changes in drivers/base/memory.c code. I think it should
> be done as kernel boot option (on by default to not break
> things using this interface now). If it be useful for maintainers
> of mm/memory_hotplug.c and drivers/base/memory.c code then
> I could do that. Currently original arch/x86/Kconfig version
> is restored.
I think adding a global flag which the Xen balloon driver can disable 
should be sufficient.  There's no need to make an separate user-settable 
control.
> > > +/* we are OK calling __meminit stuff here - we have CONFIG_MEMORY_HOTPLUG
> > > */
> > > +static int __ref xen_add_memory(int nid, u64 start, u64 size)
> > Could this be __meminit too then?
> Good question. I looked throught the code and could
> not find any simple explanation why mm/memory_hotplug.c
> authors used __ref instead __meminit. Could you (mm/memory_hotplug.c
> authors/maintainers) tell us why ???
Quite possibly a left-over from something else.  You could just try 
making it __meminit, then compile with, erm, the option which shows you 
section conflicts (it shows the number of conflicts at the end of the 
kernel build by default, and tells you how to explicitly list them).
> > > +{
> > > +       pg_data_t *pgdat = NULL;
> > > +       int new_pgdat = 0, ret;
> > > +
> > > +       lock_system_sleep();
> > 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?
> Yes, because as I know S3 state is supported by Xen guests.
Yes, but I'm assuming the interaction between S3 and ACPI hotplug memory 
isn't something that concerns a Xen guest; our hotplug mechanism is 
completely different.
> > > +               r->name = "System RAM";
> > How about making it clear its Xen hotplug RAM?  Or do things care about
> > the "System RAM" name?
> As I know no however as I saw anybody do not differentiate between
> normal and hotplugged memory. I thought about that ealier however
> stated that this soultion does not give us any real gain. That is why
> I decided to use standard name for hotplugged memory.
Its cosmetic, but it would be useful to see what's going on.

I'll send more detailed comments on the whole patch in a separate mail.

    J

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