Re: [PATCH 2/6] x86: Minimize initial e820 messages

[David Rientjes]

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@xxxxxxx>
> Reviewed-by: Jack Steiner <steiner@xxxxxxx>
> Reviewed-by: Robin Holt <holt@xxxxxxx>
> ---
>  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  */
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/