From: Mike Travis <travis@sgi.com>
To: David Rientjes <rientjes@google.com>
Cc: Ingo Molnar <mingo@elte.hu>, Jack Steiner <steiner@sgi.com>,
Robin Holt <holt@sgi.com>, Len Brown <len.brown@intel.com>,
Thomas Gleixner <tglx@linutronix.de>,
"H. Peter Anvin" <hpa@zytor.com>,
Andrew Morton <akpm@linux-foundation.org>,
Yinghai Lu <yhlu.kernel@gmail.com>,
linux-acpi@vger.kernel.org, x86@kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/6] x86: Minimize initial e820 messages
Date: Mon, 21 Feb 2011 12:50:06 -0800 [thread overview]
Message-ID: <4D62CFFE.6050803@sgi.com> (raw)
In-Reply-To: <alpine.DEB.2.00.1102191700030.27722@chino.kir.corp.google.com>
David Rientjes wrote:
> 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 <travis@sgi.com>
>> Reviewed-by: Jack Steiner <steiner@sgi.com>
>> Reviewed-by: Robin Holt <holt@sgi.com>
>> ---
>> 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 ).
Good point, thanks.
>
>> +
>> +/* 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.
Suggestions?
>
>> + 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.
UM, what do you think it should be? ;-)
>
>> + 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.
What (!not) doesn't make sense? ;-)
>
>> +{
>> + 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?
Why would we?
>
>> + (unsigned long long) e820.map[i].addr,
>> + (unsigned long long) e820.map[i].size,
>> + e820_types(e820.map[i].type));
>> + }
>> + }
>> + if (!hdr)
>> + pr_info("<none>\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.
Again, I'm open for suggestions. I thought that changing base-end to base+range
would be coherent?
>
> 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.
The problem comes when there are zillions of them.
>
>> + i, md->phys_addr, md->num_pages << EFI_PAGE_SHIFT,
>> + (md->num_pages >> (20 - EFI_PAGE_SHIFT)),
>> + md->type, md->attribute);
>> }
>> }
>> #endif /* EFI_DEBUG */
next prev parent reply other threads:[~2011-02-21 20:50 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-02-19 2:47 [PATCH 0/6] init: Shrink early messages to prevent overflowing the kernel log buffer Mike Travis
2011-02-19 2:47 ` [PATCH 1/6] ACPI: Minimize X2APIC initial messages Mike Travis
2011-02-20 1:50 ` David Rientjes
2011-02-21 20:42 ` Mike Travis
2011-02-22 0:13 ` David Rientjes
2011-02-19 2:47 ` [PATCH 2/6] x86: Minimize initial e820 messages Mike Travis
2011-02-20 1:51 ` David Rientjes
2011-02-21 20:50 ` Mike Travis [this message]
2011-02-22 0:13 ` David Rientjes
2011-02-22 18:43 ` Bjorn Helgaas
2011-02-19 2:47 ` [PATCH 3/6] x86: Minimize SRAT messages Mike Travis
2011-02-20 1:51 ` David Rientjes
2011-02-23 21:24 ` Mike Travis
2011-02-24 4:12 ` David Rientjes
2011-02-19 2:47 ` [PATCH 4/6] printk: Break out printk_time Mike Travis
2011-02-20 1:51 ` David Rientjes
2011-02-19 2:47 ` [PATCH 5/6] printk: Minimize time zero output Mike Travis
2011-02-20 1:51 ` David Rientjes
2011-02-21 20:56 ` Mike Travis
2011-02-22 0:13 ` David Rientjes
2011-02-19 2:47 ` [PATCH 6/6] printk: Allocate kernel log buffer earlier Mike Travis
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4D62CFFE.6050803@sgi.com \
--to=travis@sgi.com \
--cc=akpm@linux-foundation.org \
--cc=holt@sgi.com \
--cc=hpa@zytor.com \
--cc=len.brown@intel.com \
--cc=linux-acpi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@elte.hu \
--cc=rientjes@google.com \
--cc=steiner@sgi.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
--cc=yhlu.kernel@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox