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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Wed, Sep 07, 2011 at 11:44:36AM +0100, David Vrabel wrote:
> 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?

Please.
> 
> >>  {
> >> -	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.

Can define "logically exist" ?

or "non-HIGHMEM kernel .. don't try to use it' - but we do
use it.
> 
> >> +	 */
> >> +	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?

Right, but you might wnat to put in here too so folks won't try to
use past __init. Or just stick a comment in there warning folks
about it.

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