From: Vlastimil Babka <vbabka@suse.cz>
To: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
Andrew Morton <akpm@linux-foundation.org>,
Joonsoo Kim <iamjoonsoo.kim@lge.com>,
Minchan Kim <minchan@kernel.org>,
Sasha Levin <sasha.levin@oracle.com>,
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
Mel Gorman <mgorman@suse.de>, Michal Hocko <mhocko@suse.cz>
Subject: Re: [PATCH 1/2] mm, printk: introduce new format string for flags
Date: Wed, 2 Dec 2015 21:34:46 +0100 [thread overview]
Message-ID: <565F55E6.6080201@suse.cz> (raw)
In-Reply-To: <87io4hi06n.fsf@rasmusvillemoes.dk>
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];
next prev parent reply other threads:[~2015-12-02 20:34 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-24 12:36 [PATCH v2 0/9] page_owner improvements for debugging Vlastimil Babka
2015-11-24 12:36 ` [PATCH v2 1/9] mm, debug: fix wrongly filtered flags in dump_vma() Vlastimil Babka
2015-11-27 9:52 ` Vlastimil Babka
2015-11-24 12:36 ` [PATCH v2 2/9] mm, page_owner: print symbolic migratetype of both page and pageblock Vlastimil Babka
2015-11-25 8:11 ` Joonsoo Kim
2015-11-24 12:36 ` [PATCH v2 3/9] mm, page_owner: convert page_owner_inited to static key Vlastimil Babka
2015-11-25 14:52 ` Michal Hocko
2015-11-25 15:08 ` Vlastimil Babka
2015-11-25 15:25 ` Peter Zijlstra
2015-11-25 15:46 ` Michal Hocko
2015-11-24 12:36 ` [PATCH v2 4/9] mm, page_owner: copy page owner info during migration Vlastimil Babka
2015-11-24 12:36 ` [PATCH v2 5/9] mm, page_owner: track and print last migrate reason Vlastimil Babka
2015-11-25 8:13 ` Joonsoo Kim
2015-11-26 10:39 ` Vlastimil Babka
2015-11-24 12:36 ` [PATCH v2 6/9] mm, debug: introduce dump_gfpflag_names() for symbolic printing of gfp_flags Vlastimil Babka
2015-11-25 8:16 ` Joonsoo Kim
2015-11-25 10:28 ` Vlastimil Babka
2015-11-27 3:40 ` yalin wang
2015-11-24 12:36 ` [PATCH v2 7/9] mm, page_owner: dump page owner info from dump_page() Vlastimil Babka
2015-11-25 14:58 ` Michal Hocko
2015-11-26 10:43 ` Vlastimil Babka
2015-11-24 12:36 ` [PATCH v2 8/9] mm, page_alloc: print symbolic gfp_flags on allocation failure Vlastimil Babka
2015-11-25 14:33 ` Michal Hocko
2015-11-24 12:36 ` [PATCH v2 9/9] mm, oom: print symbolic gfp_flags in oom warning Vlastimil Babka
2015-11-25 14:31 ` Michal Hocko
2016-01-07 21:29 ` David Rientjes
2016-01-08 11:34 ` Vlastimil Babka
2015-11-25 14:30 ` [PATCH v2 0/9] page_owner improvements for debugging Michal Hocko
2015-11-30 16:10 ` [PATCH 1/2] mm, printk: introduce new format string for flags Vlastimil Babka
2015-11-30 16:10 ` [PATCH 2/2] mm, page_owner: provide symbolic page flags and gfp_flags Vlastimil Babka
2015-12-02 11:01 ` [PATCH 1/2] mm, printk: introduce new format string for flags Rasmus Villemoes
2015-12-02 20:34 ` Vlastimil Babka [this message]
2015-12-03 12:37 ` Rasmus Villemoes
2015-12-03 13:46 ` Vlastimil Babka
2015-12-04 15:16 ` [PATCH v2 1/3] " Vlastimil Babka
2015-12-04 15:16 ` [PATCH v2 2/3] mm, page_owner: provide symbolic page flags and gfp_flags Vlastimil Babka
2015-12-04 15:16 ` [PATCH v2 3/3] mm, debug: move bad flags printing to bad_page() Vlastimil Babka
2015-12-05 20:00 ` [PATCH v2 1/3] mm, printk: introduce new format string for flags Rasmus Villemoes
2015-12-09 11:29 ` Arnd Bergmann
2015-12-09 20:48 ` Vlastimil Babka
2015-12-10 12:26 ` James Hogan
2015-12-10 2:59 ` Joonsoo Kim
2015-12-10 4:04 ` Steven Rostedt
2015-12-10 4:12 ` Joonsoo Kim
2015-12-10 8:41 ` Rasmus Villemoes
2015-12-10 10:03 ` Vlastimil Babka
2015-12-14 3:03 ` Joonsoo Kim
2015-12-10 3:51 ` Steven Rostedt
2015-12-10 9:51 ` Vlastimil Babka
2015-12-02 17:40 ` [PATCH 1/2] " yalin wang
2015-12-02 21:04 ` Vlastimil Babka
2015-12-03 0:11 ` yalin wang
2015-12-03 8:03 ` Rasmus Villemoes
2015-12-03 18:38 ` yalin wang
2015-12-04 1:04 ` yalin wang
2015-12-04 14:15 ` Vlastimil Babka
2015-12-10 4:03 ` Steven Rostedt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=565F55E6.6080201@suse.cz \
--to=vbabka@suse.cz \
--cc=akpm@linux-foundation.org \
--cc=iamjoonsoo.kim@lge.com \
--cc=kirill.shutemov@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux@rasmusvillemoes.dk \
--cc=mgorman@suse.de \
--cc=mhocko@suse.cz \
--cc=minchan@kernel.org \
--cc=sasha.levin@oracle.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).