Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754860Ab1BTBva (ORCPT ); Sat, 19 Feb 2011 20:51:30 -0500 Received: from smtp-out.google.com ([74.125.121.67]:4058 "EHLO smtp-out.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754222Ab1BTBvJ (ORCPT ); Sat, 19 Feb 2011 20:51:09 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=google.com; s=beta; h=date:from:x-x-sender:to:cc:subject:in-reply-to:message-id :references:user-agent:mime-version:content-type; b=oXjZ+GL6uKwdMi1iUIaUKFCcPYhGdFW1FY6NBC0FWXpBtnXkE2g8C2Jk8ln0Lug2EE YXJ8rjmQbs4x4EtBB0Bg== Date: Sat, 19 Feb 2011 17:51:00 -0800 (PST) From: David Rientjes X-X-Sender: rientjes@chino.kir.corp.google.com To: Mike Travis cc: Ingo Molnar , Jack Steiner , Robin Holt , Len Brown , Thomas Gleixner , "H. Peter Anvin" , Andrew Morton , Yinghai Lu , linux-acpi@vger.kernel.org, x86@kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/6] x86: Minimize initial e820 messages In-Reply-To: <20110219024705.695093866@gulag1.americas.sgi.com> Message-ID: References: <20110219024705.308511527@gulag1.americas.sgi.com> <20110219024705.695093866@gulag1.americas.sgi.com> User-Agent: Alpine 2.00 (DEB 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-System-Of-Record: true Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8881 Lines: 276 On Fri, 18 Feb 2011, Mike Travis wrote: > Minimize the early e820 messages by printing less characters > for the address range as well as abbreviating the type info > for each entry. > > Also the "modified physical RAM map" was mostly a duplicate of > the original e820 memory map printout. Minimize the output > by only printing those entries that were actually modified. > > v1: Added pertinent __init & __initdata specifiers > Changed some inlines to __init functions to avoid the > the compiler un-inlining them into the wrong section. > > v2: updated to apply to x86-tip > > Signed-off-by: Mike Travis > Reviewed-by: Jack Steiner > Reviewed-by: Robin Holt > --- > arch/x86/kernel/e820.c | 138 +++++++++++++++++++++++++++----------------- > arch/x86/platform/efi/efi.c | 10 +-- > 2 files changed, 91 insertions(+), 57 deletions(-) > > --- linux.orig/arch/x86/kernel/e820.c > +++ linux/arch/x86/kernel/e820.c > @@ -39,6 +39,13 @@ > struct e820map e820; > struct e820map e820_saved; > > +/* > + * Keep track of previous e820 mappings so we can reduce the number > + * of messages when printing the "modified" e820 map > + */ > +static struct e820map e820_prev __initdata; > +static int e820_prev_saved __initdata; bool? > + > /* For PCI or other memory-mapped resources */ > unsigned long pci_mem_start = 0xaeedbabe; > #ifdef CONFIG_PCI > @@ -125,42 +132,85 @@ void __init e820_add_region(u64 start, u > __e820_add_region(&e820, start, size, type); > } > > -static void __init e820_print_type(u32 type) > +/* long description */ > +static const char * __init e820_type_to_string(int e820_type) > +{ > + switch (e820_type) { > + case E820_RESERVED_KERN: return "Kernel RAM"; > + case E820_RAM: return "System RAM"; > + case E820_ACPI: return "ACPI Tables"; > + case E820_NVS: return "ACPI Non-Volatile Storage"; > + case E820_UNUSABLE: return "Unusable Memory"; > + default: return "Reserved"; > + } > +} All of the callers of this function would probably benefit from surrounding each return string with ( and ). > + > +/* short description, saves log space when there are 100's of e820 entries */ > +static char * __init e820_types(int e820_type) > { > - switch (type) { > - case E820_RAM: > - case E820_RESERVED_KERN: > - printk(KERN_CONT "(usable)"); > - break; > - case E820_RESERVED: > - printk(KERN_CONT "(reserved)"); > - break; > - case E820_ACPI: > - printk(KERN_CONT "(ACPI data)"); > - break; > - case E820_NVS: > - printk(KERN_CONT "(ACPI NVS)"); > - break; > - case E820_UNUSABLE: > - printk(KERN_CONT "(unusable)"); > - break; > - default: > - printk(KERN_CONT "type %u", type); > - break; > + switch (e820_type) { > + case E820_RESERVED_KERN: return "KRAM"; > + case E820_RAM: return "SRAM"; Using the abbreviation "SRAM" for "system RAM" is just going to lead to confusion. > + case E820_ACPI: return "ACPI"; > + case E820_NVS: return "NVS"; > + case E820_UNUSABLE: return "UM"; I know these are intended to be very short, but nobody is going to conclude that "UM" means unusuable memory. > + default: return "RESVD"; > } > } > > +static void __init e820_print_header(void) > +{ > + pr_info("types: %s=(%s) %s=(%s) %s=(%s) %s=(%s) %s=(%s) %s=(%s)\n", > + e820_types(E820_RESERVED_KERN), > + e820_type_to_string(E820_RESERVED_KERN), > + e820_types(E820_RAM), e820_type_to_string(E820_RAM), > + e820_types(E820_RESERVED), e820_type_to_string(E820_RESERVED), > + e820_types(E820_ACPI), e820_type_to_string(E820_ACPI), > + e820_types(E820_NVS), e820_type_to_string(E820_NVS), > + e820_types(E820_UNUSABLE), e820_type_to_string(E820_UNUSABLE)); > +} > + > +/* compare new entry with old so we only print "modified" entries */ > +static int __init not_modified(int i, int j) I know it's static, but this needs a better function name. I hope it's never passed negative actuals by mistake. > +{ > + for (; j < e820_prev.nr_map && > + e820_prev.map[j].addr <= e820.map[i].addr; j++) { > + > + if (e820.map[i].addr == e820_prev.map[j].addr && > + e820.map[i].size == e820_prev.map[j].size && > + e820.map[i].type == e820_prev.map[j].type) > + return j; > + } > + return 0; > +} > + > void __init e820_print_map(char *who) > { > - int i; > + int i, j = 0; > + int hdr = 0; > + int mod = strcmp(who, "modified") == 0; bool? > > for (i = 0; i < e820.nr_map; i++) { > - printk(KERN_INFO " %s: %016Lx - %016Lx ", who, > - (unsigned long long) e820.map[i].addr, > - (unsigned long long) > - (e820.map[i].addr + e820.map[i].size)); > - e820_print_type(e820.map[i].type); > - printk(KERN_CONT "\n"); > + /* only print those entries that were really modified */ > + if (mod) > + j = not_modified(i, j); > + > + if (!j) { > + if (!hdr++) > + e820_print_header(); > + > + pr_info("%s: %Lx+%Lx (%s)\n", who, We don't want a space prefix anymore? > + (unsigned long long) e820.map[i].addr, > + (unsigned long long) e820.map[i].size, > + e820_types(e820.map[i].type)); > + } > + } > + if (!hdr) > + pr_info("\n"); > + > + if (!e820_prev_saved) { > + memcpy(&e820_prev, &e820, sizeof(struct e820map)); > + e820_prev_saved = 1; > } > } > > @@ -437,13 +487,11 @@ static u64 __init __e820_update_range(st > size = ULLONG_MAX - start; > > end = start + size; > - printk(KERN_DEBUG "e820 update range: %016Lx - %016Lx ", > + pr_debug("e820 update range: %Lx+%Lx %s ==> %s\n", > (unsigned long long) start, > - (unsigned long long) end); > - e820_print_type(old_type); > - printk(KERN_CONT " ==> "); > - e820_print_type(new_type); > - printk(KERN_CONT "\n"); > + (unsigned long long) size, > + e820_type_to_string(old_type), > + e820_type_to_string(new_type)); > > for (i = 0; i < e820x->nr_map; i++) { > struct e820entry *ei = &e820x->map[i]; > @@ -518,12 +566,10 @@ u64 __init e820_remove_range(u64 start, > size = ULLONG_MAX - start; > > end = start + size; > - printk(KERN_DEBUG "e820 remove range: %016Lx - %016Lx ", > + printk(KERN_DEBUG "e820 remove range: %016Lx - %016Lx %s\n", > (unsigned long long) start, > - (unsigned long long) end); > - if (checktype) > - e820_print_type(old_type); > - printk(KERN_CONT "\n"); > + (unsigned long long) end, > + checktype ? e820_type_to_string(old_type) : ""); > > for (i = 0; i < e820.nr_map; i++) { > struct e820entry *ei = &e820.map[i]; > @@ -576,7 +622,7 @@ void __init update_e820(void) > if (sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &nr_map)) > return; > e820.nr_map = nr_map; > - printk(KERN_INFO "modified physical RAM map:\n"); > + printk(KERN_INFO "physical RAM map entries that were modified:\n"); > e820_print_map("modified"); > } > static void __init update_e820_saved(void) > @@ -926,18 +972,6 @@ void __init finish_e820_parsing(void) > } > } > > -static inline const char *e820_type_to_string(int e820_type) > -{ > - switch (e820_type) { > - case E820_RESERVED_KERN: > - case E820_RAM: return "System RAM"; > - case E820_ACPI: return "ACPI Tables"; > - case E820_NVS: return "ACPI Non-volatile Storage"; > - case E820_UNUSABLE: return "Unusable memory"; > - default: return "reserved"; > - } > -} > - > /* > * Mark e820 reserved areas as busy for the resource manager. > */ > --- linux.orig/arch/x86/platform/efi/efi.c > +++ linux/arch/x86/platform/efi/efi.c > @@ -306,11 +306,11 @@ static void __init print_efi_memmap(void > p < memmap.map_end; > p += memmap.desc_size, i++) { > md = p; > - printk(KERN_INFO PFX "mem%02u: type=%u, attr=0x%llx, " > - "range=[0x%016llx-0x%016llx) (%lluMB)\n", > - i, md->type, md->attribute, md->phys_addr, > - md->phys_addr + (md->num_pages << EFI_PAGE_SHIFT), > - (md->num_pages >> (20 - EFI_PAGE_SHIFT))); > + pr_info(PFX > + "mem%u: range %llx+%llx (%lluMB) type %u attr %llx\n", The range you're printing is now ambiguous because you're now showing the length rather than the ending length (implying that what you're displaying is not a range at all, but it's specified as one). I typically don't see "100+12" as describing [100,112), for example. This is also the second time in the patchset where you're printing hex values without a "0x" prefix, that can be confusing and it's not like "0x" is egregiously long. I can understand removing the zero padding, but not the prefix. > + i, md->phys_addr, md->num_pages << EFI_PAGE_SHIFT, > + (md->num_pages >> (20 - EFI_PAGE_SHIFT)), > + md->type, md->attribute); > } > } > #endif /* EFI_DEBUG */ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/