Re: [PATCH 2/2] x86/e820: Use pr_debug to avoid spamming dmesg log with debug messages

[Heiner Kallweit <hkallweit1@gmail.com>]

On 05.05.2021 18:58, Jason Baron wrote:
> 
> 
> On 5/3/21 3:40 PM, Heiner Kallweit wrote:
>> e820 emits quite some debug messages to the dmesg log. Let's restrict
>> this to cases where the debug output is actually requested. Switch to
>> pr_debug() for this purpose and make sure by checking the return code
>> that pr_cont() is only called if applicable.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  arch/x86/kernel/e820.c | 27 ++++++++++++++++-----------
>>  1 file changed, 16 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>> index bc0657f0d..67ad4d8f0 100644
>> --- a/arch/x86/kernel/e820.c
>> +++ b/arch/x86/kernel/e820.c
>> @@ -465,6 +465,7 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty
>>  	u64 end;
>>  	unsigned int i;
>>  	u64 real_updated_size = 0;
>> +	int printed;
>>  
>>  	BUG_ON(old_type == new_type);
>>  
>> @@ -472,11 +473,13 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty
>>  		size = ULLONG_MAX - start;
>>  
>>  	end = start + size;
>> -	printk(KERN_DEBUG "e820: update [mem %#010Lx-%#010Lx] ", start, end - 1);
>> -	e820_print_type(old_type);
>> -	pr_cont(" ==> ");
>> -	e820_print_type(new_type);
>> -	pr_cont("\n");
>> +	printed = pr_debug("e820: update [mem %#010Lx-%#010Lx] ", start, end - 1);
>> +	if (printed > 0) {
>> +		e820_print_type(old_type);
>> +		pr_cont(" ==> ");
>> +		e820_print_type(new_type);
>> +		pr_cont("\n");
>> +	}
> 
> 
> Hi Heiner,
> 
> We've been doing these like:
> 
> DEFINE_DYNAMIC_DEBUG_METADATA(e820_dbg, "e820 verbose mode");
> 
> .
> .
> .
> 
> if (DYNAMIC_DEBUG_BRANCH(e820_debg)) {
>     printk(KERN_DEBUG "e820: update [mem %#010Lx-%#010Lx] ", start, end - 1);
>     e820_print_type(old_type);
>     pr_cont(" ==> ");
>     e820_print_type(new_type);
>     pr_cont("\n");
> }
> 
> 
> You could then have one DEFINE_DYNAMIC_DEBUG_METADATA statement - such that it enables
> it all in one go, or do separate ones that enable it how you see fit.
> 
> Would that work here?
> 

How would we handle the case that CONFIG_DYNAMIC_DEBUG_CORE isn't defined?
Then also DEFINE_DYNAMIC_DEBUG_METADATA isn't defined and we'd need to
duplicate the logic used here:

#if defined(CONFIG_DYNAMIC_DEBUG) || \
	(defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))
#include <linux/dynamic_debug.h>
#define pr_debug(fmt, ...)			\
	dynamic_pr_debug(fmt, ##__VA_ARGS__)
#elif defined(DEBUG)
#define pr_debug(fmt, ...) \
	printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
#else
#define pr_debug(fmt, ...) \
	no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
#endif

IMO it's better to have the complexity of using DEFINE_DYNAMIC_DEBUG_METADATA
only once in the implementation of dynamic_pr_debug(), and not in every
code that wants to use pr_debug() in combination with pr_cont().

Also I think that to a certain extent pr_debug() is broken currently in case
of dynamic debugging because it has no return value, one drawback of
using not type-safe macros. This doesn't hurt so far because no caller seems to
check the return value or very few people have dynamic debugging enabled.

> Thanks,
> 
> -Jason
> 

Heiner

>>  
>>  	for (i = 0; i < table->nr_entries; i++) {
>>  		struct e820_entry *entry = &table->entries[i];
>> @@ -540,7 +543,7 @@ static u64 __init e820__range_update_kexec(u64 start, u64 size, enum e820_type o
>>  /* Remove a range of memory from the E820 table: */
>>  u64 __init e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type)
>>  {
>> -	int i;
>> +	int printed, i;
>>  	u64 end;
>>  	u64 real_removed_size = 0;
>>  
>> @@ -548,10 +551,12 @@ u64 __init e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool
>>  		size = ULLONG_MAX - start;
>>  
>>  	end = start + size;
>> -	printk(KERN_DEBUG "e820: remove [mem %#010Lx-%#010Lx] ", start, end - 1);
>> -	if (check_type)
>> -		e820_print_type(old_type);
>> -	pr_cont("\n");
>> +	printed = pr_debug("e820: remove [mem %#010Lx-%#010Lx] ", start, end - 1);
>> +	if (printed > 0) {
>> +		if (check_type)
>> +			e820_print_type(old_type);
>> +		pr_cont("\n");
>> +	}
>>  
>>  	for (i = 0; i < e820_table->nr_entries; i++) {
>>  		struct e820_entry *entry = &e820_table->entries[i];
>> @@ -1230,7 +1235,7 @@ void __init e820__reserve_resources_late(void)
>>  		if (start >= end)
>>  			continue;
>>  
>> -		printk(KERN_DEBUG "e820: reserve RAM buffer [mem %#010llx-%#010llx]\n", start, end);
>> +		pr_debug("e820: reserve RAM buffer [mem %#010llx-%#010llx]\n", start, end);
>>  		reserve_region_with_split(&iomem_resource, start, end, "RAM buffer");
>>  	}
>>  }
>>