From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH 3/5] xen: allow balloon driver to use more than one memory region Date: Wed, 7 Sep 2011 14:09:33 -0400 Message-ID: <20110907180933.GA5888@dumpdata.com> References: <1313765840-22084-1-git-send-email-david.vrabel@citrix.com> <1313765840-22084-4-git-send-email-david.vrabel@citrix.com> <20110906215706.GC24860@dumpdata.com> <4E674B14.4090306@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <4E674B14.4090306@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: David Vrabel Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org 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 > >> > >> -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