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

[Joe Perches <joe@perches.com>]

On Fri, 2021-05-14 at 15:56 -0400, Jason Baron wrote:
> 
> On 5/14/21 1:38 PM, Joe Perches wrote:
> > On Tue, 2021-05-11 at 17:31 -0400, Jason Baron wrote:
> > 
> > > That said, I do see the value in not having to open code the branch stuff, and
> > > making pr_debug() consistent with printk which does return a value. So that
> > > makes sense to me.
> > 
> > IMO: printk should not return a value.
> > 
> 
> Ok, the issue we are trying to resolve is to control whether a 'pr_debug()' statement
> is enabled and thus use that to control subsequent output. The proposed patch does:
> 
> 
> +	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");
> +	}
> 
> I do think pr_debug() here is different from printk() b/c it can be explicitly
> toggled.
> 
> I also suggested an alternative, which is possible with the current code which
> is to use DYNAMIC_DEBUG_BRANCH().
> 
> 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");
> }
> 
> That however does require one to do something like this first:
> 
> DEFINE_DYNAMIC_DEBUG_METADATA(e820_dbg, "e820 verbose mode");
> 
> So I don't feel too strongly either way, but maybe others do?

Why not avoid the problem by using temporaries on the stack
and not use pr_cont altogether?
---
 arch/x86/kernel/e820.c | 71 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 46 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index bc0657f0deed..a6e7ab4b522b 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -184,20 +184,38 @@ void __init e820__range_add(u64 start, u64 size, enum e820_type type)
 	__e820__range_add(e820_table, start, size, type);
 }
 
-static void __init e820_print_type(enum e820_type type)
+static char * __init e820_fmt_type(enum e820_type type, char *buf, size_t size)
 {
 	switch (type) {
-	case E820_TYPE_RAM:		/* Fall through: */
-	case E820_TYPE_RESERVED_KERN:	pr_cont("usable");			break;
-	case E820_TYPE_RESERVED:	pr_cont("reserved");			break;
-	case E820_TYPE_SOFT_RESERVED:	pr_cont("soft reserved");		break;
-	case E820_TYPE_ACPI:		pr_cont("ACPI data");			break;
-	case E820_TYPE_NVS:		pr_cont("ACPI NVS");			break;
-	case E820_TYPE_UNUSABLE:	pr_cont("unusable");			break;
-	case E820_TYPE_PMEM:		/* Fall through: */
-	case E820_TYPE_PRAM:		pr_cont("persistent (type %u)", type);	break;
-	default:			pr_cont("type %u", type);		break;
+	case E820_TYPE_RAM:
+	case E820_TYPE_RESERVED_KERN:
+		strscpy(buf, "usable", size);
+		break;
+	case E820_TYPE_RESERVED:
+		strscpy(buf, "reserved", size);
+		break;
+	case E820_TYPE_SOFT_RESERVED:
+		strscpy(buf, "soft reserved", size);
+		break;
+	case E820_TYPE_ACPI:
+		strscpy(buf, "ACPI data", size);
+		break;
+	case E820_TYPE_NVS:
+		strscpy(buf, "ACPI NVS", size);
+		break;
+	case E820_TYPE_UNUSABLE:
+		strscpy(buf, "unusable", size);
+		break;
+	case E820_TYPE_PMEM:
+	case E820_TYPE_PRAM:
+		scnprintf(buf, size, "persistent (type %u)", type);
+		break;
+	default:
+		scnprintf(buf, size, "type %u", type);
+		break;
 	}
+
+	return buf;
 }
 
 void __init e820__print_table(char *who)
@@ -205,13 +223,14 @@ void __init e820__print_table(char *who)
 	int i;
 
 	for (i = 0; i < e820_table->nr_entries; i++) {
-		pr_info("%s: [mem %#018Lx-%#018Lx] ",
+		char type[32];
+
+		pr_info("%s: [mem %#018Lx-%#018Lx] %s\n",
 			who,
 			e820_table->entries[i].addr,
-			e820_table->entries[i].addr + e820_table->entries[i].size - 1);
-
-		e820_print_type(e820_table->entries[i].type);
-		pr_cont("\n");
+			e820_table->entries[i].addr + e820_table->entries[i].size - 1,
+			e820_fmt_type(e820_table->entries[i].type,
+				      type, sizeof(type)));
 	}
 }
 
@@ -465,6 +484,8 @@ __e820__range_update(struct e820_table *table, u64 start, u64 size, enum e820_ty
 	u64 end;
 	unsigned int i;
 	u64 real_updated_size = 0;
+	char type1[32];
+	char type2[32];
 
 	BUG_ON(old_type == new_type);
 
@@ -472,11 +493,10 @@ __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");
+	printk(KERN_DEBUG "e820: update [mem %#010Lx-%#010Lx] %s ==> %s\n",
+	       start, end - 1,
+	       e820_fmt_type(old_type, type1, sizeof(type1)),
+	       e820_fmt_type(new_type, type2, sizeof(type2)));
 
 	for (i = 0; i < table->nr_entries; i++) {
 		struct e820_entry *entry = &table->entries[i];
@@ -543,15 +563,16 @@ u64 __init e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool
 	int i;
 	u64 end;
 	u64 real_removed_size = 0;
+	char type[32];
 
 	if (size > (ULLONG_MAX - start))
 		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");
+	printk(KERN_DEBUG "e820: remove [mem %#010Lx-%#010Lx]%s%s\n",
+	       start, end - 1,
+	       check_type ? " " : "",
+	       check_type ? e820_fmt_type(old_type, type, sizeof(type)) : "");
 
 	for (i = 0; i < e820_table->nr_entries; i++) {
 		struct e820_entry *entry = &e820_table->entries[i];