Re: [PATCH 3/3] xen: use maximum reservation to limit dom0 memory

[David Vrabel <david.vrabel@citrix.com>]

On 16/08/11 14:53, Konrad Rzeszutek Wilk wrote:
> On Tue, Aug 16, 2011 at 11:00:38AM +0100, David Vrabel wrote:
>> Use the maximum reservation hypercall to set limit the amount of
>> usable dom0 memory.  This reduces the size of pages tables etc. if
>> dom0 is to use less memory than the maximum available.
> 
> Ok, so it sounds like this patch by itself can fix the "more page tables
> than we need" issue.

This patch with the Xen patch does, yes.

> If so, I would prefer that you stick the tiny piece of code that
> calls the xen_get_max_pages() from the setup in this patch. This way
> we can backport this particular patch to stable tree without including
> the other patchsets you have posted. And it is a nicely contained
> one-patch-fixes-the-problem.

Does this problem need to be fixed in stable?  It has a simple
workaround (the 'mem' kernel command line option) and requires an
updated Xen.

I do think that patches #1 and #2 are useful because they allow 32-bit
guests to have more low memory, rather than making all balloon memory
high memory.

I could rearrange the order. Make #3 first so it can also be applied to
3.0.n and 3.1 and then #1 and #2 could be queued for 3.2.

>> Note this requires a patched Xen that sets max_pages when creating dom0.
> 
> Please mention in the description the c/s and the name of the patch.

Ok.

>> --- a/arch/x86/xen/setup.c
>> +++ b/arch/x86/xen/setup.c
>> @@ -197,7 +197,12 @@ static unsigned long __init xen_get_max_pages(void)
>>  	unsigned long max_pages = MAX_DOMAIN_PAGES; /* Limited by memory map. */
>>  
>>  	if (xen_initial_domain()) {
>> -		/* FIXME: ask hypervisor for max pages. */
>> +		domid_t domid = DOMID_SELF;
>> +		int ret;
>> +
>> +		ret = HYPERVISOR_memory_op(XENMEM_maximum_reservation, &domid);
>> +		if (ret > 0)
>> +			max_pages = ret;
> Don't you want to clamp it? Say MAX_DOMAIN_PAGES is set to 1GB, and you
> set it to 2GB here - that will blow the P2M out. Perhaps

It is...

>         max_pages = min(ret, max_pages); ?
> 
>>  	}
>>  
>>  	return min(max_pages, MAX_DOMAIN_PAGES);

... here.

David