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 <[email protected]>
Reviewed-by: Jack Steiner <[email protected]>
Reviewed-by: Robin Holt <[email protected]>
---
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;
+
/* 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";
+ }
+}
+
+/* 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";
+ case E820_ACPI: return "ACPI";
+ case E820_NVS: return "NVS";
+ case E820_UNUSABLE: return "UM";
+ 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)
+{
+ 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;
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,
+ (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",
+ i, md->phys_addr, md->num_pages << EFI_PAGE_SHIFT,
+ (md->num_pages >> (20 - EFI_PAGE_SHIFT)),
+ md->type, md->attribute);
}
}
#endif /* EFI_DEBUG */
--
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 <[email protected]>
> Reviewed-by: Jack Steiner <[email protected]>
> Reviewed-by: Robin Holt <[email protected]>
> ---
> 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("<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.
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 */
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 <[email protected]>
>> Reviewed-by: Jack Steiner <[email protected]>
>> Reviewed-by: Robin Holt <[email protected]>
>> ---
>> 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 */
On Mon, 21 Feb 2011, Mike Travis wrote:
> > > --- 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?
>
Well, SRAM is just unacceptable for obvious reasons. I hope something
like "SysRAM" and "KernRAM" isn't too long.
> >
> > > + 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? ;-)
>
lol, probably just "unusable."
> >
> > > + 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? ;-)
>
It's a poor function name because it's way too generic, it should include
e820 in some way. e820_entry_is_unmodified(), for example.
> > > 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?
To differentiate it from the other messages since we're printing multiple
lines that only make sense in relationship to one another (like how we
print stacks on x86).
> >
> > > + (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?
A range consists of a start and an end, so you changed base-end to
base+length, not base+range. If I see "100+12", I see that as 112 because
it seems like it's either arithmetic or an offset from base 100. The last
thing that comes to mind is [100,112), though. Changing this isn't really
a part of the goals of your patchset either.
> > 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'd be surprised if you can't spare two characters for a hex value in the
kernel log.
On Friday, February 18, 2011 07:47:07 pm 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.
> + pr_info("%s: %Lx+%Lx (%s)\n", who,
> + (unsigned long long) e820.map[i].addr,
> + (unsigned long long) e820.map[i].size,
> + e820_types(e820.map[i].type));
If we're going to change the way we print E820 ranges, I think
we should make them consistent with the way we handle %pR, e.g.,
use something like this:
"%s: [mem %#018Lx-%#018Lx]"
This is a little different because %pR wouldn't make the field
so wide, but when we discussed this earlier, keeping the table
alignment was thought to be important. That discussion starts
here: https://lkml.org/lkml/2010/9/22/248
Same comment applies to the update, remove, PCI gap (which
your patch currently doesn't touch), and EFI output.
I know this kind of derails the whole "remove characters" point
of your patch (and I support that in principle), but I do think
consistency is important, too. We have too many different ways
of printing the same information.
Bjorn