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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home