Re: [PATCH 3/5] xen: allow balloon driver to use more than one memory region

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

On 06/09/11 22:57, Konrad Rzeszutek Wilk wrote:
> On Fri, Aug 19, 2011 at 03:57:18PM +0100, David Vrabel wrote:
>> 
>> +static void __init balloon_add_memory_region(unsigned long start_pfn, unsigned long pages)
> 
> That looks suspiciously like it has more than 80 lines. You ran the
> _all_ of the patches through scripts/checkpath.pl right?

Yes, I used checkpatch.pl but I tend to treat the 80 character limit as
a guideline rather than a hard limit and only pay attention to it if
breaking a line improves readability.

I assume your preference is for an 80 character hard limit?

>>  {
>> -	domid_t domid = DOMID_SELF;
>>  	unsigned long pfn, extra_pfn_end;
>>  	struct page *page;
>> +
>> +	/*
>> +	 * Initialise the balloon with excess memory space.  We need
>> +	 * to make sure we don't add memory which doesn't exist or
>> +	 * logically exist.  The E820 map can be trimmed to be smaller
>> +	 * than the amount of physical memory due to the mem= command
>> +	 * line parameter.  And if this is a 32-bit non-HIGHMEM kernel
>> +	 * on a system with memory which requires highmem to access,
>> +	 * don't try to use it.
> 
> 
> That whole comment is just confusing.. But I do know that you
> just moved it, but I wonder if it makes sense to remove it or
> alter it in the future.

I think the comment is valid.

>> +	 */
>> +	extra_pfn_end = min(min(max_pfn, e820_end_of_ram_pfn()),
>> +			    start_pfn + pages);

It's this line that it's documenting.

>> --- a/include/xen/page.h
>> +++ b/include/xen/page.h
>> @@ -3,6 +3,13 @@
>>  
>>  #include <asm/xen/page.h>
>>  
>> -extern phys_addr_t xen_extra_mem_start, xen_extra_mem_size;
>> +struct xen_memory_region {
>> +	phys_addr_t start;
>> +	phys_addr_t size;
>> +};
>> +
>> +#define XEN_EXTRA_MEM_MAX_REGIONS 128 /* == E820MAX */
>> +
>> +extern struct xen_memory_region xen_extra_mem[XEN_EXTRA_MEM_MAX_REGIONS];
> 
> __initdata

Only the definition (in arch/x86/xen/setup.c) needs the __initdata
attribute, right?

I just checked and it does end up in the .init.data section.

David