Messages in this thread | | | Subject | Re: [PATCH 2/2] x86/e820: Use pr_debug to avoid spamming dmesg log with debug messages | From | Jason Baron <> | Date | Fri, 14 May 2021 16:32:02 -0400 |
| |
On 5/14/21 4:22 PM, Joe Perches wrote: > 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?
Nice. That's fine with me Heiner?
I think though Heiner wants to use pr_debug() below instead of printk with KERN_DEBUG, at least that was my understanding. But that would be a simpple s/printk/pr_debug().
Thanks,
-Jason
> --- > 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]; > >
| |