Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933339AbbLBRkd (ORCPT ); Wed, 2 Dec 2015 12:40:33 -0500 Received: from mail-pa0-f54.google.com ([209.85.220.54]:35138 "EHLO mail-pa0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933141AbbLBRk2 convert rfc822-to-8bit (ORCPT ); Wed, 2 Dec 2015 12:40:28 -0500 Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 9.0 \(3094\)) Subject: Re: [PATCH 1/2] mm, printk: introduce new format string for flags From: yalin wang In-Reply-To: <1448899821-9671-1-git-send-email-vbabka@suse.cz> Date: Wed, 2 Dec 2015 09:40:26 -0800 Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton , Joonsoo Kim , Minchan Kim , Sasha Levin , "Kirill A. Shutemov" , Mel Gorman , Michal Hocko , Rasmus Villemoes Content-Transfer-Encoding: 8BIT Message-Id: <4EAD2C33-D0E4-4DEB-92E5-9C0457E8635C@gmail.com> References: <20151125143010.GI27283@dhcp22.suse.cz> <1448899821-9671-1-git-send-email-vbabka@suse.cz> To: Vlastimil Babka X-Mailer: Apple Mail (2.3094) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 15144 Lines: 454 > On Nov 30, 2015, at 08:10, Vlastimil Babka wrote: > > In mm we use several kinds of flags bitfields that are sometimes printed for > debugging purposes, or exported to userspace via sysfs. To make them easier to > interpret independently on kernel version and config, we want to dump also the > symbolic flag names. So far this has been done with repeated calls to > pr_cont(), which is unreliable on SMP, and not usable for e.g. sysfs export. > > To get a more reliable and universal solution, this patch extends printk() > format string for pointers to handle the page flags (%pgp), gfp_flags (%pgg) > and vma flags (%pgv). Existing users of dump_flag_names() are converted and > simplified. > > It would be possible to pass flags by value instead of pointer, but the %p > format string for pointers already has extensions for various kernel > structures, so it's a good fit, and the extra indirection in a non-critical > path is negligible. > > Signed-off-by: Vlastimil Babka > Cc: Rasmus Villemoes > --- > I'm sending it on top of the page_owner series, as it's already in mmotm. > But to reduce churn (in case this approach is accepted), I can later > incorporate it and resend it whole. > > Documentation/printk-formats.txt | 14 ++++ > include/linux/mmdebug.h | 5 +- > lib/vsprintf.c | 31 ++++++++ > mm/debug.c | 150 ++++++++++++++++++++++----------------- > mm/oom_kill.c | 5 +- > mm/page_alloc.c | 5 +- > mm/page_owner.c | 5 +- > 7 files changed, 140 insertions(+), 75 deletions(-) > > diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt > index b784c270105f..4b5156e74b09 100644 > --- a/Documentation/printk-formats.txt > +++ b/Documentation/printk-formats.txt > @@ -292,6 +292,20 @@ Raw pointer value SHOULD be printed with %p. The kernel supports > > Passed by reference. > > +Flags bitfields such as page flags, gfp_flags: > + > + %pgp 0x1fffff8000086c(referenced|uptodate|lru|active|private) > + %pgg 0x24202c4(GFP_USER|GFP_DMA32|GFP_NOWARN) > + %pgv 0x875(read|exec|mayread|maywrite|mayexec|denywrite) > + > + For printing raw values of flags bitfields together with symbolic > + strings that would construct the value. The type of flags is given by > + the third character. Currently supported are [p]age flags, [g]fp_flags > + and [v]ma_flags. The flag names and print order depends on the > + particular type. > + > + Passed by reference. > + > Network device features: > > %pNF 0x000000000000c000 > diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h > index 3b77fab7ad28..e6518df259ca 100644 > --- a/include/linux/mmdebug.h > +++ b/include/linux/mmdebug.h > @@ -2,6 +2,7 @@ > #define LINUX_MM_DEBUG_H 1 > > #include > +#include > > struct page; > struct vm_area_struct; > @@ -10,7 +11,9 @@ struct mm_struct; > extern void dump_page(struct page *page, const char *reason); > extern void dump_page_badflags(struct page *page, const char *reason, > unsigned long badflags); > -extern void dump_gfpflag_names(unsigned long gfp_flags); > +extern char *format_page_flags(unsigned long flags, char *buf, char *end); > +extern char *format_vma_flags(unsigned long flags, char *buf, char *end); > +extern char *format_gfp_flags(gfp_t gfp_flags, char *buf, char*end); > void dump_vma(const struct vm_area_struct *vma); > void dump_mm(const struct mm_struct *mm); > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index f9cee8e1233c..41cd122bd307 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -31,6 +31,7 @@ > #include > #include > #include > +#include > > #include /* for PAGE_SIZE */ > #include /* for dereference_function_descriptor() */ > @@ -1361,6 +1362,29 @@ char *clock(char *buf, char *end, struct clk *clk, struct printf_spec spec, > } > } > > +static noinline_for_stack > +char *flags_string(char *buf, char *end, void *flags_ptr, > + struct printf_spec spec, const char *fmt) > +{ > + unsigned long flags; > + gfp_t gfp_flags; > + > + switch (fmt[1]) { > + case 'p': > + flags = *(unsigned long *)flags_ptr; > + return format_page_flags(flags, buf, end); > + case 'v': > + flags = *(unsigned long *)flags_ptr; > + return format_vma_flags(flags, buf, end); > + case 'g': > + gfp_flags = *(gfp_t *)flags_ptr; > + return format_gfp_flags(gfp_flags, buf, end); > + default: > + WARN_ONCE(1, "Unsupported flags modifier: %c\n", fmt[1]); > + return 0; > + } > +} > + > int kptr_restrict __read_mostly; > > /* > @@ -1448,6 +1472,11 @@ int kptr_restrict __read_mostly; > * - 'Cn' For a clock, it prints the name (Common Clock Framework) or address > * (legacy clock framework) of the clock > * - 'Cr' For a clock, it prints the current rate of the clock > + * - 'g' For flags to be printed as a collection of symbolic strings that would > + * construct the specific value. Supported flags given by option: > + * p page flags (see struct page) given as pointer to unsigned long > + * g gfp flags (GFP_* and __GFP_*) given as pointer to gfp_t > + * v vma flags (VM_*) given as pointer to unsigned long > * > * ** Please update also Documentation/printk-formats.txt when making changes ** > * > @@ -1600,6 +1629,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr, > return dentry_name(buf, end, > ((const struct file *)ptr)->f_path.dentry, > spec, fmt); > + case 'g': > + return flags_string(buf, end, ptr, spec, fmt); > } > spec.flags |= SMALL; > if (spec.field_width == -1) { > diff --git a/mm/debug.c b/mm/debug.c > index 2fdf0999e6f9..a092111920e7 100644 > --- a/mm/debug.c > +++ b/mm/debug.c > @@ -59,40 +59,109 @@ static const struct trace_print_flags pageflag_names[] = { > #endif > }; > > +static const struct trace_print_flags vmaflags_names[] = { > + {VM_READ, "read" }, > + {VM_WRITE, "write" }, > + {VM_EXEC, "exec" }, > + {VM_SHARED, "shared" }, > + {VM_MAYREAD, "mayread" }, > + {VM_MAYWRITE, "maywrite" }, > + {VM_MAYEXEC, "mayexec" }, > + {VM_MAYSHARE, "mayshare" }, > + {VM_GROWSDOWN, "growsdown" }, > + {VM_PFNMAP, "pfnmap" }, > + {VM_DENYWRITE, "denywrite" }, > + {VM_LOCKONFAULT, "lockonfault" }, > + {VM_LOCKED, "locked" }, > + {VM_IO, "io" }, > + {VM_SEQ_READ, "seqread" }, > + {VM_RAND_READ, "randread" }, > + {VM_DONTCOPY, "dontcopy" }, > + {VM_DONTEXPAND, "dontexpand" }, > + {VM_ACCOUNT, "account" }, > + {VM_NORESERVE, "noreserve" }, > + {VM_HUGETLB, "hugetlb" }, > +#if defined(CONFIG_X86) > + {VM_PAT, "pat" }, > +#elif defined(CONFIG_PPC) > + {VM_SAO, "sao" }, > +#elif defined(CONFIG_PARISC) || defined(CONFIG_METAG) || defined(CONFIG_IA64) > + {VM_GROWSUP, "growsup" }, > +#elif !defined(CONFIG_MMU) > + {VM_MAPPED_COPY, "mappedcopy" }, > +#else > + {VM_ARCH_1, "arch_1" }, > +#endif > + {VM_DONTDUMP, "dontdump" }, > +#ifdef CONFIG_MEM_SOFT_DIRTY > + {VM_SOFTDIRTY, "softdirty" }, > +#endif > + {VM_MIXEDMAP, "mixedmap" }, > + {VM_HUGEPAGE, "hugepage" }, > + {VM_NOHUGEPAGE, "nohugepage" }, > + {VM_MERGEABLE, "mergeable" }, > +}; > + > static const struct trace_print_flags gfpflag_names[] = { > __def_gfpflag_names > }; > > -static void dump_flag_names(unsigned long flags, > - const struct trace_print_flags *names, int count) > +static char *format_flag_names(unsigned long flags, unsigned long mask_out, > + const struct trace_print_flags *names, int count, > + char *buf, char *end) > { > const char *delim = ""; > unsigned long mask; > int i; > > - pr_cont("("); > + buf += snprintf(buf, end - buf, "%#lx(", flags); > + > + flags &= ~mask_out; > > for (i = 0; i < count && flags; i++) { > + if (buf >= end) > + break; > > mask = names[i].mask; > if ((flags & mask) != mask) > continue; > > flags &= ~mask; > - pr_cont("%s%s", delim, names[i].name); > + buf += snprintf(buf, end - buf, "%s%s", delim, names[i].name); > delim = "|"; > } > > /* check for left over flags */ > - if (flags) > - pr_cont("%s%#lx", delim, flags); > + if (flags && (buf < end)) > + buf += snprintf(buf, end - buf, "%s%#lx", delim, flags); > + > + if (buf < end) { > + *buf = ')'; > + buf++; > + } > > - pr_cont(")\n"); > + return buf; > } > > -void dump_gfpflag_names(unsigned long gfp_flags) > +char *format_page_flags(unsigned long flags, char *buf, char *end) > { > - dump_flag_names(gfp_flags, gfpflag_names, ARRAY_SIZE(gfpflag_names)); > + /* remove zone id */ > + unsigned long mask = (1UL << NR_PAGEFLAGS) - 1; > + > + return format_flag_names(flags, ~mask, pageflag_names, > + ARRAY_SIZE(pageflag_names), buf, end); > +} > + > +char *format_vma_flags(unsigned long flags, char *buf, char *end) > +{ > + return format_flag_names(flags, 0, vmaflags_names, > + ARRAY_SIZE(vmaflags_names), buf, end); > +} > + > +char *format_gfp_flags(gfp_t gfp_flags, char *buf, char *end) > +{ > + return format_flag_names(gfp_flags, 0, gfpflag_names, > + ARRAY_SIZE(gfpflag_names), buf, end); > } > > void dump_page_badflags(struct page *page, const char *reason, > @@ -108,18 +177,15 @@ void dump_page_badflags(struct page *page, const char *reason, > pr_cont("\n"); > BUILD_BUG_ON(ARRAY_SIZE(pageflag_names) != __NR_PAGEFLAGS); > > - pr_emerg("flags: %#lx", printflags); > + pr_emerg("flags: %pgp\n", &printflags); > /* remove zone id */ > printflags &= (1UL << NR_PAGEFLAGS) - 1; > - dump_flag_names(printflags, pageflag_names, ARRAY_SIZE(pageflag_names)); > > if (reason) > pr_alert("page dumped because: %s\n", reason); > if (page->flags & badflags) { > printflags = page->flags & badflags; > - pr_alert("bad because of flags: %#lx:", printflags); > - dump_flag_names(printflags, pageflag_names, > - ARRAY_SIZE(pageflag_names)); > + pr_alert("bad because of flags: %pgp\n", &printflags); > } > #ifdef CONFIG_MEMCG > if (page->mem_cgroup) > @@ -136,63 +202,19 @@ EXPORT_SYMBOL(dump_page); > > #ifdef CONFIG_DEBUG_VM > > -static const struct trace_print_flags vmaflags_names[] = { > - {VM_READ, "read" }, > - {VM_WRITE, "write" }, > - {VM_EXEC, "exec" }, > - {VM_SHARED, "shared" }, > - {VM_MAYREAD, "mayread" }, > - {VM_MAYWRITE, "maywrite" }, > - {VM_MAYEXEC, "mayexec" }, > - {VM_MAYSHARE, "mayshare" }, > - {VM_GROWSDOWN, "growsdown" }, > - {VM_PFNMAP, "pfnmap" }, > - {VM_DENYWRITE, "denywrite" }, > - {VM_LOCKONFAULT, "lockonfault" }, > - {VM_LOCKED, "locked" }, > - {VM_IO, "io" }, > - {VM_SEQ_READ, "seqread" }, > - {VM_RAND_READ, "randread" }, > - {VM_DONTCOPY, "dontcopy" }, > - {VM_DONTEXPAND, "dontexpand" }, > - {VM_ACCOUNT, "account" }, > - {VM_NORESERVE, "noreserve" }, > - {VM_HUGETLB, "hugetlb" }, > -#if defined(CONFIG_X86) > - {VM_PAT, "pat" }, > -#elif defined(CONFIG_PPC) > - {VM_SAO, "sao" }, > -#elif defined(CONFIG_PARISC) || defined(CONFIG_METAG) || defined(CONFIG_IA64) > - {VM_GROWSUP, "growsup" }, > -#elif !defined(CONFIG_MMU) > - {VM_MAPPED_COPY, "mappedcopy" }, > -#else > - {VM_ARCH_1, "arch_1" }, > -#endif > - {VM_DONTDUMP, "dontdump" }, > -#ifdef CONFIG_MEM_SOFT_DIRTY > - {VM_SOFTDIRTY, "softdirty" }, > -#endif > - {VM_MIXEDMAP, "mixedmap" }, > - {VM_HUGEPAGE, "hugepage" }, > - {VM_NOHUGEPAGE, "nohugepage" }, > - {VM_MERGEABLE, "mergeable" }, > -}; > - > void dump_vma(const struct vm_area_struct *vma) > { > pr_emerg("vma %p start %p end %p\n" > "next %p prev %p mm %p\n" > "prot %lx anon_vma %p vm_ops %p\n" > - "pgoff %lx file %p private_data %p\n", > + "pgoff %lx file %p private_data %p\n" > + "flags: %pgv\n", > vma, (void *)vma->vm_start, (void *)vma->vm_end, vma->vm_next, > vma->vm_prev, vma->vm_mm, > (unsigned long)pgprot_val(vma->vm_page_prot), > vma->anon_vma, vma->vm_ops, vma->vm_pgoff, > - vma->vm_file, vma->vm_private_data); > - pr_emerg("flags: %#lx", vma->vm_flags); > - dump_flag_names(vma->vm_flags, vmaflags_names, > - ARRAY_SIZE(vmaflags_names)); > + vma->vm_file, vma->vm_private_data, > + &vma->vm_flags); > } > EXPORT_SYMBOL(dump_vma); > > @@ -263,9 +285,7 @@ void dump_mm(const struct mm_struct *mm) > "" /* This is here to not have a comma! */ > ); > > - pr_emerg("def_flags: %#lx", mm->def_flags); > - dump_flag_names(mm->def_flags, vmaflags_names, > - ARRAY_SIZE(vmaflags_names)); > + pr_emerg("def_flags: %pgv\n", &mm->def_flags); > } > > #endif /* CONFIG_DEBUG_VM */ > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 542d56c93209..63a68b62ee68 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -387,10 +387,9 @@ static void dump_header(struct oom_control *oc, struct task_struct *p, > struct mem_cgroup *memcg) > { > pr_warning("%s invoked oom-killer: order=%d, oom_score_adj=%hd, " > - "gfp_mask=0x%x", > + "gfp_mask=%pgg\n", > current->comm, oc->order, current->signal->oom_score_adj, > - oc->gfp_mask); > - dump_gfpflag_names(oc->gfp_mask); > + &oc->gfp_mask); > > cpuset_print_current_mems_allowed(); > dump_stack(); > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 80349acd8c17..77d2c75f80e4 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -2711,9 +2711,8 @@ void warn_alloc_failed(gfp_t gfp_mask, unsigned int order, const char *fmt, ...) > va_end(args); > } > > - pr_warn("%s: page allocation failure: order:%u, mode:0x%x", > - current->comm, order, gfp_mask); > - dump_gfpflag_names(gfp_mask); > + pr_warn("%s: page allocation failure: order:%u, mode:%pgg\n", > + current->comm, order, &gfp_mask); > dump_stack(); > if (!should_suppress_show_mem()) > show_mem(filter); > diff --git a/mm/page_owner.c b/mm/page_owner.c > index f4acd2452c35..ff862b6d12da 100644 > --- a/mm/page_owner.c > +++ b/mm/page_owner.c > @@ -208,9 +208,8 @@ void __dump_page_owner(struct page *page) > return; > } > > - pr_alert("page allocated via order %u, migratetype %s, gfp_mask 0x%x", > - page_ext->order, migratetype_names[mt], gfp_mask); > - dump_gfpflag_names(gfp_mask); > + pr_alert("page allocated via order %u, migratetype %s, gfp_mask %pgg\n", > + page_ext->order, migratetype_names[mt], &gfp_mask); > print_stack_trace(&trace, 0); > > if (page_ext->last_migrate_reason != -1) > -- > 2.6.3 > i am thinking why not make %pg* to be more generic ? not restricted to only GFP / vma flags / page flags . so could we change format like this ? define a flag spec struct to include flag and trace_print_flags and some other option : typedef struct { unsigned long flag; struct trace_print_flags *flags; unsigned long option; } flag_sec; flag_sec my_flag; in printk we only pass like this : printk(“%pg\n”, &my_flag) ; then it can print any flags defined by user . more useful for other drivers to use . Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/