Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1276394 > unrolled thread

[PATCH v2 0/9] page_owner improvements for debugging

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/9] page_owner improvements for debugging Vlastimil Babka <vbabka@suse.cz> - 2015-11-24 13:40 +0100
    [PATCH v2 4/9] mm, page_owner: copy page owner info during migration Vlastimil Babka <vbabka@suse.cz> - 2015-11-24 13:40 +0100
    [PATCH v2 3/9] mm, page_owner: convert page_owner_inited to static key Vlastimil Babka <vbabka@suse.cz> - 2015-11-24 13:40 +0100
      Re: [PATCH v2 3/9] mm, page_owner: convert page_owner_inited to  static key Michal Hocko <mhocko@kernel.org> - 2015-11-25 16:00 +0100
        Re: [PATCH v2 3/9] mm, page_owner: convert page_owner_inited to  static key Vlastimil Babka <vbabka@suse.cz> - 2015-11-25 16:10 +0100
          Re: [PATCH v2 3/9] mm, page_owner: convert page_owner_inited to  static key Peter Zijlstra <peterz@infradead.org> - 2015-11-25 16:30 +0100
            Re: [PATCH v2 3/9] mm, page_owner: convert page_owner_inited to  static key Michal Hocko <mhocko@kernel.org> - 2015-11-25 16:50 +0100
    [PATCH v2 2/9] mm, page_owner: print symbolic migratetype of both page and pageblock Vlastimil Babka <vbabka@suse.cz> - 2015-11-24 13:40 +0100
      Re: [PATCH v2 2/9] mm, page_owner: print symbolic migratetype of  both page and pageblock Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2015-11-25 09:20 +0100
    [PATCH v2 8/9] mm, page_alloc: print symbolic gfp_flags on allocation failure Vlastimil Babka <vbabka@suse.cz> - 2015-11-24 13:50 +0100
      Re: [PATCH v2 8/9] mm, page_alloc: print symbolic gfp_flags on  allocation failure Michal Hocko <mhocko@kernel.org> - 2015-11-25 15:40 +0100
    [PATCH v2 6/9] mm, debug: introduce dump_gfpflag_names() for symbolic printing of gfp_flags Vlastimil Babka <vbabka@suse.cz> - 2015-11-24 13:50 +0100
      Re: [PATCH v2 6/9] mm, debug: introduce dump_gfpflag_names() for  symbolic printing of gfp_flags Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2015-11-25 09:20 +0100
        Re: [PATCH v2 6/9] mm, debug: introduce dump_gfpflag_names() for  symbolic printing of gfp_flags Vlastimil Babka <vbabka@suse.cz> - 2015-11-25 11:30 +0100
          Re: [PATCH v2 6/9] mm, debug: introduce dump_gfpflag_names() for symbolic printing of gfp_flags yalin wang <yalin.wang2010@gmail.com> - 2015-11-27 04:50 +0100
    Re: [PATCH v2 0/9] page_owner improvements for debugging Michal Hocko <mhocko@kernel.org> - 2015-11-25 15:40 +0100
      [PATCH 1/2] mm, printk: introduce new format string for flags Vlastimil Babka <vbabka@suse.cz> - 2015-11-30 17:20 +0100
        [PATCH 2/2] mm, page_owner: provide symbolic page flags and gfp_flags Vlastimil Babka <vbabka@suse.cz> - 2015-11-30 17:20 +0100
        Re: [PATCH 1/2] mm, printk: introduce new format string for flags Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-12-02 12:10 +0100
          Re: [PATCH 1/2] mm, printk: introduce new format string for flags Vlastimil Babka <vbabka@suse.cz> - 2015-12-02 21:40 +0100
            Re: [PATCH 1/2] mm, printk: introduce new format string for flags Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-12-03 13:40 +0100
              Re: [PATCH 1/2] mm, printk: introduce new format string for flags Vlastimil Babka <vbabka@suse.cz> - 2015-12-03 14:50 +0100
          [PATCH v2 3/3] mm, debug: move bad flags printing to bad_page() Vlastimil Babka <vbabka@suse.cz> - 2015-12-04 16:20 +0100
          [PATCH v2 2/3] mm, page_owner: provide symbolic page flags and gfp_flags Vlastimil Babka <vbabka@suse.cz> - 2015-12-04 16:20 +0100
          [PATCH v2 1/3] mm, printk: introduce new format string for flags Vlastimil Babka <vbabka@suse.cz> - 2015-12-04 16:20 +0100
            Re: [PATCH v2 1/3] mm, printk: introduce new format string for flags Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-12-05 21:10 +0100
            Re: [PATCH v2 1/3] mm, printk: introduce new format string for flags Arnd Bergmann <arnd@arndb.de> - 2015-12-09 12:40 +0100
              Re: [PATCH v2 1/3] mm, printk: introduce new format string for flags Vlastimil Babka <vbabka@suse.cz> - 2015-12-09 21:50 +0100
              Re: [PATCH v2 1/3] mm, printk: introduce new format string for flags James Hogan <james.hogan@imgtec.com> - 2015-12-10 13:30 +0100
            Re: [PATCH v2 1/3] mm, printk: introduce new format string for flags Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2015-12-10 04:00 +0100
              Re: [PATCH v2 1/3] mm, printk: introduce new format string for flags Steven Rostedt <rostedt@goodmis.org> - 2015-12-10 05:10 +0100
                Re: [PATCH v2 1/3] mm, printk: introduce new format string for flags Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2015-12-10 05:50 +0100
                Re: [PATCH v2 1/3] mm, printk: introduce new format string for flags Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-12-10 09:50 +0100
                Re: [PATCH v2 1/3] mm, printk: introduce new format string for flags Vlastimil Babka <vbabka@suse.cz> - 2015-12-10 11:10 +0100
            Re: [PATCH v2 1/3] mm, printk: introduce new format string for flags Steven Rostedt <rostedt@goodmis.org> - 2015-12-10 05:00 +0100
              Re: [PATCH v2 1/3] mm, printk: introduce new format string for flags Vlastimil Babka <vbabka@suse.cz> - 2015-12-10 11:00 +0100
        Re: [PATCH 1/2] mm, printk: introduce new format string for flags yalin wang <yalin.wang2010@gmail.com> - 2015-12-02 18:50 +0100
          Re: [PATCH 1/2] mm, printk: introduce new format string for flags Vlastimil Babka <vbabka@suse.cz> - 2015-12-02 22:20 +0100
            Re: [PATCH 1/2] mm, printk: introduce new format string for flags yalin wang <yalin.wang2010@gmail.com> - 2015-12-03 01:20 +0100
              Re: [PATCH 1/2] mm, printk: introduce new format string for flags Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-12-03 09:10 +0100
                Re: [PATCH 1/2] mm, printk: introduce new format string for flags yalin wang <yalin.wang2010@gmail.com> - 2015-12-03 19:40 +0100
                  Re: [PATCH 1/2] mm, printk: introduce new format string for flags yalin wang <yalin.wang2010@gmail.com> - 2015-12-04 02:10 +0100
                  Re: [PATCH 1/2] mm, printk: introduce new format string for flags Vlastimil Babka <vbabka@suse.cz> - 2015-12-04 15:20 +0100
                    Re: [PATCH 1/2] mm, printk: introduce new format string for flags Steven Rostedt <rostedt@goodmis.org> - 2015-12-10 05:10 +0100

Page 1 of 3  [1] 2 3  Next page →

#1276394 — [PATCH v2 0/9] page_owner improvements for debugging

This is the second version of patchset which originally aimed to improve the
page_owner functionality. Thanks to feedback from v1 and some bugs I
discovered along the way, it is now larger in scope and number of patches.
It's based on next-20151124.

For page_owner, the main changes are
o Use static key to further reduce overhead when compiled in but not enabled.
o Improve output wrt. page and pageblock migratetypes
o Transfer the info on page migrations and track last migration reason.
o Dump the info as part of dump_page() to hopefully help debugging.

For the last point, Kirill requested a human readable printing of gfp_mask and
migratetype after v1. At that point it probably makes a lot of sense to do the
same for page alloc failure and OOM warnings. The flags have been undergoing
revisions recently, and we might be getting reports from various kernel
versions that differ. The ./scripts/gfp-translate tool needs to be pointed at
the corresponding sources to be accurate.  The downside is potentially breaking
scripts that grep these warnings, but it's not a first change done there over
the years.

Note I'm not entirely happy about the dump_gfpflag_names() implementation, due
to usage of pr_cont() unreliable on SMP (and I've seen spurious newlines in
dmesg output, while being correct on serial console or /var/log/messages).
It also doesn't allow plugging the gfp_mask translation into
/sys/kernel/debug/page_owner where it also could make sense. Maybe a new
*printf formatting flag? Too specialized maybe? Or just prepare the string in
a buffer on stack with strscpy?

Other changes since v1:
o Change placement of page owner migration calls to cover missing cases (Hugh)
o Move dump_page_owner() call up from dump_page_badflags(), so the latter can
  be used for adding debugging prints without page owner info (Kirill)

Vlastimil Babka (9):
  mm, debug: fix wrongly filtered flags in dump_vma()
  mm, page_owner: print symbolic migratetype of both page and pageblock
  mm, page_owner: convert page_owner_inited to static key
  mm, page_owner: copy page owner info during migration
  mm, page_owner: track and print last migrate reason
  mm, debug: introduce dump_gfpflag_names() for symbolic printing of
    gfp_flags
  mm, page_owner: dump page owner info from dump_page()
  mm, page_alloc: print symbolic gfp_flags on allocation failure
  mm, oom: print symbolic gfp_flags in oom warning

 Documentation/vm/page_owner.txt |  9 +++--
 include/linux/migrate.h         |  6 ++-
 include/linux/mmdebug.h         |  1 +
 include/linux/mmzone.h          |  3 ++
 include/linux/page_ext.h        |  1 +
 include/linux/page_owner.h      | 50 ++++++++++++++++++-------
 include/trace/events/gfpflags.h | 14 +++++--
 mm/debug.c                      | 44 ++++++++++++++++------
 mm/migrate.c                    | 23 ++++++++++--
 mm/oom_kill.c                   | 10 +++--
 mm/page_alloc.c                 | 18 ++++++++-
 mm/page_owner.c                 | 82 +++++++++++++++++++++++++++++++++++++----
 mm/vmstat.c                     | 15 +-------
 13 files changed, 213 insertions(+), 63 deletions(-)

-- 
2.6.3

--
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/

[toc] | [next] | [standalone]

#1276397 — [PATCH v2 4/9] mm, page_owner: copy page owner info during migration

The page_owner mechanism stores gfp_flags of an allocation and stack trace
that lead to it. During page migration, the original information is
practically replaced by the allocation of free page as the migration target.
Arguably this is less useful and might lead to all the page_owner info for
migratable pages gradually converge towards compaction or numa balancing
migrations. It has also lead to inaccuracies such as one fixed by commit
e2cfc91120fa ("mm/page_owner: set correct gfp_mask on page_owner").

This patch thus introduces copying the page_owner info during migration.
However, since the fact that the page has been migrated from its original
place might be useful for debugging, the next patch will introduce a way to
track that information as well.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 include/linux/page_owner.h | 10 +++++++++-
 mm/migrate.c               |  3 +++
 mm/page_owner.c            | 25 +++++++++++++++++++++++++
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h
index 8e2eb15..6440daa 100644
--- a/include/linux/page_owner.h
+++ b/include/linux/page_owner.h
@@ -11,6 +11,7 @@ extern void __reset_page_owner(struct page *page, unsigned int order);
 extern void __set_page_owner(struct page *page,
 			unsigned int order, gfp_t gfp_mask);
 extern gfp_t __get_page_owner_gfp(struct page *page);
+extern void __copy_page_owner(struct page *oldpage, struct page *newpage);
 
 static inline void reset_page_owner(struct page *page, unsigned int order)
 {
@@ -32,6 +33,11 @@ static inline gfp_t get_page_owner_gfp(struct page *page)
 	else
 		return 0;
 }
+static inline void copy_page_owner(struct page *oldpage, struct page *newpage)
+{
+	if (static_branch_unlikely(&page_owner_inited))
+		__copy_page_owner(oldpage, newpage);
+}
 #else
 static inline void reset_page_owner(struct page *page, unsigned int order)
 {
@@ -44,6 +50,8 @@ static inline gfp_t get_page_owner_gfp(struct page *page)
 {
 	return 0;
 }
-
+static inline void copy_page_owner(struct page *oldpage, struct page *newpage)
+{
+}
 #endif /* CONFIG_PAGE_OWNER */
 #endif /* __LINUX_PAGE_OWNER_H */
diff --git a/mm/migrate.c b/mm/migrate.c
index b1034f9..863a0f1 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -38,6 +38,7 @@
 #include <linux/balloon_compaction.h>
 #include <linux/mmu_notifier.h>
 #include <linux/page_idle.h>
+#include <linux/page_owner.h>
 
 #include <asm/tlbflush.h>
 
@@ -578,6 +579,8 @@ void migrate_page_copy(struct page *newpage, struct page *page)
 	 */
 	if (PageWriteback(newpage))
 		end_page_writeback(newpage);
+
+	copy_page_owner(page, newpage);
 }
 
 /************************************************************
diff --git a/mm/page_owner.c b/mm/page_owner.c
index 10a6a46..f571e55 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -84,6 +84,31 @@ gfp_t __get_page_owner_gfp(struct page *page)
 	return page_ext->gfp_mask;
 }
 
+void __copy_page_owner(struct page *oldpage, struct page *newpage)
+{
+	struct page_ext *old_ext = lookup_page_ext(oldpage);
+	struct page_ext *new_ext = lookup_page_ext(newpage);
+	int i;
+
+	new_ext->order = old_ext->order;
+	new_ext->gfp_mask = old_ext->gfp_mask;
+	new_ext->nr_entries = old_ext->nr_entries;
+
+	for (i = 0; i < ARRAY_SIZE(new_ext->trace_entries); i++)
+		new_ext->trace_entries[i] = old_ext->trace_entries[i];
+
+	/*
+	 * We don't clear the bit on the oldpage as it's going to be freed
+	 * after migration. Until then, the info can be useful in case of
+	 * a bug, and the overal stats will be off a bit only temporarily.
+	 * Also, migrate_misplaced_transhuge_page() can still fail the
+	 * migration and then we want the oldpage to retain the info. But
+	 * in that case we also don't need to explicitly clear the info from
+	 * the new page, which will be freed.
+	 */
+	__set_bit(PAGE_EXT_OWNER, &new_ext->flags);
+}
+
 static ssize_t
 print_page_owner(char __user *buf, size_t count, unsigned long pfn,
 		struct page *page, struct page_ext *page_ext)
-- 
2.6.3

--
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/

[toc] | [prev] | [next] | [standalone]

#1276399 — [PATCH v2 3/9] mm, page_owner: convert page_owner_inited to static key

CONFIG_PAGE_OWNER attempts to impose negligible runtime overhead when enabled
during compilation, but not actually enabled during runtime by boot param
page_owner=on. This overhead can be further reduced using the static key
mechanism, which this patch does.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 Documentation/vm/page_owner.txt |  9 +++++----
 include/linux/page_owner.h      | 22 ++++++++++------------
 mm/page_owner.c                 |  9 +++++----
 mm/vmstat.c                     |  2 +-
 4 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/Documentation/vm/page_owner.txt b/Documentation/vm/page_owner.txt
index 8f3ce9b..ffff143 100644
--- a/Documentation/vm/page_owner.txt
+++ b/Documentation/vm/page_owner.txt
@@ -28,10 +28,11 @@ with page owner and page owner is disabled in runtime due to no enabling
 boot option, runtime overhead is marginal. If disabled in runtime, it
 doesn't require memory to store owner information, so there is no runtime
 memory overhead. And, page owner inserts just two unlikely branches into
-the page allocator hotpath and if it returns false then allocation is
-done like as the kernel without page owner. These two unlikely branches
-would not affect to allocation performance. Following is the kernel's
-code size change due to this facility.
+the page allocator hotpath and if not enabled, then allocation is done
+like as the kernel without page owner. These two unlikely branches should
+not affect to allocation performance, especially if the static keys jump
+label patching functionality is available. Following is the kernel's code
+size change due to this facility.
 
 - Without page owner
    text    data     bss     dec     hex filename
diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h
index cacaabe..8e2eb15 100644
--- a/include/linux/page_owner.h
+++ b/include/linux/page_owner.h
@@ -1,8 +1,10 @@
 #ifndef __LINUX_PAGE_OWNER_H
 #define __LINUX_PAGE_OWNER_H
 
+#include <linux/jump_label.h>
+
 #ifdef CONFIG_PAGE_OWNER
-extern bool page_owner_inited;
+extern struct static_key_false page_owner_inited;
 extern struct page_ext_operations page_owner_ops;
 
 extern void __reset_page_owner(struct page *page, unsigned int order);
@@ -12,27 +14,23 @@ extern gfp_t __get_page_owner_gfp(struct page *page);
 
 static inline void reset_page_owner(struct page *page, unsigned int order)
 {
-	if (likely(!page_owner_inited))
-		return;
-
-	__reset_page_owner(page, order);
+	if (static_branch_unlikely(&page_owner_inited))
+		__reset_page_owner(page, order);
 }
 
 static inline void set_page_owner(struct page *page,
 			unsigned int order, gfp_t gfp_mask)
 {
-	if (likely(!page_owner_inited))
-		return;
-
-	__set_page_owner(page, order, gfp_mask);
+	if (static_branch_unlikely(&page_owner_inited))
+		__set_page_owner(page, order, gfp_mask);
 }
 
 static inline gfp_t get_page_owner_gfp(struct page *page)
 {
-	if (likely(!page_owner_inited))
+	if (static_branch_unlikely(&page_owner_inited))
+		return __get_page_owner_gfp(page);
+	else
 		return 0;
-
-	return __get_page_owner_gfp(page);
 }
 #else
 static inline void reset_page_owner(struct page *page, unsigned int order)
diff --git a/mm/page_owner.c b/mm/page_owner.c
index f35826e..10a6a46 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -5,10 +5,11 @@
 #include <linux/bootmem.h>
 #include <linux/stacktrace.h>
 #include <linux/page_owner.h>
+#include <linux/jump_label.h>
 #include "internal.h"
 
 static bool page_owner_disabled = true;
-bool page_owner_inited __read_mostly;
+DEFINE_STATIC_KEY_FALSE(page_owner_inited);
 
 static void init_early_allocated_pages(void);
 
@@ -37,7 +38,7 @@ static void init_page_owner(void)
 	if (page_owner_disabled)
 		return;
 
-	page_owner_inited = true;
+	static_branch_enable(&page_owner_inited);
 	init_early_allocated_pages();
 }
 
@@ -157,7 +158,7 @@ read_page_owner(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	struct page *page;
 	struct page_ext *page_ext;
 
-	if (!page_owner_inited)
+	if (!static_branch_unlikely(&page_owner_inited))
 		return -EINVAL;
 
 	page = NULL;
@@ -305,7 +306,7 @@ static int __init pageowner_init(void)
 {
 	struct dentry *dentry;
 
-	if (!page_owner_inited) {
+	if (!static_branch_unlikely(&page_owner_inited)) {
 		pr_info("page_owner is disabled\n");
 		return 0;
 	}
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 53b722b..3f7ec14 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1117,7 +1117,7 @@ static void pagetypeinfo_showmixedcount(struct seq_file *m, pg_data_t *pgdat)
 #ifdef CONFIG_PAGE_OWNER
 	int mtype;
 
-	if (!page_owner_inited)
+	if (!static_branch_unlikely(&page_owner_inited))
 		return;
 
 	drain_all_pages(NULL);
-- 
2.6.3

--
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/

[toc] | [prev] | [next] | [standalone]

#1277440 — Re: [PATCH v2 3/9] mm, page_owner: convert page_owner_inited to static key

On Tue 24-11-15 13:36:15, Vlastimil Babka wrote:
> CONFIG_PAGE_OWNER attempts to impose negligible runtime overhead when enabled
> during compilation, but not actually enabled during runtime by boot param
> page_owner=on. This overhead can be further reduced using the static key
> mechanism, which this patch does.

Is this really worth doing? If we do not have jump labels then the check
will be atomic rather than a simple access, so it would be more costly,
no? Or am I missing something?
-- 
Michal Hocko
SUSE Labs
--
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/

[toc] | [prev] | [next] | [standalone]

#1277451 — Re: [PATCH v2 3/9] mm, page_owner: convert page_owner_inited to static key

[+CC PeterZ]

On 11/25/2015 03:52 PM, Michal Hocko wrote:
> On Tue 24-11-15 13:36:15, Vlastimil Babka wrote:
>> CONFIG_PAGE_OWNER attempts to impose negligible runtime overhead when enabled
>> during compilation, but not actually enabled during runtime by boot param
>> page_owner=on. This overhead can be further reduced using the static key
>> mechanism, which this patch does.
> 
> Is this really worth doing?

Well, I assume that jump labels exist for a reason, and allocation hot paths are
sufficiently sensitive to be worth it? It's not an extra maintenance burden for
us anyway. Just a bit different content of the if () line.

> If we do not have jump labels then the check
> will be atomic rather than a simple access, so it would be more costly,
> no? Or am I missing something?

Well, atomic read is a simple READ_ONCE on x86_64. That excludes some compiler
optimizations, but it's not expensive for the CPU. The optimization would be
caching the value of the flag to a register, which would only potentially affect
multiple checks from the same function (and its inlines). Which doesn't happen
AFAIK, as it's just once in the allocation and once in the free path?

Now I admit I have no idea if there are architectures that don't support jump
labels *and* have an expensive atomic read, and whether we care?
--
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/

[toc] | [prev] | [next] | [standalone]

#1277479 — Re: [PATCH v2 3/9] mm, page_owner: convert page_owner_inited to static key

On Wed, Nov 25, 2015 at 04:08:11PM +0100, Vlastimil Babka wrote:
> Now I admit I have no idea if there are architectures that don't support jump
> labels *and* have an expensive atomic read, and whether we care?

atomic_read() is basically always READ_ONCE(), there's a few archs that
implement it in asm with a 'weird' load instruction, but its still a
load. The worst is I think an uncached load for blackfin or somesuch.

There's plenty archs that do not support the jump label bits, but
typically you don't care much about those. I'm not aware of an arch that
cannot fundamentally implement jump_label support if they wanted to.
--
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/

[toc] | [prev] | [next] | [standalone]

#1277511 — Re: [PATCH v2 3/9] mm, page_owner: convert page_owner_inited to static key

On Wed 25-11-15 16:25:33, Peter Zijlstra wrote:
> On Wed, Nov 25, 2015 at 04:08:11PM +0100, Vlastimil Babka wrote:
> > Now I admit I have no idea if there are architectures that don't support jump
> > labels *and* have an expensive atomic read, and whether we care?
> 
> atomic_read() is basically always READ_ONCE(), there's a few archs that
> implement it in asm with a 'weird' load instruction, but its still a
> load. The worst is I think an uncached load for blackfin or somesuch.
> 
> There's plenty archs that do not support the jump label bits, but
> typically you don't care much about those. I'm not aware of an arch that
> cannot fundamentally implement jump_label support if they wanted to.

OK, I see. Thanks for the clarification! Then I do not have any
objections.

-- 
Michal Hocko
SUSE Labs
--
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/

[toc] | [prev] | [next] | [standalone]

#1276401 — [PATCH v2 2/9] mm, page_owner: print symbolic migratetype of both page and pageblock

The information in /sys/kernel/debug/page_owner includes the migratetype of
the pageblock the page belongs to. This is also checked against the page's
migratetype (as declared by gfp_flags during its allocation), and the page is
reported as Fallback if its migratetype differs from the pageblock's one.

This is somewhat misleading because in fact fallback allocation is not the only
reason why these two can differ. It also doesn't direcly provide the page's
migratetype, although it's possible to derive that from the gfp_flags.

It's arguably better to print both page and pageblock's migratetype and leave
the interpretation to the consumer than to suggest fallback allocation as the
only possible reason. While at it, we can print the migratetypes as string
the same way as /proc/pagetypeinfo does, as some of the numeric values depend
on kernel configuration. For that, this patch moves the migratetype_names
array from #ifdef CONFIG_PROC_FS part of mm/vmstat.c to mm/page_alloc.c and
exports it.

Example page_owner entry after the patch:

Page allocated via order 0, mask 0x2420848
PFN 512 type Reclaimable Block 1 type Reclaimable Flags   R  LA
 [<ffffffff81164e8a>] __alloc_pages_nodemask+0x15a/0xa30
 [<ffffffff811ab808>] alloc_pages_current+0x88/0x120
 [<ffffffff8115bc36>] __page_cache_alloc+0xe6/0x120
 [<ffffffff8115c226>] pagecache_get_page+0x56/0x200
 [<ffffffff81205892>] __getblk_slow+0xd2/0x2b0
 [<ffffffff81205ab0>] __getblk_gfp+0x40/0x50
 [<ffffffff81206ad7>] __breadahead+0x17/0x50
 [<ffffffffa0437b27>] __ext4_get_inode_loc+0x397/0x3e0 [ext4]

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 include/linux/mmzone.h |  3 +++
 mm/page_alloc.c        | 13 +++++++++++++
 mm/page_owner.c        |  6 +++---
 mm/vmstat.c            | 13 -------------
 4 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 3b6fb71..2bfad18 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -63,6 +63,9 @@ enum {
 	MIGRATE_TYPES
 };
 
+/* In mm/page_alloc.c; keep in sync also with show_migration_types() there */
+extern char * const migratetype_names[MIGRATE_TYPES];
+
 #ifdef CONFIG_CMA
 #  define is_migrate_cma(migratetype) unlikely((migratetype) == MIGRATE_CMA)
 #else
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 35ab351..61a023a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -229,6 +229,19 @@ static char * const zone_names[MAX_NR_ZONES] = {
 #endif
 };
 
+char * const migratetype_names[MIGRATE_TYPES] = {
+	"Unmovable",
+	"Movable",
+	"Reclaimable",
+	"HighAtomic",
+#ifdef CONFIG_CMA
+	"CMA",
+#endif
+#ifdef CONFIG_MEMORY_ISOLATION
+	"Isolate",
+#endif
+};
+
 compound_page_dtor * const compound_page_dtors[] = {
 	NULL,
 	free_compound_page,
diff --git a/mm/page_owner.c b/mm/page_owner.c
index 983c3a1..f35826e 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -110,11 +110,11 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
 	pageblock_mt = get_pfnblock_migratetype(page, pfn);
 	page_mt  = gfpflags_to_migratetype(page_ext->gfp_mask);
 	ret += snprintf(kbuf + ret, count - ret,
-			"PFN %lu Block %lu type %d %s Flags %s%s%s%s%s%s%s%s%s%s%s%s\n",
+			"PFN %lu type %s Block %lu type %s Flags %s%s%s%s%s%s%s%s%s%s%s%s\n",
 			pfn,
+			migratetype_names[page_mt],
 			pfn >> pageblock_order,
-			pageblock_mt,
-			pageblock_mt != page_mt ? "Fallback" : "        ",
+			migratetype_names[pageblock_mt],
 			PageLocked(page)	? "K" : " ",
 			PageError(page)		? "E" : " ",
 			PageReferenced(page)	? "R" : " ",
diff --git a/mm/vmstat.c b/mm/vmstat.c
index f7ebad2..53b722b 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -921,19 +921,6 @@ static void walk_zones_in_node(struct seq_file *m, pg_data_t *pgdat,
 #endif
 
 #ifdef CONFIG_PROC_FS
-static char * const migratetype_names[MIGRATE_TYPES] = {
-	"Unmovable",
-	"Movable",
-	"Reclaimable",
-	"HighAtomic",
-#ifdef CONFIG_CMA
-	"CMA",
-#endif
-#ifdef CONFIG_MEMORY_ISOLATION
-	"Isolate",
-#endif
-};
-
 static void frag_show_print(struct seq_file *m, pg_data_t *pgdat,
 						struct zone *zone)
 {
-- 
2.6.3

--
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/

[toc] | [prev] | [next] | [standalone]

#1277092 — Re: [PATCH v2 2/9] mm, page_owner: print symbolic migratetype of both page and pageblock

On Tue, Nov 24, 2015 at 01:36:14PM +0100, Vlastimil Babka wrote:
> The information in /sys/kernel/debug/page_owner includes the migratetype of
> the pageblock the page belongs to. This is also checked against the page's
> migratetype (as declared by gfp_flags during its allocation), and the page is
> reported as Fallback if its migratetype differs from the pageblock's one.
> 
> This is somewhat misleading because in fact fallback allocation is not the only
> reason why these two can differ. It also doesn't direcly provide the page's
> migratetype, although it's possible to derive that from the gfp_flags.
> 
> It's arguably better to print both page and pageblock's migratetype and leave
> the interpretation to the consumer than to suggest fallback allocation as the
> only possible reason. While at it, we can print the migratetypes as string
> the same way as /proc/pagetypeinfo does, as some of the numeric values depend
> on kernel configuration. For that, this patch moves the migratetype_names
> array from #ifdef CONFIG_PROC_FS part of mm/vmstat.c to mm/page_alloc.c and
> exports it.
> 
> Example page_owner entry after the patch:
> 
> Page allocated via order 0, mask 0x2420848
> PFN 512 type Reclaimable Block 1 type Reclaimable Flags   R  LA
>  [<ffffffff81164e8a>] __alloc_pages_nodemask+0x15a/0xa30
>  [<ffffffff811ab808>] alloc_pages_current+0x88/0x120
>  [<ffffffff8115bc36>] __page_cache_alloc+0xe6/0x120
>  [<ffffffff8115c226>] pagecache_get_page+0x56/0x200
>  [<ffffffff81205892>] __getblk_slow+0xd2/0x2b0
>  [<ffffffff81205ab0>] __getblk_gfp+0x40/0x50
>  [<ffffffff81206ad7>] __breadahead+0x17/0x50
>  [<ffffffffa0437b27>] __ext4_get_inode_loc+0x397/0x3e0 [ext4]
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  include/linux/mmzone.h |  3 +++
>  mm/page_alloc.c        | 13 +++++++++++++
>  mm/page_owner.c        |  6 +++---
>  mm/vmstat.c            | 13 -------------
>  4 files changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 3b6fb71..2bfad18 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -63,6 +63,9 @@ enum {
>  	MIGRATE_TYPES
>  };
>  
> +/* In mm/page_alloc.c; keep in sync also with show_migration_types() there */
> +extern char * const migratetype_names[MIGRATE_TYPES];
> +
>  #ifdef CONFIG_CMA
>  #  define is_migrate_cma(migratetype) unlikely((migratetype) == MIGRATE_CMA)
>  #else
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 35ab351..61a023a 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -229,6 +229,19 @@ static char * const zone_names[MAX_NR_ZONES] = {
>  #endif
>  };
>  
> +char * const migratetype_names[MIGRATE_TYPES] = {
> +	"Unmovable",
> +	"Movable",
> +	"Reclaimable",
> +	"HighAtomic",
> +#ifdef CONFIG_CMA
> +	"CMA",
> +#endif
> +#ifdef CONFIG_MEMORY_ISOLATION
> +	"Isolate",
> +#endif
> +};
> +
>  compound_page_dtor * const compound_page_dtors[] = {
>  	NULL,
>  	free_compound_page,
> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index 983c3a1..f35826e 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
> @@ -110,11 +110,11 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
>  	pageblock_mt = get_pfnblock_migratetype(page, pfn);
>  	page_mt  = gfpflags_to_migratetype(page_ext->gfp_mask);
>  	ret += snprintf(kbuf + ret, count - ret,
> -			"PFN %lu Block %lu type %d %s Flags %s%s%s%s%s%s%s%s%s%s%s%s\n",
> +			"PFN %lu type %s Block %lu type %s Flags %s%s%s%s%s%s%s%s%s%s%s%s\n",

How about generalizing dump_flag_names() more and using it here?
This output is neat than dump_flag_names() but not complete.

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/

[toc] | [prev] | [next] | [standalone]

#1276406 — [PATCH v2 8/9] mm, page_alloc: print symbolic gfp_flags on allocation failure

It would be useful to translate gfp_flags into string representation when
printing in case of an allocation failure, especially as the flags have been
undergoing some changes recently and the script ./scripts/gfp-translate needs
a matching source version to be accurate.

Example output:

stapio: page allocation failure: order:9, mode:0x2080020(GFP_ATOMIC)

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/page_alloc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index f806a1a..80349ac 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2711,9 +2711,9 @@ 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\n",
+	pr_warn("%s: page allocation failure: order:%u, mode:0x%x",
 		current->comm, order, gfp_mask);
-
+	dump_gfpflag_names(gfp_mask);
 	dump_stack();
 	if (!should_suppress_show_mem())
 		show_mem(filter);
-- 
2.6.3

--
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/

[toc] | [prev] | [next] | [standalone]

#1277434 — Re: [PATCH v2 8/9] mm, page_alloc: print symbolic gfp_flags on allocation failure

On Tue 24-11-15 13:36:20, Vlastimil Babka wrote:
> It would be useful to translate gfp_flags into string representation when
> printing in case of an allocation failure, especially as the flags have been
> undergoing some changes recently and the script ./scripts/gfp-translate needs
> a matching source version to be accurate.
> 
> Example output:
> 
> stapio: page allocation failure: order:9, mode:0x2080020(GFP_ATOMIC)

I like this _very much_

> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

If this can be done with a printk formatter it would be even nicer but
this is good enough for the OOM purpose.

Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/page_alloc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index f806a1a..80349ac 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2711,9 +2711,9 @@ 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\n",
> +	pr_warn("%s: page allocation failure: order:%u, mode:0x%x",
>  		current->comm, order, gfp_mask);
> -
> +	dump_gfpflag_names(gfp_mask);
>  	dump_stack();
>  	if (!should_suppress_show_mem())
>  		show_mem(filter);
> -- 
> 2.6.3

-- 
Michal Hocko
SUSE Labs
--
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/

[toc] | [prev] | [next] | [standalone]

#1276407 — [PATCH v2 6/9] mm, debug: introduce dump_gfpflag_names() for symbolic printing of gfp_flags

It would be useful to convert gfp_flags into string representation when
printing them in case of allocation failure, OOM etc. There's a script
./scripts/gfp-translate to make this simpler, but it needs the matching version
of the sources to be accurate, and the flags have been undergoing some changes
recently.

The ftrace framework already has this translation in the form of
show_gfp_flags() defined in include/trace/events/gfpflags.h which defines the
translation table internally. Allow reusing the table outside ftrace by putting
it behind __def_gfpflag_names definition and introduce dump_gfpflag_names() to
handle the printing.

While at it, also fill in the names for the flags and flag combinations that
have been missing in the table. GFP_NOWAIT no longer equals to "no flags", so
change the output for no flags to "none".

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 include/linux/mmdebug.h         |  1 +
 include/trace/events/gfpflags.h | 14 +++++++++++---
 mm/debug.c                      | 10 ++++++++++
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index c447d80..3b77fab 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -10,6 +10,7 @@ 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);
 void dump_vma(const struct vm_area_struct *vma);
 void dump_mm(const struct mm_struct *mm);
 
diff --git a/include/trace/events/gfpflags.h b/include/trace/events/gfpflags.h
index dde6bf0..3d580fd 100644
--- a/include/trace/events/gfpflags.h
+++ b/include/trace/events/gfpflags.h
@@ -8,8 +8,8 @@
  *
  * Thus most bits set go first.
  */
-#define show_gfp_flags(flags)						\
-	(flags) ? __print_flags(flags, "|",				\
+
+#define __def_gfpflag_names						\
 	{(unsigned long)GFP_TRANSHUGE,		"GFP_TRANSHUGE"},	\
 	{(unsigned long)GFP_HIGHUSER_MOVABLE,	"GFP_HIGHUSER_MOVABLE"}, \
 	{(unsigned long)GFP_HIGHUSER,		"GFP_HIGHUSER"},	\
@@ -19,9 +19,13 @@
 	{(unsigned long)GFP_NOFS,		"GFP_NOFS"},		\
 	{(unsigned long)GFP_ATOMIC,		"GFP_ATOMIC"},		\
 	{(unsigned long)GFP_NOIO,		"GFP_NOIO"},		\
+	{(unsigned long)GFP_NOWAIT,		"GFP_NOWAIT"},		\
+	{(unsigned long)__GFP_DMA,		"GFP_DMA"},		\
+	{(unsigned long)__GFP_DMA32,		"GFP_DMA32"},		\
 	{(unsigned long)__GFP_HIGH,		"GFP_HIGH"},		\
 	{(unsigned long)__GFP_ATOMIC,		"GFP_ATOMIC"},		\
 	{(unsigned long)__GFP_IO,		"GFP_IO"},		\
+	{(unsigned long)__GFP_FS,		"GFP_FS"},		\
 	{(unsigned long)__GFP_COLD,		"GFP_COLD"},		\
 	{(unsigned long)__GFP_NOWARN,		"GFP_NOWARN"},		\
 	{(unsigned long)__GFP_REPEAT,		"GFP_REPEAT"},		\
@@ -36,8 +40,12 @@
 	{(unsigned long)__GFP_RECLAIMABLE,	"GFP_RECLAIMABLE"},	\
 	{(unsigned long)__GFP_MOVABLE,		"GFP_MOVABLE"},		\
 	{(unsigned long)__GFP_NOTRACK,		"GFP_NOTRACK"},		\
+	{(unsigned long)__GFP_WRITE,		"GFP_WRITE"},		\
 	{(unsigned long)__GFP_DIRECT_RECLAIM,	"GFP_DIRECT_RECLAIM"},	\
 	{(unsigned long)__GFP_KSWAPD_RECLAIM,	"GFP_KSWAPD_RECLAIM"},	\
 	{(unsigned long)__GFP_OTHER_NODE,	"GFP_OTHER_NODE"}	\
-	) : "GFP_NOWAIT"
 
+#define show_gfp_flags(flags)						\
+	(flags) ? __print_flags(flags, "|",				\
+	__def_gfpflag_names						\
+	) : "none"
diff --git a/mm/debug.c b/mm/debug.c
index d9718fc..1a71a3b 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -9,6 +9,7 @@
 #include <linux/mm.h>
 #include <linux/trace_events.h>
 #include <linux/memcontrol.h>
+#include <trace/events/gfpflags.h>
 
 static const struct trace_print_flags pageflag_names[] = {
 	{1UL << PG_locked,		"locked"	},
@@ -46,6 +47,10 @@ static const struct trace_print_flags pageflag_names[] = {
 #endif
 };
 
+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)
 {
@@ -73,6 +78,11 @@ static void dump_flag_names(unsigned long flags,
 	pr_cont(")\n");
 }
 
+void dump_gfpflag_names(unsigned long gfp_flags)
+{
+	dump_flag_names(gfp_flags, gfpflag_names, ARRAY_SIZE(gfpflag_names));
+}
+
 void dump_page_badflags(struct page *page, const char *reason,
 		unsigned long badflags)
 {
-- 
2.6.3

--
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/

[toc] | [prev] | [next] | [standalone]

#1277084 — Re: [PATCH v2 6/9] mm, debug: introduce dump_gfpflag_names() for symbolic printing of gfp_flags

On Tue, Nov 24, 2015 at 01:36:18PM +0100, Vlastimil Babka wrote:
> It would be useful to convert gfp_flags into string representation when
> printing them in case of allocation failure, OOM etc. There's a script
> ./scripts/gfp-translate to make this simpler, but it needs the matching version
> of the sources to be accurate, and the flags have been undergoing some changes
> recently.
> 
> The ftrace framework already has this translation in the form of
> show_gfp_flags() defined in include/trace/events/gfpflags.h which defines the
> translation table internally. Allow reusing the table outside ftrace by putting
> it behind __def_gfpflag_names definition and introduce dump_gfpflag_names() to
> handle the printing.
> 
> While at it, also fill in the names for the flags and flag combinations that
> have been missing in the table. GFP_NOWAIT no longer equals to "no flags", so
> change the output for no flags to "none".
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  include/linux/mmdebug.h         |  1 +
>  include/trace/events/gfpflags.h | 14 +++++++++++---
>  mm/debug.c                      | 10 ++++++++++
>  3 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
> index c447d80..3b77fab 100644
> --- a/include/linux/mmdebug.h
> +++ b/include/linux/mmdebug.h
> @@ -10,6 +10,7 @@ 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);
>  void dump_vma(const struct vm_area_struct *vma);
>  void dump_mm(const struct mm_struct *mm);
>  
> diff --git a/include/trace/events/gfpflags.h b/include/trace/events/gfpflags.h
> index dde6bf0..3d580fd 100644
> --- a/include/trace/events/gfpflags.h
> +++ b/include/trace/events/gfpflags.h
> @@ -8,8 +8,8 @@
>   *
>   * Thus most bits set go first.
>   */
> -#define show_gfp_flags(flags)						\
> -	(flags) ? __print_flags(flags, "|",				\
> +
> +#define __def_gfpflag_names						\
>  	{(unsigned long)GFP_TRANSHUGE,		"GFP_TRANSHUGE"},	\
>  	{(unsigned long)GFP_HIGHUSER_MOVABLE,	"GFP_HIGHUSER_MOVABLE"}, \
>  	{(unsigned long)GFP_HIGHUSER,		"GFP_HIGHUSER"},	\
> @@ -19,9 +19,13 @@
>  	{(unsigned long)GFP_NOFS,		"GFP_NOFS"},		\
>  	{(unsigned long)GFP_ATOMIC,		"GFP_ATOMIC"},		\
>  	{(unsigned long)GFP_NOIO,		"GFP_NOIO"},		\
> +	{(unsigned long)GFP_NOWAIT,		"GFP_NOWAIT"},		\
> +	{(unsigned long)__GFP_DMA,		"GFP_DMA"},		\
> +	{(unsigned long)__GFP_DMA32,		"GFP_DMA32"},		\
>  	{(unsigned long)__GFP_HIGH,		"GFP_HIGH"},		\
>  	{(unsigned long)__GFP_ATOMIC,		"GFP_ATOMIC"},		\
>  	{(unsigned long)__GFP_IO,		"GFP_IO"},		\
> +	{(unsigned long)__GFP_FS,		"GFP_FS"},		\
>  	{(unsigned long)__GFP_COLD,		"GFP_COLD"},		\
>  	{(unsigned long)__GFP_NOWARN,		"GFP_NOWARN"},		\
>  	{(unsigned long)__GFP_REPEAT,		"GFP_REPEAT"},		\
> @@ -36,8 +40,12 @@
>  	{(unsigned long)__GFP_RECLAIMABLE,	"GFP_RECLAIMABLE"},	\
>  	{(unsigned long)__GFP_MOVABLE,		"GFP_MOVABLE"},		\
>  	{(unsigned long)__GFP_NOTRACK,		"GFP_NOTRACK"},		\
> +	{(unsigned long)__GFP_WRITE,		"GFP_WRITE"},		\
>  	{(unsigned long)__GFP_DIRECT_RECLAIM,	"GFP_DIRECT_RECLAIM"},	\
>  	{(unsigned long)__GFP_KSWAPD_RECLAIM,	"GFP_KSWAPD_RECLAIM"},	\
>  	{(unsigned long)__GFP_OTHER_NODE,	"GFP_OTHER_NODE"}	\
> -	) : "GFP_NOWAIT"
>  
> +#define show_gfp_flags(flags)						\
> +	(flags) ? __print_flags(flags, "|",				\
> +	__def_gfpflag_names						\
> +	) : "none"

How about moving this to gfp.h or something?
Now, we use it in out of tracepoints so there is no need to keep it
in include/trace/events/xxx.

Thanks.


> diff --git a/mm/debug.c b/mm/debug.c
> index d9718fc..1a71a3b 100644
> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -9,6 +9,7 @@
>  #include <linux/mm.h>
>  #include <linux/trace_events.h>
>  #include <linux/memcontrol.h>
> +#include <trace/events/gfpflags.h>
>  
>  static const struct trace_print_flags pageflag_names[] = {
>  	{1UL << PG_locked,		"locked"	},
> @@ -46,6 +47,10 @@ static const struct trace_print_flags pageflag_names[] = {
>  #endif
>  };
>  
> +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)
>  {
> @@ -73,6 +78,11 @@ static void dump_flag_names(unsigned long flags,
>  	pr_cont(")\n");
>  }
>  
> +void dump_gfpflag_names(unsigned long gfp_flags)
> +{
> +	dump_flag_names(gfp_flags, gfpflag_names, ARRAY_SIZE(gfpflag_names));
> +}
> +
>  void dump_page_badflags(struct page *page, const char *reason,
>  		unsigned long badflags)
>  {
> -- 
> 2.6.3
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
--
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/

[toc] | [prev] | [next] | [standalone]

#1277164 — Re: [PATCH v2 6/9] mm, debug: introduce dump_gfpflag_names() for symbolic printing of gfp_flags

On 11/25/2015 09:16 AM, Joonsoo Kim wrote:
> On Tue, Nov 24, 2015 at 01:36:18PM +0100, Vlastimil Babka wrote:
>> --- a/include/trace/events/gfpflags.h
>> +++ b/include/trace/events/gfpflags.h
>> @@ -8,8 +8,8 @@
>>   *
>>   * Thus most bits set go first.
>>   */
>> -#define show_gfp_flags(flags)						\
>> -	(flags) ? __print_flags(flags, "|",				\
>> +
>> +#define __def_gfpflag_names						\
>>  	{(unsigned long)GFP_TRANSHUGE,		"GFP_TRANSHUGE"},	\
>>  	{(unsigned long)GFP_HIGHUSER_MOVABLE,	"GFP_HIGHUSER_MOVABLE"}, \
>>  	{(unsigned long)GFP_HIGHUSER,		"GFP_HIGHUSER"},	\
>> @@ -19,9 +19,13 @@
>>  	{(unsigned long)GFP_NOFS,		"GFP_NOFS"},		\
>>  	{(unsigned long)GFP_ATOMIC,		"GFP_ATOMIC"},		\
>>  	{(unsigned long)GFP_NOIO,		"GFP_NOIO"},		\
>> +	{(unsigned long)GFP_NOWAIT,		"GFP_NOWAIT"},		\
>> +	{(unsigned long)__GFP_DMA,		"GFP_DMA"},		\
>> +	{(unsigned long)__GFP_DMA32,		"GFP_DMA32"},		\
>>  	{(unsigned long)__GFP_HIGH,		"GFP_HIGH"},		\
>>  	{(unsigned long)__GFP_ATOMIC,		"GFP_ATOMIC"},		\
>>  	{(unsigned long)__GFP_IO,		"GFP_IO"},		\
>> +	{(unsigned long)__GFP_FS,		"GFP_FS"},		\
>>  	{(unsigned long)__GFP_COLD,		"GFP_COLD"},		\
>>  	{(unsigned long)__GFP_NOWARN,		"GFP_NOWARN"},		\
>>  	{(unsigned long)__GFP_REPEAT,		"GFP_REPEAT"},		\
>> @@ -36,8 +40,12 @@
>>  	{(unsigned long)__GFP_RECLAIMABLE,	"GFP_RECLAIMABLE"},	\
>>  	{(unsigned long)__GFP_MOVABLE,		"GFP_MOVABLE"},		\
>>  	{(unsigned long)__GFP_NOTRACK,		"GFP_NOTRACK"},		\
>> +	{(unsigned long)__GFP_WRITE,		"GFP_WRITE"},		\
>>  	{(unsigned long)__GFP_DIRECT_RECLAIM,	"GFP_DIRECT_RECLAIM"},	\
>>  	{(unsigned long)__GFP_KSWAPD_RECLAIM,	"GFP_KSWAPD_RECLAIM"},	\
>>  	{(unsigned long)__GFP_OTHER_NODE,	"GFP_OTHER_NODE"}	\
>> -	) : "GFP_NOWAIT"
>>  
>> +#define show_gfp_flags(flags)						\
>> +	(flags) ? __print_flags(flags, "|",				\
>> +	__def_gfpflag_names						\
>> +	) : "none"
> 
> How about moving this to gfp.h or something?
> Now, we use it in out of tracepoints so there is no need to keep it
> in include/trace/events/xxx.

Hm I didn't want to pollute such widely included header with such defines. And
show_gfp_flags shouldn't be there definitely as it depends on __print_flags.
What do others think?

--
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/

[toc] | [prev] | [next] | [standalone]

#1278516 — Re: [PATCH v2 6/9] mm, debug: introduce dump_gfpflag_names() for symbolic printing of gfp_flags

> On Nov 25, 2015, at 18:28, Vlastimil Babka <vbabka@suse.cz> wrote:
> 
> On 11/25/2015 09:16 AM, Joonsoo Kim wrote:
>> On Tue, Nov 24, 2015 at 01:36:18PM +0100, Vlastimil Babka wrote:
>>> --- a/include/trace/events/gfpflags.h
>>> +++ b/include/trace/events/gfpflags.h
>>> @@ -8,8 +8,8 @@
>>>  *
>>>  * Thus most bits set go first.
>>>  */
>>> -#define show_gfp_flags(flags)						\
>>> -	(flags) ? __print_flags(flags, "|",				\
>>> +
>>> +#define __def_gfpflag_names						\
>>> 	{(unsigned long)GFP_TRANSHUGE,		"GFP_TRANSHUGE"},	\
>>> 	{(unsigned long)GFP_HIGHUSER_MOVABLE,	"GFP_HIGHUSER_MOVABLE"}, \
>>> 	{(unsigned long)GFP_HIGHUSER,		"GFP_HIGHUSER"},	\
>>> @@ -19,9 +19,13 @@
>>> 	{(unsigned long)GFP_NOFS,		"GFP_NOFS"},		\
>>> 	{(unsigned long)GFP_ATOMIC,		"GFP_ATOMIC"},		\
>>> 	{(unsigned long)GFP_NOIO,		"GFP_NOIO"},		\
>>> +	{(unsigned long)GFP_NOWAIT,		"GFP_NOWAIT"},		\
>>> +	{(unsigned long)__GFP_DMA,		"GFP_DMA"},		\
>>> +	{(unsigned long)__GFP_DMA32,		"GFP_DMA32"},		\
>>> 	{(unsigned long)__GFP_HIGH,		"GFP_HIGH"},		\
>>> 	{(unsigned long)__GFP_ATOMIC,		"GFP_ATOMIC"},		\
>>> 	{(unsigned long)__GFP_IO,		"GFP_IO"},		\
>>> +	{(unsigned long)__GFP_FS,		"GFP_FS"},		\
>>> 	{(unsigned long)__GFP_COLD,		"GFP_COLD"},		\
>>> 	{(unsigned long)__GFP_NOWARN,		"GFP_NOWARN"},		\
>>> 	{(unsigned long)__GFP_REPEAT,		"GFP_REPEAT"},		\
>>> @@ -36,8 +40,12 @@
>>> 	{(unsigned long)__GFP_RECLAIMABLE,	"GFP_RECLAIMABLE"},	\
>>> 	{(unsigned long)__GFP_MOVABLE,		"GFP_MOVABLE"},		\
>>> 	{(unsigned long)__GFP_NOTRACK,		"GFP_NOTRACK"},		\
>>> +	{(unsigned long)__GFP_WRITE,		"GFP_WRITE"},		\
>>> 	{(unsigned long)__GFP_DIRECT_RECLAIM,	"GFP_DIRECT_RECLAIM"},	\
>>> 	{(unsigned long)__GFP_KSWAPD_RECLAIM,	"GFP_KSWAPD_RECLAIM"},	\
>>> 	{(unsigned long)__GFP_OTHER_NODE,	"GFP_OTHER_NODE"}	\
>>> -	) : "GFP_NOWAIT"
>>> 
>>> +#define show_gfp_flags(flags)						\
>>> +	(flags) ? __print_flags(flags, "|",				\
>>> +	__def_gfpflag_names						\
>>> +	) : "none"
>> 
>> How about moving this to gfp.h or something?
>> Now, we use it in out of tracepoints so there is no need to keep it
>> in include/trace/events/xxx.
> 
> Hm I didn't want to pollute such widely included header with such defines. And
> show_gfp_flags shouldn't be there definitely as it depends on __print_flags.
> What do others think?
how about add this into standard printk()  format ?
like cpu mask print in printk use %*pb[l]  ,
it define a macro cpumask_pr_args  to print cpumask .

we can also define a new format like %pG  means print flag ,
then it will be useful for other code to use , like dump vma /  mm  flags ..

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/

[toc] | [prev] | [next] | [standalone]

#1277436

On Tue 24-11-15 13:36:12, Vlastimil Babka wrote:
[...]
> For the last point, Kirill requested a human readable printing of gfp_mask and
> migratetype after v1. At that point it probably makes a lot of sense to do the
> same for page alloc failure and OOM warnings. The flags have been undergoing
> revisions recently, and we might be getting reports from various kernel
> versions that differ. The ./scripts/gfp-translate tool needs to be pointed at
> the corresponding sources to be accurate.  The downside is potentially breaking
> scripts that grep these warnings, but it's not a first change done there over
> the years.

Yes this is very helpful! Thanks for doing this.
 
> Note I'm not entirely happy about the dump_gfpflag_names() implementation, due
> to usage of pr_cont() unreliable on SMP (and I've seen spurious newlines in
> dmesg output, while being correct on serial console or /var/log/messages).
> It also doesn't allow plugging the gfp_mask translation into
> /sys/kernel/debug/page_owner where it also could make sense. Maybe a new
> *printf formatting flag?

I wouldn't object. gfp_mask has its own "type" so having a specific
formatter sounds like a good idea to me.

-- 
Michal Hocko
SUSE Labs
--
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/

[toc] | [prev] | [next] | [standalone]

#1280045 — [PATCH 1/2] mm, printk: introduce new format string for flags

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 <vbabka@suse.cz>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
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 <linux/stringify.h>
+#include <linux/types.h>
 
 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 <linux/dcache.h>
 #include <linux/cred.h>
 #include <net/addrconf.h>
+#include <linux/mmdebug.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
 #include <asm/sections.h>	/* 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

--
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/

[toc] | [prev] | [next] | [standalone]

#1280050 — [PATCH 2/2] mm, page_owner: provide symbolic page flags and gfp_flags

With the new format strings for flags, we can now provide symbolic page and gfp
flags in the /sys/kernel/debug/page_owner file. This replaces the positional
printing of page flags as single letters, which might have looked nicer, but
was limited to a subset of flags, and required the user to remember the
letters.

Example of the adjusted format:

Page allocated via order 0, mask 0x24213ca(GFP_HIGHUSER_MOVABLE|GFP_COLD|GFP_NOWARN|GFP_NORETRY)
PFN 674308 type Movable Block 1317 type Movable Flags 0x1fffff80010068(uptodate|lru|active|mappedtodisk)
 [<ffffffff81164e9a>] __alloc_pages_nodemask+0x15a/0xa30
 [<ffffffff811ab938>] alloc_pages_current+0x88/0x120
 [<ffffffff8115bc46>] __page_cache_alloc+0xe6/0x120
 [<ffffffff81168b9b>] __do_page_cache_readahead+0xdb/0x200
 [<ffffffff81168df5>] ondemand_readahead+0x135/0x260
 [<ffffffff81168f8c>] page_cache_async_readahead+0x6c/0x70
 [<ffffffff8115d5f8>] generic_file_read_iter+0x378/0x590
 [<ffffffff811d12a7>] __vfs_read+0xa7/0xd0
Page has been migrated, last migrate reason: compaction

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/page_owner.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/mm/page_owner.c b/mm/page_owner.c
index ff862b6d12da..421765a53c68 100644
--- a/mm/page_owner.c
+++ b/mm/page_owner.c
@@ -135,8 +135,8 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
 		return -ENOMEM;
 
 	ret = snprintf(kbuf, count,
-			"Page allocated via order %u, mask 0x%x\n",
-			page_ext->order, page_ext->gfp_mask);
+			"Page allocated via order %u, mask %pgg\n",
+			page_ext->order, &page_ext->gfp_mask);
 
 	if (ret >= count)
 		goto err;
@@ -145,23 +145,12 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn,
 	pageblock_mt = get_pfnblock_migratetype(page, pfn);
 	page_mt  = gfpflags_to_migratetype(page_ext->gfp_mask);
 	ret += snprintf(kbuf + ret, count - ret,
-			"PFN %lu type %s Block %lu type %s Flags %s%s%s%s%s%s%s%s%s%s%s%s\n",
+			"PFN %lu type %s Block %lu type %s Flags %pgp\n",
 			pfn,
 			migratetype_names[page_mt],
 			pfn >> pageblock_order,
 			migratetype_names[pageblock_mt],
-			PageLocked(page)	? "K" : " ",
-			PageError(page)		? "E" : " ",
-			PageReferenced(page)	? "R" : " ",
-			PageUptodate(page)	? "U" : " ",
-			PageDirty(page)		? "D" : " ",
-			PageLRU(page)		? "L" : " ",
-			PageActive(page)	? "A" : " ",
-			PageSlab(page)		? "S" : " ",
-			PageWriteback(page)	? "W" : " ",
-			PageCompound(page)	? "C" : " ",
-			PageSwapCache(page)	? "B" : " ",
-			PageMappedToDisk(page)	? "M" : " ");
+			&page->flags);
 
 	if (ret >= count)
 		goto err;
-- 
2.6.3

--
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/

[toc] | [prev] | [next] | [standalone]

#1281629 — Re: [PATCH 1/2] mm, printk: introduce new format string for flags

On Mon, Nov 30 2015, Vlastimil Babka <vbabka@suse.cz> 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 <vbabka@suse.cz>
> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> 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(-)

I'd prefer to have the formatting code in vsprintf.c, so that we'd avoid
having to call vsnprintf recursively (and repeatedly - not that this is
going to be used in hot paths, but if the box is going down it might be
nice to get the debug info out a few thousand cycles earlier). That'll
also make it easier to avoid the bugs below.


> 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)
> +

I think it would be better (and more flexible) if %pg* only stood for
printing the | chain of strings. Let people pass the flags twice if they
also want the numeric value; then they're also able to choose 0-padding
and whatnot, can use other kinds of parentheses, etc., etc. So

  pr_emerg("flags: 0x%08lu [%pgp]\n", printflags, &printflags)


> +	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 <linux/stringify.h>
> +#include <linux/types.h>
>  
>  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 <linux/dcache.h>
>  #include <linux/cred.h>
>  #include <net/addrconf.h>
> +#include <linux/mmdebug.h>
>  
>  #include <asm/page.h>		/* for PAGE_SIZE */
>  #include <asm/sections.h>	/* 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;
> +	}
> +}
> +

That return 0 aka return NULL will lead to an oops when the next thing
is printed. Did you mean 'return buf;'? 


>  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);

Sorry, you can't do it like this. The buf you've been passed from inside
vsnprintf may be beyond end, so end-buf is a negative number which will
(get converted to a huge positive size_t and) trigger a WARN_ONCE and
get you a return value of 0.


> +	flags &= ~mask_out;
>  
>  	for (i = 0; i < count && flags; i++) {
> +		if (buf >= end)
> +			break;

Even if you fix the above, this is also wrong. We have to return the
length of the string that would be generated if there was room enough,
so we cannot make an early return like this. As I said above, the
easiest way to do that is to do it inside vsprintf.c, where we have
e.g. string() available. So I'd do something like


char *format_flags(char *buf, char *end, unsigned long flags,
                   const struct trace_print_flags *names)
{
  unsigned long mask;
  const struct printf_spec strspec = {/* appropriate defaults*/}
  const struct printf_spec numspec = {/* appropriate defaults*/}

  for ( ; flags && names->mask; names++) {
    mask = names->mask;
    if ((flags & mask) != mask)
      continue;
    flags &= ~mask;
    buf = string(buf, end, names->name, strspec);
    if (flags) {
      if (buf < end)
        *buf = '|';
      buf++;
    }
  }
  if (flags)
    buf = number(buf, end, flags, numspec);
  return buf;
}

[where I've assumed that the trace_print_flags array is terminated with
an entry with 0 mask. Passing its length is also possible, but maybe a
little awkward if the arrays are defined in mm/ and contents depend on
.config.] 

Then flags_string() would call this directly with an appropriate array
for names, and we avoid the individual tiny helper
functions. flags_string() can still do the mask_out thing for page
flags, especially when/if the numeric and string representations are not
done at the same time.

Rasmus
--
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/

[toc] | [prev] | [next] | [standalone]

#1282357 — Re: [PATCH 1/2] mm, printk: introduce new format string for flags

On 12/02/2015 12:01 PM, Rasmus Villemoes wrote:
> On Mon, Nov 30 2015, Vlastimil Babka <vbabka@suse.cz> wrote:
> 
> I'd prefer to have the formatting code in vsprintf.c, so that we'd avoid
> having to call vsnprintf recursively (and repeatedly - not that this is
> going to be used in hot paths, but if the box is going down it might be
> nice to get the debug info out a few thousand cycles earlier). That'll
> also make it easier to avoid the bugs below.

OK, I'll try.

>> 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)
>> +
> 
> I think it would be better (and more flexible) if %pg* only stood for
> printing the | chain of strings. Let people pass the flags twice if they
> also want the numeric value; then they're also able to choose 0-padding
> and whatnot, can use other kinds of parentheses, etc., etc. So
> 
>   pr_emerg("flags: 0x%08lu [%pgp]\n", printflags, &printflags)

I had it initially like this, but then thought it was somewhat repetitive and
all current users did use the same format. But I agree it's more generic to do
it as you say so I'll change it.

>> @@ -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;
>> +	}
>> +}
>> +
> 
> That return 0 aka return NULL will lead to an oops when the next thing
> is printed. Did you mean 'return buf;'? 

Uh, right.

>>  
>> -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);
> 
> Sorry, you can't do it like this. The buf you've been passed from inside
> vsnprintf may be beyond end

Ah, didn't realize that :/

> , so end-buf is a negative number which will
> (get converted to a huge positive size_t and) trigger a WARN_ONCE and
> get you a return value of 0.
> 
> 
>> +	flags &= ~mask_out;
>>  
>>  	for (i = 0; i < count && flags; i++) {
>> +		if (buf >= end)
>> +			break;
> 
> Even if you fix the above, this is also wrong. We have to return the
> length of the string that would be generated if there was room enough,
> so we cannot make an early return like this. As I said above, the
> easiest way to do that is to do it inside vsprintf.c, where we have
> e.g. string() available. So I'd do something like
> 
> 
> char *format_flags(char *buf, char *end, unsigned long flags,
>                    const struct trace_print_flags *names)
> {
>   unsigned long mask;
>   const struct printf_spec strspec = {/* appropriate defaults*/}
>   const struct printf_spec numspec = {/* appropriate defaults*/}
> 
>   for ( ; flags && names->mask; names++) {
>     mask = names->mask;
>     if ((flags & mask) != mask)
>       continue;
>     flags &= ~mask;
>     buf = string(buf, end, names->name, strspec);
>     if (flags) {
>       if (buf < end)
>         *buf = '|';
>       buf++;
>     }
>   }
>   if (flags)
>     buf = number(buf, end, flags, numspec);
>   return buf;
> }

Thanks a lot for your review and suggestions!

> [where I've assumed that the trace_print_flags array is terminated with
> an entry with 0 mask. Passing its length is also possible, but maybe a
> little awkward if the arrays are defined in mm/ and contents depend on
> .config.] 

> Then flags_string() would call this directly with an appropriate array
> for names, and we avoid the individual tiny helper
> functions. flags_string() can still do the mask_out thing for page
> flags, especially when/if the numeric and string representations are not
> done at the same time.
> 
> Rasmus

Zero-terminated array is a good idea to get rid of the ARRAY_SIZE with helpers
needing to live in the same .c file etc.

But if I were to keep the array definitions in mm/debug.c with declarations
(which don't know the size yet) in e.g. <linux/mmdebug.h> (which lib/vsnprintf.c
would include so that format_flags() can reference them, is there a more elegant
way than the one below?

--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -7,6 +7,9 @@
 struct page;
 struct vm_area_struct;
 struct mm_struct;
+struct trace_print_flags; // can't include trace_events.h here
+
+extern const struct trace_print_flags *pageflag_names;

 extern void dump_page(struct page *page, const char *reason);
 extern void dump_page_badflags(struct page *page, const char *reason,
diff --git a/mm/debug.c b/mm/debug.c
index a092111920e7..1cbc60544b87 100644
--- a/mm/debug.c
+++ b/mm/debug.c
@@ -23,7 +23,7 @@ char *migrate_reason_names[MR_TYPES] = {
 	"cma",
 };

-static const struct trace_print_flags pageflag_names[] = {
+const struct trace_print_flags __pageflag_names[] = {
 	{1UL << PG_locked,		"locked"	},
 	{1UL << PG_error,		"error"		},
 	{1UL << PG_referenced,		"referenced"	},
@@ -59,6 +59,8 @@ static const struct trace_print_flags pageflag_names[] = {
 #endif
 };

+const struct trace_print_flags *pageflag_names = &__pageflag_names[0];



--
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/

[toc] | [prev] | [next] | [standalone]

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web