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

[Steven Rostedt]

On Fri, Dec 04, 2015 at 03:15:45PM +0100, Vlastimil Babka wrote:
> On 12/03/2015 07:38 PM, yalin wang wrote:
> >that’s all, see cpumask_pr_args(masks) macro,
> >it also use macro and  %*pb  to print cpu mask .
> >i think this method is not very complex to use .
> 
> Well, one also has to write the appropriate translation tables.
> 
> >search source code ,
> >there is lots of printk to print flag into hex number :
> >$ grep -n  -r 'printk.*flag.*%x’  .
> >it will be great if this flag string print is generic.
> 
> I think it can always be done later, this is an internal API. For now we
> just have 3 quite generic flags, so let's not over-engineer things right
> now.
> 

As long as it is never used in the TP_printk() part of a tracepoint. As soon
as it is, trace-cmd and perf will update parse-events to handle that
parameter, and as soon as that is done, it becomes a userspace ABI.

Just be warned.

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Steven Rostedt]

On Fri, Dec 04, 2015 at 03:15:45PM +0100, Vlastimil Babka wrote:
> On 12/03/2015 07:38 PM, yalin wang wrote:
> >that’s all, see cpumask_pr_args(masks) macro,
> >it also use macro and  %*pb  to print cpu mask .
> >i think this method is not very complex to use .
> 
> Well, one also has to write the appropriate translation tables.
> 
> >search source code ,
> >there is lots of printk to print flag into hex number :
> >$ grep -n  -r 'printk.*flag.*%x’  .
> >it will be great if this flag string print is generic.
> 
> I think it can always be done later, this is an internal API. For now we
> just have 3 quite generic flags, so let's not over-engineer things right
> now.
> 

As long as it is never used in the TP_printk() part of a tracepoint. As soon
as it is, trace-cmd and perf will update parse-events to handle that
parameter, and as soon as that is done, it becomes a userspace ABI.

Just be warned.

-- Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Vlastimil Babka]

On 12/03/2015 07:38 PM, yalin wang wrote:

that’s all, see cpumask_pr_args(masks) macro,
it also use macro and  %*pb  to print cpu mask .
i think this method is not very complex to use .


Well, one also has to write the appropriate translation tables.


search source code ,
there is lots of printk to print flag into hex number :
$ grep -n  -r 'printk.*flag.*%x’  .
it will be great if this flag string print is generic.


I think it can always be done later, this is an internal API. For now we 
just have 3 quite generic flags, so let's not over-engineer things right 
now.



Thanks


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Vlastimil Babka]

On 12/03/2015 07:38 PM, yalin wang wrote:

that’s all, see cpumask_pr_args(masks) macro,
it also use macro and  %*pb  to print cpu mask .
i think this method is not very complex to use .


Well, one also has to write the appropriate translation tables.


search source code ,
there is lots of printk to print flag into hex number :
$ grep -n  -r 'printk.*flag.*%x’  .
it will be great if this flag string print is generic.


I think it can always be done later, this is an internal API. For now we 
just have 3 quite generic flags, so let's not over-engineer things right 
now.



Thanks


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[yalin wang]

>> Technically, I think the answer is yes, at least in C99 (and I suppose
>> gcc would accept it in gnu89 mode as well).
>> 
>> printk("%pg\n", &(struct flag_printer){.flags = my_flags, .names = 
>> vmaflags_names});
>> 
>> Not tested, and I still don't think it would be particularly readable
>> even when macroized
>> 
>> printk("%pg\n", PRINTF_VMAFLAGS(my_flags));
> i test on gcc 4.9.3, it can work for this method,
> so the final solution like this:
> printk.h:
> struct flag_fmt_spec {
>   unsigned long flag;
>   struct trace_print_flags *flags;
>   int array_size;
>   char delimiter; }
> 
> #define FLAG_FORMAT(flag, flag_array, delimiter) (&(struct flag_ft_spec){ 
> .flag = flag, .flags = flag_array, .array_size = ARRAY_SIZE(flag_array), 
> .delimiter = delimiter})
> #define VMA_FLAG_FORMAT(flag)  FLAG_FORMAT(flag, vmaflags_names, ‘|’)
a little change:
#define VMA_FLAG_FORMAT(vma)  FLAG_FORMAT(vma->vm_flags, 
vmaflags_names, ‘|’)


> source code:
> printk("%pg\n", VMA_FLAG_FORMAT(my_flags)); 
a little change:
printk("%pg\n", VMA_FLAG_FORMAT(vma)); 

> 
> that’s all, see cpumask_pr_args(masks) macro,
> it also use macro and  %*pb  to print cpu mask .
> i think this method is not very complex to use .
> 
> search source code ,
> there is lots of printk to print flag into hex number :
> $ grep -n  -r 'printk.*flag.*%x’  .
> it will be great if this flag string print is generic.
> 
> Thanks

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[yalin wang]

> On Dec 3, 2015, at 00:03, Rasmus Villemoes  wrote:
> 
> On Thu, Dec 03 2015, yalin wang  wrote:
> 
>>> On Dec 2, 2015, at 13:04, Vlastimil Babka  wrote:
>>> 
>>> On 12/02/2015 06:40 PM, yalin wang wrote:
>>> 
>>> (please trim your reply next time, no need to quote whole patch here)
>>> 
 i am thinking why not make %pg* to be more generic ?
 not restricted to only GFP / vma flags / page flags .
 so could we change format like this ?
 define a flag spec struct to include flag and trace_print_flags and some 
 other option :
 typedef struct { 
 unsigned long flag;
 structtrace_print_flags *flags;
 unsigned long option; } flag_sec;
 flag_sec my_flag;
 in printk we only pass like this :
 printk(“%pg\n”, _flag) ;
 then it can print any flags defined by user .
 more useful for other drivers to use .
>>> 
>>> I don't know, it sounds quite complicated
> 
> Agreed, I think this would be premature generalization. There's also
> some value in having the individual %pgX specifiers, as that allows
> individual tweaks such as the mask_out for page flags.
> 
> given that we had no flags printing
>> 
if we use this generic method, %pgX where X can be used to specify some flag to
mask out some thing .  it will be great .

> 
> Compared to printk("%pgv\n", >flag), I know which I'd prefer to read.
> 
>> i am not if DECLARE_FLAG_PRINTK_FMT and FLAG_PRINTK_FMT macro 
>> can be defined into one macro ?
>> maybe need some trick here .
>> 
>> is it possible ?
> 
> Technically, I think the answer is yes, at least in C99 (and I suppose
> gcc would accept it in gnu89 mode as well).
> 
> printk("%pg\n", &(struct flag_printer){.flags = my_flags, .names = 
> vmaflags_names});
> 
> Not tested, and I still don't think it would be particularly readable
> even when macroized
> 
> printk("%pg\n", PRINTF_VMAFLAGS(my_flags));
i test on gcc 4.9.3, it can work for this method,
so the final solution like this:
printk.h:
struct flag_fmt_spec {
unsigned long flag;
struct trace_print_flags *flags;
int array_size;
char delimiter; }

#define FLAG_FORMAT(flag, flag_array, delimiter) (&(struct flag_ft_spec){ .flag 
= flag, .flags = flag_array, .array_size = ARRAY_SIZE(flag_array), .delimiter = 
delimiter})
#define VMA_FLAG_FORMAT(flag)  FLAG_FORMAT(flag, vmaflags_names, ‘|')

source code:
printk("%pg\n", VMA_FLAG_FORMAT(my_flags)); 

that’s all, see cpumask_pr_args(masks) macro,
it also use macro and  %*pb  to print cpu mask .
i think this method is not very complex to use .

search source code ,
there is lots of printk to print flag into hex number :
$ grep -n  -r 'printk.*flag.*%x’  .
it will be great if this flag string print is generic.

Thanks









--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Vlastimil Babka]

On 12/03/2015 01:37 PM, Rasmus Villemoes wrote:
> On Wed, Dec 02 2015, Vlastimil Babka  wrote:
>> --- 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];
> 
> Ugh. I think it would be better if either the definition of struct
> trace_print_flags is moved somewhere where everybody can see it or to
> make our own identical type definition. For now I'd go with the latter,
> also since this doesn't really have anything to do with the tracing
> subsystem. Then just declare the array in the header
> 
> extern const struct print_flags pageflag_names[];

Ugh so yesterday I copy/pasted the definition and still got an error, which I
probably didn't read closely enough. I assumed that if it needs the full
definition of "struct trace_print_flags" here to know the size, it would also
need to know the lenght of the array as well.

But now it works. Well, copy/pasting the definition fails as long as both
headers are included and it's redefining the struct (even though it's the same
thing). But looks like I can move it from trace_events.h to tracepoint.h and it
won't blow off (knock knock).

I suck at C.

> (If you do the extra indirection thing, __pageflag_names could still be
> static, and it would be best to declare the pointer itself const as
> well, but I'd rather we don't go that way.)
> 
> Rasmus
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Rasmus Villemoes]

On Wed, Dec 02 2015, Vlastimil Babka  wrote:

>> [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.] 
...
>
>> 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.  (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];

Ugh. I think it would be better if either the definition of struct
trace_print_flags is moved somewhere where everybody can see it or to
make our own identical type definition. For now I'd go with the latter,
also since this doesn't really have anything to do with the tracing
subsystem. Then just declare the array in the header

extern const struct print_flags pageflag_names[];

(If you do the extra indirection thing, __pageflag_names could still be
static, and it would be best to declare the pointer itself const as
well, but I'd rather we don't go that way.)

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Rasmus Villemoes]

On Thu, Dec 03 2015, yalin wang  wrote:

>> On Dec 2, 2015, at 13:04, Vlastimil Babka  wrote:
>> 
>> On 12/02/2015 06:40 PM, yalin wang wrote:
>> 
>> (please trim your reply next time, no need to quote whole patch here)
>> 
>>> i am thinking why not make %pg* to be more generic ?
>>> not restricted to only GFP / vma flags / page flags .
>>> so could we change format like this ?
>>> define a flag spec struct to include flag and trace_print_flags and some 
>>> other option :
>>> typedef struct { 
>>> unsigned long flag;
>>> structtrace_print_flags *flags;
>>> unsigned long option; } flag_sec;
>>> flag_sec my_flag;
>>> in printk we only pass like this :
>>> printk(“%pg\n”, _flag) ;
>>> then it can print any flags defined by user .
>>> more useful for other drivers to use .
>> 
>> I don't know, it sounds quite complicated

Agreed, I think this would be premature generalization. There's also
some value in having the individual %pgX specifiers, as that allows
individual tweaks such as the mask_out for page flags.

 given that we had no flags printing
>> for years and now there's just three kinds of them. The extra struct 
>> flag_sec is
>> IMHO nuissance. No other printk format needs such thing AFAIK? For example, 
>> if I
>> were to print page flags from several places, each would have to define the
>> struct flag_sec instance, or some header would have to provide it?
> this can be avoided by provide a macro in header file .
> we can add a new struct to declare trace_print_flags :
> for example:
> #define DECLARE_FLAG_PRINTK_FMT(name, flags_array)   flag_spec name = { 
> .flags = flags_array};
> #define FLAG_PRINTK_FMT(name, flag) ({  name.flag = flag;  })
>
> in source code :
> DECLARE_FLAG_PRINTK_FMT(my_flag, vmaflags_names);
> printk(“%pg\n”, FLAG_PRINTK_FMT(my_flag, vma->flag));
>

Compared to printk("%pgv\n", >flag), I know which I'd prefer to read.

> i am not if DECLARE_FLAG_PRINTK_FMT and FLAG_PRINTK_FMT macro 
> can be defined into one macro ?
> maybe need some trick here .
>
> is it possible ?

Technically, I think the answer is yes, at least in C99 (and I suppose
gcc would accept it in gnu89 mode as well).

printk("%pg\n", &(struct flag_printer){.flags = my_flags, .names = 
vmaflags_names});

Not tested, and I still don't think it would be particularly readable
even when macroized

printk("%pg\n", PRINTF_VMAFLAGS(my_flags));

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Rasmus Villemoes]

On Wed, Dec 02 2015, Vlastimil Babka  wrote:

>> [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.] 
...
>
>> 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.  (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];

Ugh. I think it would be better if either the definition of struct
trace_print_flags is moved somewhere where everybody can see it or to
make our own identical type definition. For now I'd go with the latter,
also since this doesn't really have anything to do with the tracing
subsystem. Then just declare the array in the header

extern const struct print_flags pageflag_names[];

(If you do the extra indirection thing, __pageflag_names could still be
static, and it would be best to declare the pointer itself const as
well, but I'd rather we don't go that way.)

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Rasmus Villemoes]

On Thu, Dec 03 2015, yalin wang  wrote:

>> On Dec 2, 2015, at 13:04, Vlastimil Babka  wrote:
>> 
>> On 12/02/2015 06:40 PM, yalin wang wrote:
>> 
>> (please trim your reply next time, no need to quote whole patch here)
>> 
>>> i am thinking why not make %pg* to be more generic ?
>>> not restricted to only GFP / vma flags / page flags .
>>> so could we change format like this ?
>>> define a flag spec struct to include flag and trace_print_flags and some 
>>> other option :
>>> typedef struct { 
>>> unsigned long flag;
>>> structtrace_print_flags *flags;
>>> unsigned long option; } flag_sec;
>>> flag_sec my_flag;
>>> in printk we only pass like this :
>>> printk(“%pg\n”, _flag) ;
>>> then it can print any flags defined by user .
>>> more useful for other drivers to use .
>> 
>> I don't know, it sounds quite complicated

Agreed, I think this would be premature generalization. There's also
some value in having the individual %pgX specifiers, as that allows
individual tweaks such as the mask_out for page flags.

 given that we had no flags printing
>> for years and now there's just three kinds of them. The extra struct 
>> flag_sec is
>> IMHO nuissance. No other printk format needs such thing AFAIK? For example, 
>> if I
>> were to print page flags from several places, each would have to define the
>> struct flag_sec instance, or some header would have to provide it?
> this can be avoided by provide a macro in header file .
> we can add a new struct to declare trace_print_flags :
> for example:
> #define DECLARE_FLAG_PRINTK_FMT(name, flags_array)   flag_spec name = { 
> .flags = flags_array};
> #define FLAG_PRINTK_FMT(name, flag) ({  name.flag = flag;  })
>
> in source code :
> DECLARE_FLAG_PRINTK_FMT(my_flag, vmaflags_names);
> printk(“%pg\n”, FLAG_PRINTK_FMT(my_flag, vma->flag));
>

Compared to printk("%pgv\n", >flag), I know which I'd prefer to read.

> i am not if DECLARE_FLAG_PRINTK_FMT and FLAG_PRINTK_FMT macro 
> can be defined into one macro ?
> maybe need some trick here .
>
> is it possible ?

Technically, I think the answer is yes, at least in C99 (and I suppose
gcc would accept it in gnu89 mode as well).

printk("%pg\n", &(struct flag_printer){.flags = my_flags, .names = 
vmaflags_names});

Not tested, and I still don't think it would be particularly readable
even when macroized

printk("%pg\n", PRINTF_VMAFLAGS(my_flags));

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Vlastimil Babka]

On 12/03/2015 01:37 PM, Rasmus Villemoes wrote:
> On Wed, Dec 02 2015, Vlastimil Babka  wrote:
>> --- 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];
> 
> Ugh. I think it would be better if either the definition of struct
> trace_print_flags is moved somewhere where everybody can see it or to
> make our own identical type definition. For now I'd go with the latter,
> also since this doesn't really have anything to do with the tracing
> subsystem. Then just declare the array in the header
> 
> extern const struct print_flags pageflag_names[];

Ugh so yesterday I copy/pasted the definition and still got an error, which I
probably didn't read closely enough. I assumed that if it needs the full
definition of "struct trace_print_flags" here to know the size, it would also
need to know the lenght of the array as well.

But now it works. Well, copy/pasting the definition fails as long as both
headers are included and it's redefining the struct (even though it's the same
thing). But looks like I can move it from trace_events.h to tracepoint.h and it
won't blow off (knock knock).

I suck at C.

> (If you do the extra indirection thing, __pageflag_names could still be
> static, and it would be best to declare the pointer itself const as
> well, but I'd rather we don't go that way.)
> 
> Rasmus
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[yalin wang]

> On Dec 3, 2015, at 00:03, Rasmus Villemoes  wrote:
> 
> On Thu, Dec 03 2015, yalin wang  wrote:
> 
>>> On Dec 2, 2015, at 13:04, Vlastimil Babka  wrote:
>>> 
>>> On 12/02/2015 06:40 PM, yalin wang wrote:
>>> 
>>> (please trim your reply next time, no need to quote whole patch here)
>>> 
 i am thinking why not make %pg* to be more generic ?
 not restricted to only GFP / vma flags / page flags .
 so could we change format like this ?
 define a flag spec struct to include flag and trace_print_flags and some 
 other option :
 typedef struct { 
 unsigned long flag;
 structtrace_print_flags *flags;
 unsigned long option; } flag_sec;
 flag_sec my_flag;
 in printk we only pass like this :
 printk(“%pg\n”, _flag) ;
 then it can print any flags defined by user .
 more useful for other drivers to use .
>>> 
>>> I don't know, it sounds quite complicated
> 
> Agreed, I think this would be premature generalization. There's also
> some value in having the individual %pgX specifiers, as that allows
> individual tweaks such as the mask_out for page flags.
> 
> given that we had no flags printing
>> 
if we use this generic method, %pgX where X can be used to specify some flag to
mask out some thing .  it will be great .

> 
> Compared to printk("%pgv\n", >flag), I know which I'd prefer to read.
> 
>> i am not if DECLARE_FLAG_PRINTK_FMT and FLAG_PRINTK_FMT macro 
>> can be defined into one macro ?
>> maybe need some trick here .
>> 
>> is it possible ?
> 
> Technically, I think the answer is yes, at least in C99 (and I suppose
> gcc would accept it in gnu89 mode as well).
> 
> printk("%pg\n", &(struct flag_printer){.flags = my_flags, .names = 
> vmaflags_names});
> 
> Not tested, and I still don't think it would be particularly readable
> even when macroized
> 
> printk("%pg\n", PRINTF_VMAFLAGS(my_flags));
i test on gcc 4.9.3, it can work for this method,
so the final solution like this:
printk.h:
struct flag_fmt_spec {
unsigned long flag;
struct trace_print_flags *flags;
int array_size;
char delimiter; }

#define FLAG_FORMAT(flag, flag_array, delimiter) (&(struct flag_ft_spec){ .flag 
= flag, .flags = flag_array, .array_size = ARRAY_SIZE(flag_array), .delimiter = 
delimiter})
#define VMA_FLAG_FORMAT(flag)  FLAG_FORMAT(flag, vmaflags_names, ‘|')

source code:
printk("%pg\n", VMA_FLAG_FORMAT(my_flags)); 

that’s all, see cpumask_pr_args(masks) macro,
it also use macro and  %*pb  to print cpu mask .
i think this method is not very complex to use .

search source code ,
there is lots of printk to print flag into hex number :
$ grep -n  -r 'printk.*flag.*%x’  .
it will be great if this flag string print is generic.

Thanks









--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[yalin wang]

>> Technically, I think the answer is yes, at least in C99 (and I suppose
>> gcc would accept it in gnu89 mode as well).
>> 
>> printk("%pg\n", &(struct flag_printer){.flags = my_flags, .names = 
>> vmaflags_names});
>> 
>> Not tested, and I still don't think it would be particularly readable
>> even when macroized
>> 
>> printk("%pg\n", PRINTF_VMAFLAGS(my_flags));
> i test on gcc 4.9.3, it can work for this method,
> so the final solution like this:
> printk.h:
> struct flag_fmt_spec {
>   unsigned long flag;
>   struct trace_print_flags *flags;
>   int array_size;
>   char delimiter; }
> 
> #define FLAG_FORMAT(flag, flag_array, delimiter) (&(struct flag_ft_spec){ 
> .flag = flag, .flags = flag_array, .array_size = ARRAY_SIZE(flag_array), 
> .delimiter = delimiter})
> #define VMA_FLAG_FORMAT(flag)  FLAG_FORMAT(flag, vmaflags_names, ‘|’)
a little change:
#define VMA_FLAG_FORMAT(vma)  FLAG_FORMAT(vma->vm_flags, 
vmaflags_names, ‘|’)


> source code:
> printk("%pg\n", VMA_FLAG_FORMAT(my_flags)); 
a little change:
printk("%pg\n", VMA_FLAG_FORMAT(vma)); 

> 
> that’s all, see cpumask_pr_args(masks) macro,
> it also use macro and  %*pb  to print cpu mask .
> i think this method is not very complex to use .
> 
> search source code ,
> there is lots of printk to print flag into hex number :
> $ grep -n  -r 'printk.*flag.*%x’  .
> it will be great if this flag string print is generic.
> 
> Thanks

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[yalin wang]

> On Dec 2, 2015, at 13:04, Vlastimil Babka  wrote:
> 
> On 12/02/2015 06:40 PM, yalin wang wrote:
> 
> (please trim your reply next time, no need to quote whole patch here)
> 
>> i am thinking why not make %pg* to be more generic ?
>> not restricted to only GFP / vma flags / page flags .
>> so could we change format like this ?
>> define a flag spec struct to include flag and trace_print_flags and some 
>> other option :
>> typedef struct { 
>> unsigned long flag;
>> structtrace_print_flags *flags;
>> unsigned long option; } flag_sec;
>> flag_sec my_flag;
>> in printk we only pass like this :
>> printk(“%pg\n”, _flag) ;
>> then it can print any flags defined by user .
>> more useful for other drivers to use .
> 
> I don't know, it sounds quite complicated given that we had no flags printing
> for years and now there's just three kinds of them. The extra struct flag_sec 
> is
> IMHO nuissance. No other printk format needs such thing AFAIK? For example, 
> if I
> were to print page flags from several places, each would have to define the
> struct flag_sec instance, or some header would have to provide it?
this can be avoided by provide a macro in header file .
we can add a new struct to declare trace_print_flags :
for example:
#define DECLARE_FLAG_PRINTK_FMT(name, flags_array)   flag_spec name = { .flags 
= flags_array};
#define FLAG_PRINTK_FMT(name, flag) ({  name.flag = flag;  })

in source code :
DECLARE_FLAG_PRINTK_FMT(my_flag, vmaflags_names);
printk(“%pg\n”, FLAG_PRINTK_FMT(my_flag, vma->flag));

i am not if DECLARE_FLAG_PRINTK_FMT and FLAG_PRINTK_FMT macro 
can be defined into one macro ?
maybe need some trick here .

is it possible ?


Thanks



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Vlastimil Babka]

On 12/02/2015 06:40 PM, yalin wang wrote:

(please trim your reply next time, no need to quote whole patch here)

> i am thinking why not make %pg* to be more generic ?
> not restricted to only GFP / vma flags / page flags .
> so could we change format like this ?
> define a flag spec struct to include flag and trace_print_flags and some 
> other option :
> typedef struct { 
> unsigned long flag;
> struct trace_print_flags *flags;
> unsigned long option; } flag_sec;
> flag_sec my_flag;
> in printk we only pass like this :
> printk(“%pg\n”, _flag) ;
> then it can print any flags defined by user .
> more useful for other drivers to use .

I don't know, it sounds quite complicated given that we had no flags printing
for years and now there's just three kinds of them. The extra struct flag_sec is
IMHO nuissance. No other printk format needs such thing AFAIK? For example, if I
were to print page flags from several places, each would have to define the
struct flag_sec instance, or some header would have to provide it?

I could maybe accept passing a flag value and trace_print_flags * as two
separate parameters, but I guess that breaks an ancient invariant of one
parameter per format string...

> Thanks
> 
> 
> 
> 
> 
> 
> 
> 
> 
>  
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Vlastimil Babka]

On 12/02/2015 12:01 PM, Rasmus Villemoes wrote:
> On Mon, Nov 30 2015, Vlastimil Babka  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:
>> +
>> +%pgp0x1f886c(referenced|uptodate|lru|active|private)
>> +%pgg0x24202c4(GFP_USER|GFP_DMA32|GFP_NOWARN)
>> +%pgv0x875(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, )

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 

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

[yalin wang]

> On Nov 30, 2015, at 08:10, Vlastimil Babka  wrote:
> 
> In mm we use several kinds of flags bitfields that are sometimes printed for
> debugging purposes, or exported to userspace via sysfs. To make them easier to
> interpret independently on kernel version and config, we want to dump also the
> symbolic flag names. So far this has been done with repeated calls to
> pr_cont(), which is unreliable on SMP, and not usable for e.g. sysfs export.
> 
> To get a more reliable and universal solution, this patch extends printk()
> format string for pointers to handle the page flags (%pgp), gfp_flags (%pgg)
> and vma flags (%pgv). Existing users of dump_flag_names() are converted and
> simplified.
> 
> It would be possible to pass flags by value instead of pointer, but the %p
> format string for pointers already has extensions for various kernel
> structures, so it's a good fit, and the extra indirection in a non-critical
> path is negligible.
> 
> Signed-off-by: Vlastimil Babka 
> Cc: Rasmus Villemoes 
> ---
> I'm sending it on top of the page_owner series, as it's already in mmotm.
> But to reduce churn (in case this approach is accepted), I can later
> incorporate it and resend it whole.
> 
> Documentation/printk-formats.txt |  14 
> include/linux/mmdebug.h  |   5 +-
> lib/vsprintf.c   |  31 
> mm/debug.c   | 150 ++-
> mm/oom_kill.c|   5 +-
> mm/page_alloc.c  |   5 +-
> mm/page_owner.c  |   5 +-
> 7 files changed, 140 insertions(+), 75 deletions(-)
> 
> diff --git a/Documentation/printk-formats.txt 
> b/Documentation/printk-formats.txt
> index b784c270105f..4b5156e74b09 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -292,6 +292,20 @@ Raw pointer value SHOULD be printed with %p. The kernel 
> supports
> 
>   Passed by reference.
> 
> +Flags bitfields such as page flags, gfp_flags:
> +
> + %pgp0x1f886c(referenced|uptodate|lru|active|private)
> + %pgg0x24202c4(GFP_USER|GFP_DMA32|GFP_NOWARN)
> + %pgv0x875(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:
> 
>   %pNF0xc000
> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
> index 3b77fab7ad28..e6518df259ca 100644
> --- a/include/linux/mmdebug.h
> +++ b/include/linux/mmdebug.h
> @@ -2,6 +2,7 @@
> #define LINUX_MM_DEBUG_H 1
> 
> #include 
> +#include 
> 
> struct page;
> struct vm_area_struct;
> @@ -10,7 +11,9 @@ struct mm_struct;
> extern void dump_page(struct page *page, const char *reason);
> extern void dump_page_badflags(struct page *page, const char *reason,
>  unsigned long badflags);
> -extern void dump_gfpflag_names(unsigned long gfp_flags);
> +extern char *format_page_flags(unsigned long flags, char *buf, char *end);
> +extern char *format_vma_flags(unsigned long flags, char *buf, char *end);
> +extern char *format_gfp_flags(gfp_t gfp_flags, char *buf, char*end);
> void dump_vma(const struct vm_area_struct *vma);
> void dump_mm(const struct mm_struct *mm);
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index f9cee8e1233c..41cd122bd307 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -31,6 +31,7 @@
> #include 
> #include 
> #include 
> +#include 
> 
> #include  /* for PAGE_SIZE */
> #include  /* for dereference_function_descriptor() */
> @@ -1361,6 +1362,29 @@ char *clock(char *buf, char *end, struct clk *clk, 
> struct printf_spec spec,
>   }
> }
> 
> +static noinline_for_stack
> +char *flags_string(char *buf, char *end, void *flags_ptr,
> + struct printf_spec spec, const char *fmt)
> +{
> + unsigned long flags;
> + gfp_t gfp_flags;
> +
> + switch (fmt[1]) {
> + case 'p':
> + flags = *(unsigned long *)flags_ptr;
> + return format_page_flags(flags, buf, end);
> + case 'v':
> + flags = *(unsigned long *)flags_ptr;
> + return format_vma_flags(flags, buf, end);
> + case 'g':
> + gfp_flags = *(gfp_t *)flags_ptr;
> + return format_gfp_flags(gfp_flags, buf, end);
> + default:
> + WARN_ONCE(1, "Unsupported flags modifier: %c\n", fmt[1]);
> + return 0;
> + }
> +}
> +
> int kptr_restrict __read_mostly;
> 
> /*
> @@ -1448,6 +1472,11 @@ int kptr_restrict __read_mostly;
>  * - 'Cn' For a clock, it prints the name (Common Clock Framework) or address
>  *(legacy clock framework) of the clock
>  * - 'Cr' For a clock, it 

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

[Rasmus Villemoes]

On Mon, Nov 30 2015, Vlastimil Babka  wrote:

> In mm we use several kinds of flags bitfields that are sometimes printed for
> debugging purposes, or exported to userspace via sysfs. To make them easier to
> interpret independently on kernel version and config, we want to dump also the
> symbolic flag names. So far this has been done with repeated calls to
> pr_cont(), which is unreliable on SMP, and not usable for e.g. sysfs export.
>
> To get a more reliable and universal solution, this patch extends printk()
> format string for pointers to handle the page flags (%pgp), gfp_flags (%pgg)
> and vma flags (%pgv). Existing users of dump_flag_names() are converted and
> simplified.
>
> It would be possible to pass flags by value instead of pointer, but the %p
> format string for pointers already has extensions for various kernel
> structures, so it's a good fit, and the extra indirection in a non-critical
> path is negligible.
>
> Signed-off-by: Vlastimil Babka 
> Cc: Rasmus Villemoes 
> ---
> I'm sending it on top of the page_owner series, as it's already in mmotm.
> But to reduce churn (in case this approach is accepted), I can later
> incorporate it and resend it whole.
>
>  Documentation/printk-formats.txt |  14 
>  include/linux/mmdebug.h  |   5 +-
>  lib/vsprintf.c   |  31 
>  mm/debug.c   | 150 
> ++-
>  mm/oom_kill.c|   5 +-
>  mm/page_alloc.c  |   5 +-
>  mm/page_owner.c  |   5 +-
>  7 files changed, 140 insertions(+), 75 deletions(-)

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:
> +
> + %pgp0x1f886c(referenced|uptodate|lru|active|private)
> + %pgg0x24202c4(GFP_USER|GFP_DMA32|GFP_NOWARN)
> + %pgv0x875(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, )


> + 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:
>  
>   %pNF0xc000
> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
> index 3b77fab7ad28..e6518df259ca 100644
> --- a/include/linux/mmdebug.h
> +++ b/include/linux/mmdebug.h
> @@ -2,6 +2,7 @@
>  #define LINUX_MM_DEBUG_H 1
>  
>  #include 
> +#include 
>  
>  struct page;
>  struct vm_area_struct;
> @@ -10,7 +11,9 @@ struct mm_struct;
>  extern void dump_page(struct page *page, const char *reason);
>  extern void dump_page_badflags(struct page *page, const char *reason,
>  unsigned long badflags);
> -extern void dump_gfpflag_names(unsigned long gfp_flags);
> +extern char *format_page_flags(unsigned long flags, char *buf, char *end);
> +extern char *format_vma_flags(unsigned long flags, char *buf, char *end);
> +extern char *format_gfp_flags(gfp_t gfp_flags, char *buf, char*end);
>  void dump_vma(const struct vm_area_struct *vma);
>  void dump_mm(const struct mm_struct *mm);
>  
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index f9cee8e1233c..41cd122bd307 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -31,6 +31,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include /* for PAGE_SIZE */
>  #include /* for dereference_function_descriptor() */
> @@ -1361,6 +1362,29 @@ char *clock(char *buf, char *end, struct clk *clk, 
> struct printf_spec spec,
>   }
>  }
>  
> +static noinline_for_stack
> +char *flags_string(char *buf, char *end, void *flags_ptr,
> + struct printf_spec spec, const char *fmt)
> +{
> + unsigned long flags;
> + gfp_t gfp_flags;
> +
> + switch (fmt[1]) {
> + case 'p':
> + flags = *(unsigned long *)flags_ptr;
> + 

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

[yalin wang]

> On Nov 30, 2015, at 08:10, Vlastimil Babka  wrote:
> 
> In mm we use several kinds of flags bitfields that are sometimes printed for
> debugging purposes, or exported to userspace via sysfs. To make them easier to
> interpret independently on kernel version and config, we want to dump also the
> symbolic flag names. So far this has been done with repeated calls to
> pr_cont(), which is unreliable on SMP, and not usable for e.g. sysfs export.
> 
> To get a more reliable and universal solution, this patch extends printk()
> format string for pointers to handle the page flags (%pgp), gfp_flags (%pgg)
> and vma flags (%pgv). Existing users of dump_flag_names() are converted and
> simplified.
> 
> It would be possible to pass flags by value instead of pointer, but the %p
> format string for pointers already has extensions for various kernel
> structures, so it's a good fit, and the extra indirection in a non-critical
> path is negligible.
> 
> Signed-off-by: Vlastimil Babka 
> Cc: Rasmus Villemoes 
> ---
> I'm sending it on top of the page_owner series, as it's already in mmotm.
> But to reduce churn (in case this approach is accepted), I can later
> incorporate it and resend it whole.
> 
> Documentation/printk-formats.txt |  14 
> include/linux/mmdebug.h  |   5 +-
> lib/vsprintf.c   |  31 
> mm/debug.c   | 150 ++-
> mm/oom_kill.c|   5 +-
> mm/page_alloc.c  |   5 +-
> mm/page_owner.c  |   5 +-
> 7 files changed, 140 insertions(+), 75 deletions(-)
> 
> diff --git a/Documentation/printk-formats.txt 
> b/Documentation/printk-formats.txt
> index b784c270105f..4b5156e74b09 100644
> --- a/Documentation/printk-formats.txt
> +++ b/Documentation/printk-formats.txt
> @@ -292,6 +292,20 @@ Raw pointer value SHOULD be printed with %p. The kernel 
> supports
> 
>   Passed by reference.
> 
> +Flags bitfields such as page flags, gfp_flags:
> +
> + %pgp0x1f886c(referenced|uptodate|lru|active|private)
> + %pgg0x24202c4(GFP_USER|GFP_DMA32|GFP_NOWARN)
> + %pgv0x875(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:
> 
>   %pNF0xc000
> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
> index 3b77fab7ad28..e6518df259ca 100644
> --- a/include/linux/mmdebug.h
> +++ b/include/linux/mmdebug.h
> @@ -2,6 +2,7 @@
> #define LINUX_MM_DEBUG_H 1
> 
> #include 
> +#include 
> 
> struct page;
> struct vm_area_struct;
> @@ -10,7 +11,9 @@ struct mm_struct;
> extern void dump_page(struct page *page, const char *reason);
> extern void dump_page_badflags(struct page *page, const char *reason,
>  unsigned long badflags);
> -extern void dump_gfpflag_names(unsigned long gfp_flags);
> +extern char *format_page_flags(unsigned long flags, char *buf, char *end);
> +extern char *format_vma_flags(unsigned long flags, char *buf, char *end);
> +extern char *format_gfp_flags(gfp_t gfp_flags, char *buf, char*end);
> void dump_vma(const struct vm_area_struct *vma);
> void dump_mm(const struct mm_struct *mm);
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index f9cee8e1233c..41cd122bd307 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -31,6 +31,7 @@
> #include 
> #include 
> #include 
> +#include 
> 
> #include  /* for PAGE_SIZE */
> #include  /* for dereference_function_descriptor() */
> @@ -1361,6 +1362,29 @@ char *clock(char *buf, char *end, struct clk *clk, 
> struct printf_spec spec,
>   }
> }
> 
> +static noinline_for_stack
> +char *flags_string(char *buf, char *end, void *flags_ptr,
> + struct printf_spec spec, const char *fmt)
> +{
> + unsigned long flags;
> + gfp_t gfp_flags;
> +
> + switch (fmt[1]) {
> + case 'p':
> + flags = *(unsigned long *)flags_ptr;
> + return format_page_flags(flags, buf, end);
> + case 'v':
> + flags = *(unsigned long *)flags_ptr;
> + return format_vma_flags(flags, buf, end);
> + case 'g':
> + gfp_flags = *(gfp_t *)flags_ptr;
> + return format_gfp_flags(gfp_flags, buf, end);
> + default:
> + WARN_ONCE(1, "Unsupported flags modifier: %c\n", fmt[1]);
> + return 0;
> + }
> +}
> +
> int kptr_restrict __read_mostly;
> 
> /*
> @@ -1448,6 +1472,11 @@ int kptr_restrict __read_mostly;
>  * - 'Cn' For a clock, it prints the name (Common Clock Framework) or address
>  *(legacy 

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

[Vlastimil Babka]

On 12/02/2015 12:01 PM, Rasmus Villemoes wrote:
> On Mon, Nov 30 2015, Vlastimil Babka  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:
>> +
>> +%pgp0x1f886c(referenced|uptodate|lru|active|private)
>> +%pgg0x24202c4(GFP_USER|GFP_DMA32|GFP_NOWARN)
>> +%pgv0x875(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, )

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. 

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

[Vlastimil Babka]

On 12/02/2015 06:40 PM, yalin wang wrote:

(please trim your reply next time, no need to quote whole patch here)

> i am thinking why not make %pg* to be more generic ?
> not restricted to only GFP / vma flags / page flags .
> so could we change format like this ?
> define a flag spec struct to include flag and trace_print_flags and some 
> other option :
> typedef struct { 
> unsigned long flag;
> struct trace_print_flags *flags;
> unsigned long option; } flag_sec;
> flag_sec my_flag;
> in printk we only pass like this :
> printk(“%pg\n”, _flag) ;
> then it can print any flags defined by user .
> more useful for other drivers to use .

I don't know, it sounds quite complicated given that we had no flags printing
for years and now there's just three kinds of them. The extra struct flag_sec is
IMHO nuissance. No other printk format needs such thing AFAIK? For example, if I
were to print page flags from several places, each would have to define the
struct flag_sec instance, or some header would have to provide it?

I could maybe accept passing a flag value and trace_print_flags * as two
separate parameters, but I guess that breaks an ancient invariant of one
parameter per format string...

> Thanks
> 
> 
> 
> 
> 
> 
> 
> 
> 
>  
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[yalin wang]

> On Dec 2, 2015, at 13:04, Vlastimil Babka  wrote:
> 
> On 12/02/2015 06:40 PM, yalin wang wrote:
> 
> (please trim your reply next time, no need to quote whole patch here)
> 
>> i am thinking why not make %pg* to be more generic ?
>> not restricted to only GFP / vma flags / page flags .
>> so could we change format like this ?
>> define a flag spec struct to include flag and trace_print_flags and some 
>> other option :
>> typedef struct { 
>> unsigned long flag;
>> structtrace_print_flags *flags;
>> unsigned long option; } flag_sec;
>> flag_sec my_flag;
>> in printk we only pass like this :
>> printk(“%pg\n”, _flag) ;
>> then it can print any flags defined by user .
>> more useful for other drivers to use .
> 
> I don't know, it sounds quite complicated given that we had no flags printing
> for years and now there's just three kinds of them. The extra struct flag_sec 
> is
> IMHO nuissance. No other printk format needs such thing AFAIK? For example, 
> if I
> were to print page flags from several places, each would have to define the
> struct flag_sec instance, or some header would have to provide it?
this can be avoided by provide a macro in header file .
we can add a new struct to declare trace_print_flags :
for example:
#define DECLARE_FLAG_PRINTK_FMT(name, flags_array)   flag_spec name = { .flags 
= flags_array};
#define FLAG_PRINTK_FMT(name, flag) ({  name.flag = flag;  })

in source code :
DECLARE_FLAG_PRINTK_FMT(my_flag, vmaflags_names);
printk(“%pg\n”, FLAG_PRINTK_FMT(my_flag, vma->flag));

i am not if DECLARE_FLAG_PRINTK_FMT and FLAG_PRINTK_FMT macro 
can be defined into one macro ?
maybe need some trick here .

is it possible ?


Thanks



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Rasmus Villemoes]

On Mon, Nov 30 2015, Vlastimil Babka  wrote:

> In mm we use several kinds of flags bitfields that are sometimes printed for
> debugging purposes, or exported to userspace via sysfs. To make them easier to
> interpret independently on kernel version and config, we want to dump also the
> symbolic flag names. So far this has been done with repeated calls to
> pr_cont(), which is unreliable on SMP, and not usable for e.g. sysfs export.
>
> To get a more reliable and universal solution, this patch extends printk()
> format string for pointers to handle the page flags (%pgp), gfp_flags (%pgg)
> and vma flags (%pgv). Existing users of dump_flag_names() are converted and
> simplified.
>
> It would be possible to pass flags by value instead of pointer, but the %p
> format string for pointers already has extensions for various kernel
> structures, so it's a good fit, and the extra indirection in a non-critical
> path is negligible.
>
> Signed-off-by: Vlastimil Babka 
> Cc: Rasmus Villemoes 
> ---
> I'm sending it on top of the page_owner series, as it's already in mmotm.
> But to reduce churn (in case this approach is accepted), I can later
> incorporate it and resend it whole.
>
>  Documentation/printk-formats.txt |  14 
>  include/linux/mmdebug.h  |   5 +-
>  lib/vsprintf.c   |  31 
>  mm/debug.c   | 150 
> ++-
>  mm/oom_kill.c|   5 +-
>  mm/page_alloc.c  |   5 +-
>  mm/page_owner.c  |   5 +-
>  7 files changed, 140 insertions(+), 75 deletions(-)

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:
> +
> + %pgp0x1f886c(referenced|uptodate|lru|active|private)
> + %pgg0x24202c4(GFP_USER|GFP_DMA32|GFP_NOWARN)
> + %pgv0x875(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, )


> + 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:
>  
>   %pNF0xc000
> diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
> index 3b77fab7ad28..e6518df259ca 100644
> --- a/include/linux/mmdebug.h
> +++ b/include/linux/mmdebug.h
> @@ -2,6 +2,7 @@
>  #define LINUX_MM_DEBUG_H 1
>  
>  #include 
> +#include 
>  
>  struct page;
>  struct vm_area_struct;
> @@ -10,7 +11,9 @@ struct mm_struct;
>  extern void dump_page(struct page *page, const char *reason);
>  extern void dump_page_badflags(struct page *page, const char *reason,
>  unsigned long badflags);
> -extern void dump_gfpflag_names(unsigned long gfp_flags);
> +extern char *format_page_flags(unsigned long flags, char *buf, char *end);
> +extern char *format_vma_flags(unsigned long flags, char *buf, char *end);
> +extern char *format_gfp_flags(gfp_t gfp_flags, char *buf, char*end);
>  void dump_vma(const struct vm_area_struct *vma);
>  void dump_mm(const struct mm_struct *mm);
>  
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index f9cee8e1233c..41cd122bd307 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -31,6 +31,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include /* for PAGE_SIZE */
>  #include /* for dereference_function_descriptor() */
> @@ -1361,6 +1362,29 @@ char *clock(char *buf, char *end, struct clk *clk, 
> struct printf_spec spec,
>   }
>  }
>  
> +static noinline_for_stack
> +char *flags_string(char *buf, char *end, void *flags_ptr,
> + struct printf_spec spec, const char *fmt)
> +{
> + unsigned long flags;
> + gfp_t gfp_flags;
> +
> + switch (fmt[1]) {
> + case 'p':
> + 

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

[Vlastimil Babka]

In mm we use several kinds of flags bitfields that are sometimes printed for
debugging purposes, or exported to userspace via sysfs. To make them easier to
interpret independently on kernel version and config, we want to dump also the
symbolic flag names. So far this has been done with repeated calls to
pr_cont(), which is unreliable on SMP, and not usable for e.g. sysfs export.

To get a more reliable and universal solution, this patch extends printk()
format string for pointers to handle the page flags (%pgp), gfp_flags (%pgg)
and vma flags (%pgv). Existing users of dump_flag_names() are converted and
simplified.

It would be possible to pass flags by value instead of pointer, but the %p
format string for pointers already has extensions for various kernel
structures, so it's a good fit, and the extra indirection in a non-critical
path is negligible.

Signed-off-by: Vlastimil Babka 
Cc: Rasmus Villemoes 
---
I'm sending it on top of the page_owner series, as it's already in mmotm.
But to reduce churn (in case this approach is accepted), I can later
incorporate it and resend it whole.

 Documentation/printk-formats.txt |  14 
 include/linux/mmdebug.h  |   5 +-
 lib/vsprintf.c   |  31 
 mm/debug.c   | 150 ++-
 mm/oom_kill.c|   5 +-
 mm/page_alloc.c  |   5 +-
 mm/page_owner.c  |   5 +-
 7 files changed, 140 insertions(+), 75 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index b784c270105f..4b5156e74b09 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -292,6 +292,20 @@ Raw pointer value SHOULD be printed with %p. The kernel 
supports
 
Passed by reference.
 
+Flags bitfields such as page flags, gfp_flags:
+
+   %pgp0x1f886c(referenced|uptodate|lru|active|private)
+   %pgg0x24202c4(GFP_USER|GFP_DMA32|GFP_NOWARN)
+   %pgv0x875(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:
 
%pNF0xc000
diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index 3b77fab7ad28..e6518df259ca 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -2,6 +2,7 @@
 #define LINUX_MM_DEBUG_H 1
 
 #include 
+#include 
 
 struct page;
 struct vm_area_struct;
@@ -10,7 +11,9 @@ struct mm_struct;
 extern void dump_page(struct page *page, const char *reason);
 extern void dump_page_badflags(struct page *page, const char *reason,
   unsigned long badflags);
-extern void dump_gfpflag_names(unsigned long gfp_flags);
+extern char *format_page_flags(unsigned long flags, char *buf, char *end);
+extern char *format_vma_flags(unsigned long flags, char *buf, char *end);
+extern char *format_gfp_flags(gfp_t gfp_flags, char *buf, char*end);
 void dump_vma(const struct vm_area_struct *vma);
 void dump_mm(const struct mm_struct *mm);
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index f9cee8e1233c..41cd122bd307 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include   /* for PAGE_SIZE */
 #include   /* for dereference_function_descriptor() */
@@ -1361,6 +1362,29 @@ char *clock(char *buf, char *end, struct clk *clk, 
struct printf_spec spec,
}
 }
 
+static noinline_for_stack
+char *flags_string(char *buf, char *end, void *flags_ptr,
+   struct printf_spec spec, const char *fmt)
+{
+   unsigned long flags;
+   gfp_t gfp_flags;
+
+   switch (fmt[1]) {
+   case 'p':
+   flags = *(unsigned long *)flags_ptr;
+   return format_page_flags(flags, buf, end);
+   case 'v':
+   flags = *(unsigned long *)flags_ptr;
+   return format_vma_flags(flags, buf, end);
+   case 'g':
+   gfp_flags = *(gfp_t *)flags_ptr;
+   return format_gfp_flags(gfp_flags, buf, end);
+   default:
+   WARN_ONCE(1, "Unsupported flags modifier: %c\n", fmt[1]);
+   return 0;
+   }
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1448,6 +1472,11 @@ int kptr_restrict __read_mostly;
  * - 'Cn' For a clock, it prints the name (Common Clock Framework) or address
  *(legacy clock framework) of the clock
  * - 'Cr' For a clock, it prints the current rate of the clock
+ * - 'g' For flags to be printed as a collection of symbolic strings that would
+ *   construct the specific value. Supported flags given by option:
+ *   p page flags (see 

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

[Vlastimil Babka]

In mm we use several kinds of flags bitfields that are sometimes printed for
debugging purposes, or exported to userspace via sysfs. To make them easier to
interpret independently on kernel version and config, we want to dump also the
symbolic flag names. So far this has been done with repeated calls to
pr_cont(), which is unreliable on SMP, and not usable for e.g. sysfs export.

To get a more reliable and universal solution, this patch extends printk()
format string for pointers to handle the page flags (%pgp), gfp_flags (%pgg)
and vma flags (%pgv). Existing users of dump_flag_names() are converted and
simplified.

It would be possible to pass flags by value instead of pointer, but the %p
format string for pointers already has extensions for various kernel
structures, so it's a good fit, and the extra indirection in a non-critical
path is negligible.

Signed-off-by: Vlastimil Babka 
Cc: Rasmus Villemoes 
---
I'm sending it on top of the page_owner series, as it's already in mmotm.
But to reduce churn (in case this approach is accepted), I can later
incorporate it and resend it whole.

 Documentation/printk-formats.txt |  14 
 include/linux/mmdebug.h  |   5 +-
 lib/vsprintf.c   |  31 
 mm/debug.c   | 150 ++-
 mm/oom_kill.c|   5 +-
 mm/page_alloc.c  |   5 +-
 mm/page_owner.c  |   5 +-
 7 files changed, 140 insertions(+), 75 deletions(-)

diff --git a/Documentation/printk-formats.txt b/Documentation/printk-formats.txt
index b784c270105f..4b5156e74b09 100644
--- a/Documentation/printk-formats.txt
+++ b/Documentation/printk-formats.txt
@@ -292,6 +292,20 @@ Raw pointer value SHOULD be printed with %p. The kernel 
supports
 
Passed by reference.
 
+Flags bitfields such as page flags, gfp_flags:
+
+   %pgp0x1f886c(referenced|uptodate|lru|active|private)
+   %pgg0x24202c4(GFP_USER|GFP_DMA32|GFP_NOWARN)
+   %pgv0x875(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:
 
%pNF0xc000
diff --git a/include/linux/mmdebug.h b/include/linux/mmdebug.h
index 3b77fab7ad28..e6518df259ca 100644
--- a/include/linux/mmdebug.h
+++ b/include/linux/mmdebug.h
@@ -2,6 +2,7 @@
 #define LINUX_MM_DEBUG_H 1
 
 #include 
+#include 
 
 struct page;
 struct vm_area_struct;
@@ -10,7 +11,9 @@ struct mm_struct;
 extern void dump_page(struct page *page, const char *reason);
 extern void dump_page_badflags(struct page *page, const char *reason,
   unsigned long badflags);
-extern void dump_gfpflag_names(unsigned long gfp_flags);
+extern char *format_page_flags(unsigned long flags, char *buf, char *end);
+extern char *format_vma_flags(unsigned long flags, char *buf, char *end);
+extern char *format_gfp_flags(gfp_t gfp_flags, char *buf, char*end);
 void dump_vma(const struct vm_area_struct *vma);
 void dump_mm(const struct mm_struct *mm);
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index f9cee8e1233c..41cd122bd307 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -31,6 +31,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include   /* for PAGE_SIZE */
 #include   /* for dereference_function_descriptor() */
@@ -1361,6 +1362,29 @@ char *clock(char *buf, char *end, struct clk *clk, 
struct printf_spec spec,
}
 }
 
+static noinline_for_stack
+char *flags_string(char *buf, char *end, void *flags_ptr,
+   struct printf_spec spec, const char *fmt)
+{
+   unsigned long flags;
+   gfp_t gfp_flags;
+
+   switch (fmt[1]) {
+   case 'p':
+   flags = *(unsigned long *)flags_ptr;
+   return format_page_flags(flags, buf, end);
+   case 'v':
+   flags = *(unsigned long *)flags_ptr;
+   return format_vma_flags(flags, buf, end);
+   case 'g':
+   gfp_flags = *(gfp_t *)flags_ptr;
+   return format_gfp_flags(gfp_flags, buf, end);
+   default:
+   WARN_ONCE(1, "Unsupported flags modifier: %c\n", fmt[1]);
+   return 0;
+   }
+}
+
 int kptr_restrict __read_mostly;
 
 /*
@@ -1448,6 +1472,11 @@ int kptr_restrict __read_mostly;
  * - 'Cn' For a clock, it prints the name (Common Clock Framework) or address
  *(legacy clock framework) of the clock
  * - 'Cr' For a clock, it prints the current rate of the clock
+ * - 'g' For flags to be printed as a collection of symbolic strings that would
+ *   construct the specific value. Supported flags