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

[David Vrabel <david.vrabel@xxxxxxxxxx>]

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

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