* [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc @ 2017-11-06 18:02 ` Joe Perches 0 siblings, 0 replies; 18+ messages in thread From: Joe Perches @ 2017-11-06 18:02 UTC (permalink / raw) To: linux-kernel; +Cc: linux-mm KERN_CONT/pr_cont uses should be avoided where possible. Use single pr_warn calls instead. Signed-off-by: Joe Perches <joe@perches.com> --- mm/page_alloc.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 536431bf0f0c..82e6d2c914ab 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -3275,19 +3275,17 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...) if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs)) return; - pr_warn("%s: ", current->comm); - va_start(args, fmt); vaf.fmt = fmt; vaf.va = &args; - pr_cont("%pV", &vaf); - va_end(args); - - pr_cont(", mode:%#x(%pGg), nodemask=", gfp_mask, &gfp_mask); if (nodemask) - pr_cont("%*pbl\n", nodemask_pr_args(nodemask)); + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=%*pbl\n", + current->comm, &vaf, gfp_mask, &gfp_mask, + nodemask_pr_args(nodemask)); else - pr_cont("(null)\n"); + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=(null)\n", + current->comm, &vaf, gfp_mask, &gfp_mask); + va_end(args); cpuset_print_current_mems_allowed(); -- 2.15.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc @ 2017-11-06 18:02 ` Joe Perches 0 siblings, 0 replies; 18+ messages in thread From: Joe Perches @ 2017-11-06 18:02 UTC (permalink / raw) To: linux-kernel; +Cc: linux-mm KERN_CONT/pr_cont uses should be avoided where possible. Use single pr_warn calls instead. Signed-off-by: Joe Perches <joe@perches.com> --- mm/page_alloc.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 536431bf0f0c..82e6d2c914ab 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -3275,19 +3275,17 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...) if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs)) return; - pr_warn("%s: ", current->comm); - va_start(args, fmt); vaf.fmt = fmt; vaf.va = &args; - pr_cont("%pV", &vaf); - va_end(args); - - pr_cont(", mode:%#x(%pGg), nodemask=", gfp_mask, &gfp_mask); if (nodemask) - pr_cont("%*pbl\n", nodemask_pr_args(nodemask)); + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=%*pbl\n", + current->comm, &vaf, gfp_mask, &gfp_mask, + nodemask_pr_args(nodemask)); else - pr_cont("(null)\n"); + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=(null)\n", + current->comm, &vaf, gfp_mask, &gfp_mask); + va_end(args); cpuset_print_current_mems_allowed(); -- 2.15.0 -- 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> ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc 2017-11-06 18:02 ` Joe Perches @ 2017-11-07 12:50 ` Michal Hocko -1 siblings, 0 replies; 18+ messages in thread From: Michal Hocko @ 2017-11-07 12:50 UTC (permalink / raw) To: Joe Perches; +Cc: linux-kernel, linux-mm On Mon 06-11-17 10:02:56, Joe Perches wrote: > KERN_CONT/pr_cont uses should be avoided where possible. > Use single pr_warn calls instead. > > Signed-off-by: Joe Perches <joe@perches.com> > --- > mm/page_alloc.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 536431bf0f0c..82e6d2c914ab 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -3275,19 +3275,17 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...) > if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs)) > return; > > - pr_warn("%s: ", current->comm); > - > va_start(args, fmt); > vaf.fmt = fmt; > vaf.va = &args; > - pr_cont("%pV", &vaf); > - va_end(args); > - > - pr_cont(", mode:%#x(%pGg), nodemask=", gfp_mask, &gfp_mask); > if (nodemask) > - pr_cont("%*pbl\n", nodemask_pr_args(nodemask)); > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=%*pbl\n", > + current->comm, &vaf, gfp_mask, &gfp_mask, > + nodemask_pr_args(nodemask)); > else > - pr_cont("(null)\n"); > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=(null)\n", > + current->comm, &vaf, gfp_mask, &gfp_mask); > + va_end(args); > > cpuset_print_current_mems_allowed(); I do not like the duplication. It just calls for inconsistencies over time. Can we instead make %*pbl consume NULL nodemask instead? Something like the following pseudo patch + the if/else removed. If this would be possible we could simplify other code as well I think (at least oom code has to special case NULL nodemask). What do you think? --- diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h index de1c50b93c61..106fac744f49 100644 --- a/include/linux/nodemask.h +++ b/include/linux/nodemask.h @@ -104,7 +104,7 @@ extern nodemask_t _unused_nodemask_arg_; * * Can be used to provide arguments for '%*pb[l]' when printing a nodemask. */ -#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp)->bits +#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp) ? (maskp)->bits : NULL /* * The inline keyword gives the compiler room to decide to inline, or diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 1746bae94d41..6f40cf319a76 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -902,6 +902,9 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap, int cur, rbot, rtop; bool first = true; + if (!bitmap) + return buf; + /* reused to print numbers */ spec = (struct printf_spec){ .base = 10 }; -- Michal Hocko SUSE Labs ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc @ 2017-11-07 12:50 ` Michal Hocko 0 siblings, 0 replies; 18+ messages in thread From: Michal Hocko @ 2017-11-07 12:50 UTC (permalink / raw) To: Joe Perches; +Cc: linux-kernel, linux-mm On Mon 06-11-17 10:02:56, Joe Perches wrote: > KERN_CONT/pr_cont uses should be avoided where possible. > Use single pr_warn calls instead. > > Signed-off-by: Joe Perches <joe@perches.com> > --- > mm/page_alloc.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 536431bf0f0c..82e6d2c914ab 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -3275,19 +3275,17 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...) > if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs)) > return; > > - pr_warn("%s: ", current->comm); > - > va_start(args, fmt); > vaf.fmt = fmt; > vaf.va = &args; > - pr_cont("%pV", &vaf); > - va_end(args); > - > - pr_cont(", mode:%#x(%pGg), nodemask=", gfp_mask, &gfp_mask); > if (nodemask) > - pr_cont("%*pbl\n", nodemask_pr_args(nodemask)); > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=%*pbl\n", > + current->comm, &vaf, gfp_mask, &gfp_mask, > + nodemask_pr_args(nodemask)); > else > - pr_cont("(null)\n"); > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=(null)\n", > + current->comm, &vaf, gfp_mask, &gfp_mask); > + va_end(args); > > cpuset_print_current_mems_allowed(); I do not like the duplication. It just calls for inconsistencies over time. Can we instead make %*pbl consume NULL nodemask instead? Something like the following pseudo patch + the if/else removed. If this would be possible we could simplify other code as well I think (at least oom code has to special case NULL nodemask). What do you think? --- diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h index de1c50b93c61..106fac744f49 100644 --- a/include/linux/nodemask.h +++ b/include/linux/nodemask.h @@ -104,7 +104,7 @@ extern nodemask_t _unused_nodemask_arg_; * * Can be used to provide arguments for '%*pb[l]' when printing a nodemask. */ -#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp)->bits +#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp) ? (maskp)->bits : NULL /* * The inline keyword gives the compiler room to decide to inline, or diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 1746bae94d41..6f40cf319a76 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -902,6 +902,9 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap, int cur, rbot, rtop; bool first = true; + if (!bitmap) + return buf; + /* reused to print numbers */ spec = (struct printf_spec){ .base = 10 }; -- Michal Hocko SUSE Labs -- 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> ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc 2017-11-07 12:50 ` Michal Hocko @ 2017-11-07 15:34 ` Joe Perches -1 siblings, 0 replies; 18+ messages in thread From: Joe Perches @ 2017-11-07 15:34 UTC (permalink / raw) To: Michal Hocko; +Cc: linux-kernel, linux-mm On Tue, 2017-11-07 at 13:50 +0100, Michal Hocko wrote: > On Mon 06-11-17 10:02:56, Joe Perches wrote: > > KERN_CONT/pr_cont uses should be avoided where possible. > > Use single pr_warn calls instead. [] > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c [] > > @@ -3275,19 +3275,17 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...) > > if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs)) > > return; > > > > - pr_warn("%s: ", current->comm); > > - > > va_start(args, fmt); > > vaf.fmt = fmt; > > vaf.va = &args; > > - pr_cont("%pV", &vaf); > > - va_end(args); > > - > > - pr_cont(", mode:%#x(%pGg), nodemask=", gfp_mask, &gfp_mask); > > if (nodemask) > > - pr_cont("%*pbl\n", nodemask_pr_args(nodemask)); > > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=%*pbl\n", > > + current->comm, &vaf, gfp_mask, &gfp_mask, > > + nodemask_pr_args(nodemask)); > > else > > - pr_cont("(null)\n"); > > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=(null)\n", > > + current->comm, &vaf, gfp_mask, &gfp_mask); > > + va_end(args); > > > > cpuset_print_current_mems_allowed(); > > I do not like the duplication. It just calls for inconsistencies over > time. Can we instead make %*pbl consume NULL nodemask instead? > Something like the following pseudo patch + the if/else removed. > If this would be possible we could simplify other code as well I think > (at least oom code has to special case NULL nodemask). > > What do you think? I think it would be fine to have a single pr_warn. > --- > diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h [] > @@ -104,7 +104,7 @@ extern nodemask_t _unused_nodemask_arg_; > * > * Can be used to provide arguments for '%*pb[l]' when printing a nodemask. > */ > -#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp)->bits > +#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp) ? (maskp)->bits : NULL > > /* > * The inline keyword gives the compiler room to decide to inline, or > diff --git a/lib/vsprintf.c b/lib/vsprintf.c [] > @@ -902,6 +902,9 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap, > int cur, rbot, rtop; > bool first = true; > > + if (!bitmap) > + return buf; I believe this is not necessary as any NULL pointer argument passed to lib/vsprintf.c:pointer() (any %p<foo>) emits "[2 or 10 spaces](null)" on 32bit or 64 bit systems. I believe, but have not tested, that using a specific width as an argument to %*pb[l] will constrain the number of spaces before the '(null)' output in any NULL pointer use. So how about a #define like /* * nodemask_pr_args is only used with a "%*pb[l]" format for a nodemask. * A NULL nodemask uses 6 to emit "(null)" without leading spaces. */ #define nodemask_pr_args(maskp) \ (maskp) ? MAX_NUMNODES : 6, \ (maskp) ? (maskp)->bits : NULL ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc @ 2017-11-07 15:34 ` Joe Perches 0 siblings, 0 replies; 18+ messages in thread From: Joe Perches @ 2017-11-07 15:34 UTC (permalink / raw) To: Michal Hocko; +Cc: linux-kernel, linux-mm On Tue, 2017-11-07 at 13:50 +0100, Michal Hocko wrote: > On Mon 06-11-17 10:02:56, Joe Perches wrote: > > KERN_CONT/pr_cont uses should be avoided where possible. > > Use single pr_warn calls instead. [] > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c [] > > @@ -3275,19 +3275,17 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...) > > if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs)) > > return; > > > > - pr_warn("%s: ", current->comm); > > - > > va_start(args, fmt); > > vaf.fmt = fmt; > > vaf.va = &args; > > - pr_cont("%pV", &vaf); > > - va_end(args); > > - > > - pr_cont(", mode:%#x(%pGg), nodemask=", gfp_mask, &gfp_mask); > > if (nodemask) > > - pr_cont("%*pbl\n", nodemask_pr_args(nodemask)); > > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=%*pbl\n", > > + current->comm, &vaf, gfp_mask, &gfp_mask, > > + nodemask_pr_args(nodemask)); > > else > > - pr_cont("(null)\n"); > > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=(null)\n", > > + current->comm, &vaf, gfp_mask, &gfp_mask); > > + va_end(args); > > > > cpuset_print_current_mems_allowed(); > > I do not like the duplication. It just calls for inconsistencies over > time. Can we instead make %*pbl consume NULL nodemask instead? > Something like the following pseudo patch + the if/else removed. > If this would be possible we could simplify other code as well I think > (at least oom code has to special case NULL nodemask). > > What do you think? I think it would be fine to have a single pr_warn. > --- > diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h [] > @@ -104,7 +104,7 @@ extern nodemask_t _unused_nodemask_arg_; > * > * Can be used to provide arguments for '%*pb[l]' when printing a nodemask. > */ > -#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp)->bits > +#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp) ? (maskp)->bits : NULL > > /* > * The inline keyword gives the compiler room to decide to inline, or > diff --git a/lib/vsprintf.c b/lib/vsprintf.c [] > @@ -902,6 +902,9 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap, > int cur, rbot, rtop; > bool first = true; > > + if (!bitmap) > + return buf; I believe this is not necessary as any NULL pointer argument passed to lib/vsprintf.c:pointer() (any %p<foo>) emits "[2 or 10 spaces](null)" on 32bit or 64 bit systems. I believe, but have not tested, that using a specific width as an argument to %*pb[l] will constrain the number of spaces before the '(null)' output in any NULL pointer use. So how about a #define like /* * nodemask_pr_args is only used with a "%*pb[l]" format for a nodemask. * A NULL nodemask uses 6 to emit "(null)" without leading spaces. */ #define nodemask_pr_args(maskp) \ (maskp) ? MAX_NUMNODES : 6, \ (maskp) ? (maskp)->bits : NULL -- 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> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc 2017-11-07 15:34 ` Joe Perches @ 2017-11-07 15:43 ` Michal Hocko -1 siblings, 0 replies; 18+ messages in thread From: Michal Hocko @ 2017-11-07 15:43 UTC (permalink / raw) To: Joe Perches; +Cc: linux-kernel, linux-mm On Tue 07-11-17 07:34:25, Joe Perches wrote: > On Tue, 2017-11-07 at 13:50 +0100, Michal Hocko wrote: > > On Mon 06-11-17 10:02:56, Joe Perches wrote: > > > KERN_CONT/pr_cont uses should be avoided where possible. > > > Use single pr_warn calls instead. > [] > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > [] > > > @@ -3275,19 +3275,17 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...) > > > if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs)) > > > return; > > > > > > - pr_warn("%s: ", current->comm); > > > - > > > va_start(args, fmt); > > > vaf.fmt = fmt; > > > vaf.va = &args; > > > - pr_cont("%pV", &vaf); > > > - va_end(args); > > > - > > > - pr_cont(", mode:%#x(%pGg), nodemask=", gfp_mask, &gfp_mask); > > > if (nodemask) > > > - pr_cont("%*pbl\n", nodemask_pr_args(nodemask)); > > > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=%*pbl\n", > > > + current->comm, &vaf, gfp_mask, &gfp_mask, > > > + nodemask_pr_args(nodemask)); > > > else > > > - pr_cont("(null)\n"); > > > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=(null)\n", > > > + current->comm, &vaf, gfp_mask, &gfp_mask); > > > + va_end(args); > > > > > > cpuset_print_current_mems_allowed(); > > > > I do not like the duplication. It just calls for inconsistencies over > > time. Can we instead make %*pbl consume NULL nodemask instead? > > Something like the following pseudo patch + the if/else removed. > > If this would be possible we could simplify other code as well I think > > (at least oom code has to special case NULL nodemask). > > > > What do you think? > > I think it would be fine to have a single pr_warn. > > > --- > > diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h > [] > > @@ -104,7 +104,7 @@ extern nodemask_t _unused_nodemask_arg_; > > * > > * Can be used to provide arguments for '%*pb[l]' when printing a nodemask. > > */ > > -#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp)->bits > > +#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp) ? (maskp)->bits : NULL > > > > /* > > * The inline keyword gives the compiler room to decide to inline, or > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > [] > > @@ -902,6 +902,9 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap, > > int cur, rbot, rtop; > > bool first = true; > > > > + if (!bitmap) > > + return buf; > > I believe this is not necessary as any NULL pointer argument > passed to lib/vsprintf.c:pointer() (any %p<foo>) emits > "[2 or 10 spaces](null)" on 32bit or 64 bit systems. OK, I see if (!ptr && *fmt != 'K') { /* * Print (null) with the same width as a pointer so it makes * tabular output look nice. */ if (spec.field_width == -1) spec.field_width = default_width; return string(buf, end, "(null)", spec); } > I believe, but have not tested, that using a specific width > as an argument to %*pb[l] will constrain the number of > spaces before the '(null)' output in any NULL pointer use. > > So how about a #define like > > /* > * nodemask_pr_args is only used with a "%*pb[l]" format for a nodemask. > * A NULL nodemask uses 6 to emit "(null)" without leading spaces. > */ > #define nodemask_pr_args(maskp) \ > (maskp) ? MAX_NUMNODES : 6, \ > (maskp) ? (maskp)->bits : NULL Why not -1 then? -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc @ 2017-11-07 15:43 ` Michal Hocko 0 siblings, 0 replies; 18+ messages in thread From: Michal Hocko @ 2017-11-07 15:43 UTC (permalink / raw) To: Joe Perches; +Cc: linux-kernel, linux-mm On Tue 07-11-17 07:34:25, Joe Perches wrote: > On Tue, 2017-11-07 at 13:50 +0100, Michal Hocko wrote: > > On Mon 06-11-17 10:02:56, Joe Perches wrote: > > > KERN_CONT/pr_cont uses should be avoided where possible. > > > Use single pr_warn calls instead. > [] > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > [] > > > @@ -3275,19 +3275,17 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...) > > > if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs)) > > > return; > > > > > > - pr_warn("%s: ", current->comm); > > > - > > > va_start(args, fmt); > > > vaf.fmt = fmt; > > > vaf.va = &args; > > > - pr_cont("%pV", &vaf); > > > - va_end(args); > > > - > > > - pr_cont(", mode:%#x(%pGg), nodemask=", gfp_mask, &gfp_mask); > > > if (nodemask) > > > - pr_cont("%*pbl\n", nodemask_pr_args(nodemask)); > > > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=%*pbl\n", > > > + current->comm, &vaf, gfp_mask, &gfp_mask, > > > + nodemask_pr_args(nodemask)); > > > else > > > - pr_cont("(null)\n"); > > > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=(null)\n", > > > + current->comm, &vaf, gfp_mask, &gfp_mask); > > > + va_end(args); > > > > > > cpuset_print_current_mems_allowed(); > > > > I do not like the duplication. It just calls for inconsistencies over > > time. Can we instead make %*pbl consume NULL nodemask instead? > > Something like the following pseudo patch + the if/else removed. > > If this would be possible we could simplify other code as well I think > > (at least oom code has to special case NULL nodemask). > > > > What do you think? > > I think it would be fine to have a single pr_warn. > > > --- > > diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h > [] > > @@ -104,7 +104,7 @@ extern nodemask_t _unused_nodemask_arg_; > > * > > * Can be used to provide arguments for '%*pb[l]' when printing a nodemask. > > */ > > -#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp)->bits > > +#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp) ? (maskp)->bits : NULL > > > > /* > > * The inline keyword gives the compiler room to decide to inline, or > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > [] > > @@ -902,6 +902,9 @@ char *bitmap_list_string(char *buf, char *end, unsigned long *bitmap, > > int cur, rbot, rtop; > > bool first = true; > > > > + if (!bitmap) > > + return buf; > > I believe this is not necessary as any NULL pointer argument > passed to lib/vsprintf.c:pointer() (any %p<foo>) emits > "[2 or 10 spaces](null)" on 32bit or 64 bit systems. OK, I see if (!ptr && *fmt != 'K') { /* * Print (null) with the same width as a pointer so it makes * tabular output look nice. */ if (spec.field_width == -1) spec.field_width = default_width; return string(buf, end, "(null)", spec); } > I believe, but have not tested, that using a specific width > as an argument to %*pb[l] will constrain the number of > spaces before the '(null)' output in any NULL pointer use. > > So how about a #define like > > /* > * nodemask_pr_args is only used with a "%*pb[l]" format for a nodemask. > * A NULL nodemask uses 6 to emit "(null)" without leading spaces. > */ > #define nodemask_pr_args(maskp) \ > (maskp) ? MAX_NUMNODES : 6, \ > (maskp) ? (maskp)->bits : NULL Why not -1 then? -- Michal Hocko SUSE Labs -- 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> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc 2017-11-07 15:43 ` Michal Hocko @ 2017-11-07 16:03 ` Joe Perches -1 siblings, 0 replies; 18+ messages in thread From: Joe Perches @ 2017-11-07 16:03 UTC (permalink / raw) To: Michal Hocko; +Cc: linux-kernel, linux-mm On Tue, 2017-11-07 at 16:43 +0100, Michal Hocko wrote: > On Tue 07-11-17 07:34:25, Joe Perches wrote: [] > > I believe, but have not tested, that using a specific width > > as an argument to %*pb[l] will constrain the number of > > spaces before the '(null)' output in any NULL pointer use. > > > > So how about a #define like > > > > /* > > * nodemask_pr_args is only used with a "%*pb[l]" format for a nodemask. > > * A NULL nodemask uses 6 to emit "(null)" without leading spaces. > > */ > > #define nodemask_pr_args(maskp) \ > > (maskp) ? MAX_NUMNODES : 6, \ > > (maskp) ? (maskp)->bits : NULL > > Why not -1 then? I believe it's the field width and not the precision that needs to be set. But if you test it and it works, then that's fine by me. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc @ 2017-11-07 16:03 ` Joe Perches 0 siblings, 0 replies; 18+ messages in thread From: Joe Perches @ 2017-11-07 16:03 UTC (permalink / raw) To: Michal Hocko; +Cc: linux-kernel, linux-mm On Tue, 2017-11-07 at 16:43 +0100, Michal Hocko wrote: > On Tue 07-11-17 07:34:25, Joe Perches wrote: [] > > I believe, but have not tested, that using a specific width > > as an argument to %*pb[l] will constrain the number of > > spaces before the '(null)' output in any NULL pointer use. > > > > So how about a #define like > > > > /* > > * nodemask_pr_args is only used with a "%*pb[l]" format for a nodemask. > > * A NULL nodemask uses 6 to emit "(null)" without leading spaces. > > */ > > #define nodemask_pr_args(maskp) \ > > (maskp) ? MAX_NUMNODES : 6, \ > > (maskp) ? (maskp)->bits : NULL > > Why not -1 then? I believe it's the field width and not the precision that needs to be set. But if you test it and it works, then that's fine by me. -- 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> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc 2017-11-07 16:03 ` Joe Perches @ 2017-11-09 10:05 ` Michal Hocko -1 siblings, 0 replies; 18+ messages in thread From: Michal Hocko @ 2017-11-09 10:05 UTC (permalink / raw) To: Joe Perches; +Cc: linux-kernel, linux-mm On Tue 07-11-17 08:03:27, Joe Perches wrote: > On Tue, 2017-11-07 at 16:43 +0100, Michal Hocko wrote: > > On Tue 07-11-17 07:34:25, Joe Perches wrote: > [] > > > I believe, but have not tested, that using a specific width > > > as an argument to %*pb[l] will constrain the number of > > > spaces before the '(null)' output in any NULL pointer use. > > > > > > So how about a #define like > > > > > > /* > > > * nodemask_pr_args is only used with a "%*pb[l]" format for a nodemask. > > > * A NULL nodemask uses 6 to emit "(null)" without leading spaces. > > > */ > > > #define nodemask_pr_args(maskp) \ > > > (maskp) ? MAX_NUMNODES : 6, \ > > > (maskp) ? (maskp)->bits : NULL > > > > Why not -1 then? > > I believe it's the field width and not the precision that > needs to be set. But the first of the two arguments is the field with specifier, not the precision. /me confused... Anyway, the following works as expected when printing the OOM report: [ 47.005321] mem_eater invoked oom-killer: gfp_mask=0x14280ca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO), nodemask=(null), order=0, oom_score_adj=0 [ 47.007183] mem_eater cpuset=/ mems_allowed=0-1 [ 47.007829] CPU: 3 PID: 3223 Comm: mem_eater Tainted: G W 4.13.0-pr1-dirty #11 I hope I haven't overlooked anything --- >From 35aa7742d35d29af88c66dd5e6f8f5d62215f5fd Mon Sep 17 00:00:00 2001 From: Michal Hocko <mhocko@suse.com> Date: Thu, 9 Nov 2017 10:58:41 +0100 Subject: [PATCH] mm: simplify nodemask printing alloc_warn and dump_header have to explicitly handle NULL nodemask which forces both paths to use pr_cont. We can do better. printk already handles NULL pointers properly so all what we need is to teach nodemask_pr_args to handle NULL nodemask carefully. This allows simplification of both alloc_warn and dump_header and get rid of pr_cont altogether. This patch has been motivated by patch from Joe Perches http://lkml.kernel.org/r/b31236dfe3fc924054fd7842bde678e71d193638.1509991345.git.joe@perches.com Signed-off-by: Michal Hocko <mhocko@suse.com> --- include/linux/nodemask.h | 2 +- mm/oom_kill.c | 12 ++++-------- mm/page_alloc.c | 12 +++--------- 3 files changed, 8 insertions(+), 18 deletions(-) diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h index cf0b91c3ec12..5d3cc67207ed 100644 --- a/include/linux/nodemask.h +++ b/include/linux/nodemask.h @@ -103,7 +103,7 @@ extern nodemask_t _unused_nodemask_arg_; * * Can be used to provide arguments for '%*pb[l]' when printing a nodemask. */ -#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp)->bits +#define nodemask_pr_args(maskp) (maskp) ? MAX_NUMNODES : 0, (maskp) ? (maskp)->bits : NULL /* * The inline keyword gives the compiler room to decide to inline, or diff --git a/mm/oom_kill.c b/mm/oom_kill.c index 8dd0e088189b..606213a81ceb 100644 --- a/mm/oom_kill.c +++ b/mm/oom_kill.c @@ -426,14 +426,10 @@ static void dump_tasks(struct mem_cgroup *memcg, const nodemask_t *nodemask) static void dump_header(struct oom_control *oc, struct task_struct *p) { - pr_warn("%s invoked oom-killer: gfp_mask=%#x(%pGg), nodemask=", - current->comm, oc->gfp_mask, &oc->gfp_mask); - if (oc->nodemask) - pr_cont("%*pbl", nodemask_pr_args(oc->nodemask)); - else - pr_cont("(null)"); - pr_cont(", order=%d, oom_score_adj=%hd\n", - oc->order, current->signal->oom_score_adj); + pr_warn("%s invoked oom-killer: gfp_mask=%#x(%pGg), nodemask=%*pbl, order=%d, oom_score_adj=%hd\n", + current->comm, oc->gfp_mask, &oc->gfp_mask, + nodemask_pr_args(oc->nodemask), oc->order, + current->signal->oom_score_adj); if (!IS_ENABLED(CONFIG_COMPACTION) && oc->order) pr_warn("COMPACTION is disabled!!!\n"); diff --git a/mm/page_alloc.c b/mm/page_alloc.c index d755434aee94..457e43ed4c10 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -3281,20 +3281,14 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...) if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs)) return; - pr_warn("%s: ", current->comm); - va_start(args, fmt); vaf.fmt = fmt; vaf.va = &args; - pr_cont("%pV", &vaf); + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=%*pbl\n", + current->comm, &vaf, gfp_mask, &gfp_mask, + nodemask_pr_args(nodemask)); va_end(args); - pr_cont(", mode:%#x(%pGg), nodemask=", gfp_mask, &gfp_mask); - if (nodemask) - pr_cont("%*pbl\n", nodemask_pr_args(nodemask)); - else - pr_cont("(null)\n"); - cpuset_print_current_mems_allowed(); dump_stack(); -- 2.14.2 -- Michal Hocko SUSE Labs ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc @ 2017-11-09 10:05 ` Michal Hocko 0 siblings, 0 replies; 18+ messages in thread From: Michal Hocko @ 2017-11-09 10:05 UTC (permalink / raw) To: Joe Perches; +Cc: linux-kernel, linux-mm On Tue 07-11-17 08:03:27, Joe Perches wrote: > On Tue, 2017-11-07 at 16:43 +0100, Michal Hocko wrote: > > On Tue 07-11-17 07:34:25, Joe Perches wrote: > [] > > > I believe, but have not tested, that using a specific width > > > as an argument to %*pb[l] will constrain the number of > > > spaces before the '(null)' output in any NULL pointer use. > > > > > > So how about a #define like > > > > > > /* > > > * nodemask_pr_args is only used with a "%*pb[l]" format for a nodemask. > > > * A NULL nodemask uses 6 to emit "(null)" without leading spaces. > > > */ > > > #define nodemask_pr_args(maskp) \ > > > (maskp) ? MAX_NUMNODES : 6, \ > > > (maskp) ? (maskp)->bits : NULL > > > > Why not -1 then? > > I believe it's the field width and not the precision that > needs to be set. But the first of the two arguments is the field with specifier, not the precision. /me confused... Anyway, the following works as expected when printing the OOM report: [ 47.005321] mem_eater invoked oom-killer: gfp_mask=0x14280ca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO), nodemask=(null), order=0, oom_score_adj=0 [ 47.007183] mem_eater cpuset=/ mems_allowed=0-1 [ 47.007829] CPU: 3 PID: 3223 Comm: mem_eater Tainted: G W 4.13.0-pr1-dirty #11 I hope I haven't overlooked anything --- ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc 2017-11-09 10:05 ` Michal Hocko @ 2017-11-09 15:49 ` Joe Perches -1 siblings, 0 replies; 18+ messages in thread From: Joe Perches @ 2017-11-09 15:49 UTC (permalink / raw) To: Michal Hocko; +Cc: linux-kernel, linux-mm On Thu, 2017-11-09 at 11:05 +0100, Michal Hocko wrote: > On Tue 07-11-17 08:03:27, Joe Perches wrote: > > On Tue, 2017-11-07 at 16:43 +0100, Michal Hocko wrote: > > > On Tue 07-11-17 07:34:25, Joe Perches wrote: > > > > [] > > > > I believe, but have not tested, that using a specific width > > > > as an argument to %*pb[l] will constrain the number of > > > > spaces before the '(null)' output in any NULL pointer use. > > > > > > > > So how about a #define like > > > > > > > > /* > > > > * nodemask_pr_args is only used with a "%*pb[l]" format for a nodemask. > > > > * A NULL nodemask uses 6 to emit "(null)" without leading spaces. > > > > */ > > > > #define nodemask_pr_args(maskp) \ > > > > (maskp) ? MAX_NUMNODES : 6, \ > > > > (maskp) ? (maskp)->bits : NULL > > > > > > Why not -1 then? > > > > I believe it's the field width and not the precision that > > needs to be set. > > But the first of the two arguments is the field with specifier, not the > precision. /me confused... > > Anyway, the following works as expected when printing the OOM report: > [ 47.005321] mem_eater invoked oom-killer: gfp_mask=0x14280ca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO), nodemask=(null), order=0, oom_score_adj=0 > [ 47.007183] mem_eater cpuset=/ mems_allowed=0-1 > [ 47.007829] CPU: 3 PID: 3223 Comm: mem_eater Tainted: G W 4.13.0-pr1-dirty #11 > > I hope I haven't overlooked anything Hey Michal. Seems right. The bit I overlooked was that the field width is overridden if the output is longer so 0 works perfectly well. Thanks. If it's useful, Acked-by: Joe Perches <joe@perches.com> cheers, Joe > --- > From 35aa7742d35d29af88c66dd5e6f8f5d62215f5fd Mon Sep 17 00:00:00 2001 > From: Michal Hocko <mhocko@suse.com> > Date: Thu, 9 Nov 2017 10:58:41 +0100 > Subject: [PATCH] mm: simplify nodemask printing > > alloc_warn and dump_header have to explicitly handle NULL nodemask which > forces both paths to use pr_cont. We can do better. printk already > handles NULL pointers properly so all what we need is to teach > nodemask_pr_args to handle NULL nodemask carefully. This allows > simplification of both alloc_warn and dump_header and get rid of pr_cont > altogether. > > This patch has been motivated by patch from Joe Perches > http://lkml.kernel.org/r/b31236dfe3fc924054fd7842bde678e71d193638.1509991345.git.joe@perches.com > > Signed-off-by: Michal Hocko <mhocko@suse.com> > --- > include/linux/nodemask.h | 2 +- > mm/oom_kill.c | 12 ++++-------- > mm/page_alloc.c | 12 +++--------- > 3 files changed, 8 insertions(+), 18 deletions(-) > > diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h > index cf0b91c3ec12..5d3cc67207ed 100644 > --- a/include/linux/nodemask.h > +++ b/include/linux/nodemask.h > @@ -103,7 +103,7 @@ extern nodemask_t _unused_nodemask_arg_; > * > * Can be used to provide arguments for '%*pb[l]' when printing a nodemask. > */ > -#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp)->bits > +#define nodemask_pr_args(maskp) (maskp) ? MAX_NUMNODES : 0, (maskp) ? (maskp)->bits : NULL > > /* > * The inline keyword gives the compiler room to decide to inline, or > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 8dd0e088189b..606213a81ceb 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -426,14 +426,10 @@ static void dump_tasks(struct mem_cgroup *memcg, const nodemask_t *nodemask) > > static void dump_header(struct oom_control *oc, struct task_struct *p) > { > - pr_warn("%s invoked oom-killer: gfp_mask=%#x(%pGg), nodemask=", > - current->comm, oc->gfp_mask, &oc->gfp_mask); > - if (oc->nodemask) > - pr_cont("%*pbl", nodemask_pr_args(oc->nodemask)); > - else > - pr_cont("(null)"); > - pr_cont(", order=%d, oom_score_adj=%hd\n", > - oc->order, current->signal->oom_score_adj); > + pr_warn("%s invoked oom-killer: gfp_mask=%#x(%pGg), nodemask=%*pbl, order=%d, oom_score_adj=%hd\n", > + current->comm, oc->gfp_mask, &oc->gfp_mask, > + nodemask_pr_args(oc->nodemask), oc->order, > + current->signal->oom_score_adj); > if (!IS_ENABLED(CONFIG_COMPACTION) && oc->order) > pr_warn("COMPACTION is disabled!!!\n"); > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index d755434aee94..457e43ed4c10 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -3281,20 +3281,14 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...) > if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs)) > return; > > - pr_warn("%s: ", current->comm); > - > va_start(args, fmt); > vaf.fmt = fmt; > vaf.va = &args; > - pr_cont("%pV", &vaf); > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=%*pbl\n", > + current->comm, &vaf, gfp_mask, &gfp_mask, > + nodemask_pr_args(nodemask)); > va_end(args); > > - pr_cont(", mode:%#x(%pGg), nodemask=", gfp_mask, &gfp_mask); > - if (nodemask) > - pr_cont("%*pbl\n", nodemask_pr_args(nodemask)); > - else > - pr_cont("(null)\n"); > - > cpuset_print_current_mems_allowed(); > > dump_stack(); > -- > 2.14.2 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc @ 2017-11-09 15:49 ` Joe Perches 0 siblings, 0 replies; 18+ messages in thread From: Joe Perches @ 2017-11-09 15:49 UTC (permalink / raw) To: Michal Hocko; +Cc: linux-kernel, linux-mm On Thu, 2017-11-09 at 11:05 +0100, Michal Hocko wrote: > On Tue 07-11-17 08:03:27, Joe Perches wrote: > > On Tue, 2017-11-07 at 16:43 +0100, Michal Hocko wrote: > > > On Tue 07-11-17 07:34:25, Joe Perches wrote: > > > > [] > > > > I believe, but have not tested, that using a specific width > > > > as an argument to %*pb[l] will constrain the number of > > > > spaces before the '(null)' output in any NULL pointer use. > > > > > > > > So how about a #define like > > > > > > > > /* > > > > * nodemask_pr_args is only used with a "%*pb[l]" format for a nodemask. > > > > * A NULL nodemask uses 6 to emit "(null)" without leading spaces. > > > > */ > > > > #define nodemask_pr_args(maskp) \ > > > > (maskp) ? MAX_NUMNODES : 6, \ > > > > (maskp) ? (maskp)->bits : NULL > > > > > > Why not -1 then? > > > > I believe it's the field width and not the precision that > > needs to be set. > > But the first of the two arguments is the field with specifier, not the > precision. /me confused... > > Anyway, the following works as expected when printing the OOM report: > [ 47.005321] mem_eater invoked oom-killer: gfp_mask=0x14280ca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO), nodemask=(null), order=0, oom_score_adj=0 > [ 47.007183] mem_eater cpuset=/ mems_allowed=0-1 > [ 47.007829] CPU: 3 PID: 3223 Comm: mem_eater Tainted: G W 4.13.0-pr1-dirty #11 > > I hope I haven't overlooked anything Hey Michal. Seems right. The bit I overlooked was that the field width is overridden if the output is longer so 0 works perfectly well. Thanks. If it's useful, Acked-by: Joe Perches <joe@perches.com> cheers, Joe > --- > From 35aa7742d35d29af88c66dd5e6f8f5d62215f5fd Mon Sep 17 00:00:00 2001 > From: Michal Hocko <mhocko@suse.com> > Date: Thu, 9 Nov 2017 10:58:41 +0100 > Subject: [PATCH] mm: simplify nodemask printing > > alloc_warn and dump_header have to explicitly handle NULL nodemask which > forces both paths to use pr_cont. We can do better. printk already > handles NULL pointers properly so all what we need is to teach > nodemask_pr_args to handle NULL nodemask carefully. This allows > simplification of both alloc_warn and dump_header and get rid of pr_cont > altogether. > > This patch has been motivated by patch from Joe Perches > http://lkml.kernel.org/r/b31236dfe3fc924054fd7842bde678e71d193638.1509991345.git.joe@perches.com > > Signed-off-by: Michal Hocko <mhocko@suse.com> > --- > include/linux/nodemask.h | 2 +- > mm/oom_kill.c | 12 ++++-------- > mm/page_alloc.c | 12 +++--------- > 3 files changed, 8 insertions(+), 18 deletions(-) > > diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h > index cf0b91c3ec12..5d3cc67207ed 100644 > --- a/include/linux/nodemask.h > +++ b/include/linux/nodemask.h > @@ -103,7 +103,7 @@ extern nodemask_t _unused_nodemask_arg_; > * > * Can be used to provide arguments for '%*pb[l]' when printing a nodemask. > */ > -#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp)->bits > +#define nodemask_pr_args(maskp) (maskp) ? MAX_NUMNODES : 0, (maskp) ? (maskp)->bits : NULL > > /* > * The inline keyword gives the compiler room to decide to inline, or > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 8dd0e088189b..606213a81ceb 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -426,14 +426,10 @@ static void dump_tasks(struct mem_cgroup *memcg, const nodemask_t *nodemask) > > static void dump_header(struct oom_control *oc, struct task_struct *p) > { > - pr_warn("%s invoked oom-killer: gfp_mask=%#x(%pGg), nodemask=", > - current->comm, oc->gfp_mask, &oc->gfp_mask); > - if (oc->nodemask) > - pr_cont("%*pbl", nodemask_pr_args(oc->nodemask)); > - else > - pr_cont("(null)"); > - pr_cont(", order=%d, oom_score_adj=%hd\n", > - oc->order, current->signal->oom_score_adj); > + pr_warn("%s invoked oom-killer: gfp_mask=%#x(%pGg), nodemask=%*pbl, order=%d, oom_score_adj=%hd\n", > + current->comm, oc->gfp_mask, &oc->gfp_mask, > + nodemask_pr_args(oc->nodemask), oc->order, > + current->signal->oom_score_adj); > if (!IS_ENABLED(CONFIG_COMPACTION) && oc->order) > pr_warn("COMPACTION is disabled!!!\n"); > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index d755434aee94..457e43ed4c10 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -3281,20 +3281,14 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...) > if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs)) > return; > > - pr_warn("%s: ", current->comm); > - > va_start(args, fmt); > vaf.fmt = fmt; > vaf.va = &args; > - pr_cont("%pV", &vaf); > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=%*pbl\n", > + current->comm, &vaf, gfp_mask, &gfp_mask, > + nodemask_pr_args(nodemask)); > va_end(args); > > - pr_cont(", mode:%#x(%pGg), nodemask=", gfp_mask, &gfp_mask); > - if (nodemask) > - pr_cont("%*pbl\n", nodemask_pr_args(nodemask)); > - else > - pr_cont("(null)\n"); > - > cpuset_print_current_mems_allowed(); > > dump_stack(); > -- > 2.14.2 > -- 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> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc 2017-11-09 15:49 ` Joe Perches @ 2017-11-09 16:03 ` Michal Hocko -1 siblings, 0 replies; 18+ messages in thread From: Michal Hocko @ 2017-11-09 16:03 UTC (permalink / raw) To: Joe Perches; +Cc: linux-kernel, linux-mm, Andrew Morton, Tejun Heo [I've just noticed that Andrew was not on the CC, also add Tejun who has added nodemask_pr_args - the patch is http://lkml.kernel.org/r/20171109100531.3cn2hcqnuj7mjaju@dhcp22.suse.cz The only downside I can think of is that nodemask_pr_args cannot get any argument with sideeffects now. But that doesn't seem to be the case in the current code and the simplicit of the code using it is quite attractive to me] On Thu 09-11-17 07:49:07, Joe Perches wrote: > On Thu, 2017-11-09 at 11:05 +0100, Michal Hocko wrote: > > On Tue 07-11-17 08:03:27, Joe Perches wrote: > > > On Tue, 2017-11-07 at 16:43 +0100, Michal Hocko wrote: > > > > On Tue 07-11-17 07:34:25, Joe Perches wrote: > > > > > > [] > > > > > I believe, but have not tested, that using a specific width > > > > > as an argument to %*pb[l] will constrain the number of > > > > > spaces before the '(null)' output in any NULL pointer use. > > > > > > > > > > So how about a #define like > > > > > > > > > > /* > > > > > * nodemask_pr_args is only used with a "%*pb[l]" format for a nodemask. > > > > > * A NULL nodemask uses 6 to emit "(null)" without leading spaces. > > > > > */ > > > > > #define nodemask_pr_args(maskp) \ > > > > > (maskp) ? MAX_NUMNODES : 6, \ > > > > > (maskp) ? (maskp)->bits : NULL > > > > > > > > Why not -1 then? > > > > > > I believe it's the field width and not the precision that > > > needs to be set. > > > > But the first of the two arguments is the field with specifier, not the > > precision. /me confused... > > > > Anyway, the following works as expected when printing the OOM report: > > [ 47.005321] mem_eater invoked oom-killer: gfp_mask=0x14280ca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO), nodemask=(null), order=0, oom_score_adj=0 > > [ 47.007183] mem_eater cpuset=/ mems_allowed=0-1 > > [ 47.007829] CPU: 3 PID: 3223 Comm: mem_eater Tainted: G W 4.13.0-pr1-dirty #11 > > > > I hope I haven't overlooked anything > > Hey Michal. > > Seems right. The bit I overlooked was that the > field width is overridden if the output is longer > so 0 works perfectly well. > > Thanks. > > If it's useful, > > Acked-by: Joe Perches <joe@perches.com> > > cheers, Joe > > --- > > From 35aa7742d35d29af88c66dd5e6f8f5d62215f5fd Mon Sep 17 00:00:00 2001 > > From: Michal Hocko <mhocko@suse.com> > > Date: Thu, 9 Nov 2017 10:58:41 +0100 > > Subject: [PATCH] mm: simplify nodemask printing > > > > alloc_warn and dump_header have to explicitly handle NULL nodemask which > > forces both paths to use pr_cont. We can do better. printk already > > handles NULL pointers properly so all what we need is to teach > > nodemask_pr_args to handle NULL nodemask carefully. This allows > > simplification of both alloc_warn and dump_header and get rid of pr_cont > > altogether. > > > > This patch has been motivated by patch from Joe Perches > > http://lkml.kernel.org/r/b31236dfe3fc924054fd7842bde678e71d193638.1509991345.git.joe@perches.com > > > > Signed-off-by: Michal Hocko <mhocko@suse.com> > > --- > > include/linux/nodemask.h | 2 +- > > mm/oom_kill.c | 12 ++++-------- > > mm/page_alloc.c | 12 +++--------- > > 3 files changed, 8 insertions(+), 18 deletions(-) > > > > diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h > > index cf0b91c3ec12..5d3cc67207ed 100644 > > --- a/include/linux/nodemask.h > > +++ b/include/linux/nodemask.h > > @@ -103,7 +103,7 @@ extern nodemask_t _unused_nodemask_arg_; > > * > > * Can be used to provide arguments for '%*pb[l]' when printing a nodemask. > > */ > > -#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp)->bits > > +#define nodemask_pr_args(maskp) (maskp) ? MAX_NUMNODES : 0, (maskp) ? (maskp)->bits : NULL > > > > /* > > * The inline keyword gives the compiler room to decide to inline, or > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > > index 8dd0e088189b..606213a81ceb 100644 > > --- a/mm/oom_kill.c > > +++ b/mm/oom_kill.c > > @@ -426,14 +426,10 @@ static void dump_tasks(struct mem_cgroup *memcg, const nodemask_t *nodemask) > > > > static void dump_header(struct oom_control *oc, struct task_struct *p) > > { > > - pr_warn("%s invoked oom-killer: gfp_mask=%#x(%pGg), nodemask=", > > - current->comm, oc->gfp_mask, &oc->gfp_mask); > > - if (oc->nodemask) > > - pr_cont("%*pbl", nodemask_pr_args(oc->nodemask)); > > - else > > - pr_cont("(null)"); > > - pr_cont(", order=%d, oom_score_adj=%hd\n", > > - oc->order, current->signal->oom_score_adj); > > + pr_warn("%s invoked oom-killer: gfp_mask=%#x(%pGg), nodemask=%*pbl, order=%d, oom_score_adj=%hd\n", > > + current->comm, oc->gfp_mask, &oc->gfp_mask, > > + nodemask_pr_args(oc->nodemask), oc->order, > > + current->signal->oom_score_adj); > > if (!IS_ENABLED(CONFIG_COMPACTION) && oc->order) > > pr_warn("COMPACTION is disabled!!!\n"); > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index d755434aee94..457e43ed4c10 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -3281,20 +3281,14 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...) > > if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs)) > > return; > > > > - pr_warn("%s: ", current->comm); > > - > > va_start(args, fmt); > > vaf.fmt = fmt; > > vaf.va = &args; > > - pr_cont("%pV", &vaf); > > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=%*pbl\n", > > + current->comm, &vaf, gfp_mask, &gfp_mask, > > + nodemask_pr_args(nodemask)); > > va_end(args); > > > > - pr_cont(", mode:%#x(%pGg), nodemask=", gfp_mask, &gfp_mask); > > - if (nodemask) > > - pr_cont("%*pbl\n", nodemask_pr_args(nodemask)); > > - else > > - pr_cont("(null)\n"); > > - > > cpuset_print_current_mems_allowed(); > > > > dump_stack(); > > -- > > 2.14.2 > > -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc @ 2017-11-09 16:03 ` Michal Hocko 0 siblings, 0 replies; 18+ messages in thread From: Michal Hocko @ 2017-11-09 16:03 UTC (permalink / raw) To: Joe Perches; +Cc: linux-kernel, linux-mm, Andrew Morton, Tejun Heo [I've just noticed that Andrew was not on the CC, also add Tejun who has added nodemask_pr_args - the patch is http://lkml.kernel.org/r/20171109100531.3cn2hcqnuj7mjaju@dhcp22.suse.cz The only downside I can think of is that nodemask_pr_args cannot get any argument with sideeffects now. But that doesn't seem to be the case in the current code and the simplicit of the code using it is quite attractive to me] On Thu 09-11-17 07:49:07, Joe Perches wrote: > On Thu, 2017-11-09 at 11:05 +0100, Michal Hocko wrote: > > On Tue 07-11-17 08:03:27, Joe Perches wrote: > > > On Tue, 2017-11-07 at 16:43 +0100, Michal Hocko wrote: > > > > On Tue 07-11-17 07:34:25, Joe Perches wrote: > > > > > > [] > > > > > I believe, but have not tested, that using a specific width > > > > > as an argument to %*pb[l] will constrain the number of > > > > > spaces before the '(null)' output in any NULL pointer use. > > > > > > > > > > So how about a #define like > > > > > > > > > > /* > > > > > * nodemask_pr_args is only used with a "%*pb[l]" format for a nodemask. > > > > > * A NULL nodemask uses 6 to emit "(null)" without leading spaces. > > > > > */ > > > > > #define nodemask_pr_args(maskp) \ > > > > > (maskp) ? MAX_NUMNODES : 6, \ > > > > > (maskp) ? (maskp)->bits : NULL > > > > > > > > Why not -1 then? > > > > > > I believe it's the field width and not the precision that > > > needs to be set. > > > > But the first of the two arguments is the field with specifier, not the > > precision. /me confused... > > > > Anyway, the following works as expected when printing the OOM report: > > [ 47.005321] mem_eater invoked oom-killer: gfp_mask=0x14280ca(GFP_HIGHUSER_MOVABLE|__GFP_ZERO), nodemask=(null), order=0, oom_score_adj=0 > > [ 47.007183] mem_eater cpuset=/ mems_allowed=0-1 > > [ 47.007829] CPU: 3 PID: 3223 Comm: mem_eater Tainted: G W 4.13.0-pr1-dirty #11 > > > > I hope I haven't overlooked anything > > Hey Michal. > > Seems right. The bit I overlooked was that the > field width is overridden if the output is longer > so 0 works perfectly well. > > Thanks. > > If it's useful, > > Acked-by: Joe Perches <joe@perches.com> > > cheers, Joe > > --- > > From 35aa7742d35d29af88c66dd5e6f8f5d62215f5fd Mon Sep 17 00:00:00 2001 > > From: Michal Hocko <mhocko@suse.com> > > Date: Thu, 9 Nov 2017 10:58:41 +0100 > > Subject: [PATCH] mm: simplify nodemask printing > > > > alloc_warn and dump_header have to explicitly handle NULL nodemask which > > forces both paths to use pr_cont. We can do better. printk already > > handles NULL pointers properly so all what we need is to teach > > nodemask_pr_args to handle NULL nodemask carefully. This allows > > simplification of both alloc_warn and dump_header and get rid of pr_cont > > altogether. > > > > This patch has been motivated by patch from Joe Perches > > http://lkml.kernel.org/r/b31236dfe3fc924054fd7842bde678e71d193638.1509991345.git.joe@perches.com > > > > Signed-off-by: Michal Hocko <mhocko@suse.com> > > --- > > include/linux/nodemask.h | 2 +- > > mm/oom_kill.c | 12 ++++-------- > > mm/page_alloc.c | 12 +++--------- > > 3 files changed, 8 insertions(+), 18 deletions(-) > > > > diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h > > index cf0b91c3ec12..5d3cc67207ed 100644 > > --- a/include/linux/nodemask.h > > +++ b/include/linux/nodemask.h > > @@ -103,7 +103,7 @@ extern nodemask_t _unused_nodemask_arg_; > > * > > * Can be used to provide arguments for '%*pb[l]' when printing a nodemask. > > */ > > -#define nodemask_pr_args(maskp) MAX_NUMNODES, (maskp)->bits > > +#define nodemask_pr_args(maskp) (maskp) ? MAX_NUMNODES : 0, (maskp) ? (maskp)->bits : NULL > > > > /* > > * The inline keyword gives the compiler room to decide to inline, or > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > > index 8dd0e088189b..606213a81ceb 100644 > > --- a/mm/oom_kill.c > > +++ b/mm/oom_kill.c > > @@ -426,14 +426,10 @@ static void dump_tasks(struct mem_cgroup *memcg, const nodemask_t *nodemask) > > > > static void dump_header(struct oom_control *oc, struct task_struct *p) > > { > > - pr_warn("%s invoked oom-killer: gfp_mask=%#x(%pGg), nodemask=", > > - current->comm, oc->gfp_mask, &oc->gfp_mask); > > - if (oc->nodemask) > > - pr_cont("%*pbl", nodemask_pr_args(oc->nodemask)); > > - else > > - pr_cont("(null)"); > > - pr_cont(", order=%d, oom_score_adj=%hd\n", > > - oc->order, current->signal->oom_score_adj); > > + pr_warn("%s invoked oom-killer: gfp_mask=%#x(%pGg), nodemask=%*pbl, order=%d, oom_score_adj=%hd\n", > > + current->comm, oc->gfp_mask, &oc->gfp_mask, > > + nodemask_pr_args(oc->nodemask), oc->order, > > + current->signal->oom_score_adj); > > if (!IS_ENABLED(CONFIG_COMPACTION) && oc->order) > > pr_warn("COMPACTION is disabled!!!\n"); > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index d755434aee94..457e43ed4c10 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -3281,20 +3281,14 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...) > > if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs)) > > return; > > > > - pr_warn("%s: ", current->comm); > > - > > va_start(args, fmt); > > vaf.fmt = fmt; > > vaf.va = &args; > > - pr_cont("%pV", &vaf); > > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=%*pbl\n", > > + current->comm, &vaf, gfp_mask, &gfp_mask, > > + nodemask_pr_args(nodemask)); > > va_end(args); > > > > - pr_cont(", mode:%#x(%pGg), nodemask=", gfp_mask, &gfp_mask); > > - if (nodemask) > > - pr_cont("%*pbl\n", nodemask_pr_args(nodemask)); > > - else > > - pr_cont("(null)\n"); > > - > > cpuset_print_current_mems_allowed(); > > > > dump_stack(); > > -- > > 2.14.2 > > -- Michal Hocko SUSE Labs -- 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> ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc 2017-11-09 10:05 ` Michal Hocko @ 2017-11-09 16:27 ` Joe Perches -1 siblings, 0 replies; 18+ messages in thread From: Joe Perches @ 2017-11-09 16:27 UTC (permalink / raw) To: Michal Hocko; +Cc: linux-kernel, linux-mm On Thu, 2017-11-09 at 11:05 +0100, Michal Hocko wrote: [] > Subject: [PATCH] mm: simplify nodemask printing > > alloc_warn and dump_header have to explicitly handle NULL nodemask which > forces both paths to use pr_cont. We can do better. printk already > handles NULL pointers properly so all what we need is to teach > nodemask_pr_args to handle NULL nodemask carefully. This allows > simplification of both alloc_warn and dump_header and get rid of pr_cont > altogether. [] > diff --git a/mm/oom_kill.c b/mm/oom_kill.c [] > @@ -426,14 +426,10 @@ static void dump_tasks(struct mem_cgroup *memcg, const nodemask_t *nodemask) > > static void dump_header(struct oom_control *oc, struct task_struct *p) > { > - pr_warn("%s invoked oom-killer: gfp_mask=%#x(%pGg), nodemask=", > - current->comm, oc->gfp_mask, &oc->gfp_mask); > - if (oc->nodemask) > - pr_cont("%*pbl", nodemask_pr_args(oc->nodemask)); > - else > - pr_cont("(null)"); > - pr_cont(", order=%d, oom_score_adj=%hd\n", > - oc->order, current->signal->oom_score_adj); > + pr_warn("%s invoked oom-killer: gfp_mask=%#x(%pGg), nodemask=%*pbl, order=%d, oom_score_adj=%hd\n", > + current->comm, oc->gfp_mask, &oc->gfp_mask, > + nodemask_pr_args(oc->nodemask), oc->order, > + current->signal->oom_score_adj); trivia: You could align the arguments to the open parenthesis here > diff --git a/mm/page_alloc.c b/mm/page_alloc.c [] > @@ -3281,20 +3281,14 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...) > if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs)) > return; > > - pr_warn("%s: ", current->comm); > - > va_start(args, fmt); > vaf.fmt = fmt; > vaf.va = &args; > - pr_cont("%pV", &vaf); > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=%*pbl\n", > + current->comm, &vaf, gfp_mask, &gfp_mask, > + nodemask_pr_args(nodemask)); and here ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc @ 2017-11-09 16:27 ` Joe Perches 0 siblings, 0 replies; 18+ messages in thread From: Joe Perches @ 2017-11-09 16:27 UTC (permalink / raw) To: Michal Hocko; +Cc: linux-kernel, linux-mm On Thu, 2017-11-09 at 11:05 +0100, Michal Hocko wrote: [] > Subject: [PATCH] mm: simplify nodemask printing > > alloc_warn and dump_header have to explicitly handle NULL nodemask which > forces both paths to use pr_cont. We can do better. printk already > handles NULL pointers properly so all what we need is to teach > nodemask_pr_args to handle NULL nodemask carefully. This allows > simplification of both alloc_warn and dump_header and get rid of pr_cont > altogether. [] > diff --git a/mm/oom_kill.c b/mm/oom_kill.c [] > @@ -426,14 +426,10 @@ static void dump_tasks(struct mem_cgroup *memcg, const nodemask_t *nodemask) > > static void dump_header(struct oom_control *oc, struct task_struct *p) > { > - pr_warn("%s invoked oom-killer: gfp_mask=%#x(%pGg), nodemask=", > - current->comm, oc->gfp_mask, &oc->gfp_mask); > - if (oc->nodemask) > - pr_cont("%*pbl", nodemask_pr_args(oc->nodemask)); > - else > - pr_cont("(null)"); > - pr_cont(", order=%d, oom_score_adj=%hd\n", > - oc->order, current->signal->oom_score_adj); > + pr_warn("%s invoked oom-killer: gfp_mask=%#x(%pGg), nodemask=%*pbl, order=%d, oom_score_adj=%hd\n", > + current->comm, oc->gfp_mask, &oc->gfp_mask, > + nodemask_pr_args(oc->nodemask), oc->order, > + current->signal->oom_score_adj); trivia: You could align the arguments to the open parenthesis here > diff --git a/mm/page_alloc.c b/mm/page_alloc.c [] > @@ -3281,20 +3281,14 @@ void warn_alloc(gfp_t gfp_mask, nodemask_t *nodemask, const char *fmt, ...) > if ((gfp_mask & __GFP_NOWARN) || !__ratelimit(&nopage_rs)) > return; > > - pr_warn("%s: ", current->comm); > - > va_start(args, fmt); > vaf.fmt = fmt; > vaf.va = &args; > - pr_cont("%pV", &vaf); > + pr_warn("%s: %pV, mode:%#x(%pGg), nodemask=%*pbl\n", > + current->comm, &vaf, gfp_mask, &gfp_mask, > + nodemask_pr_args(nodemask)); and here -- 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> ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2017-11-09 16:27 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-06 18:02 [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc Joe Perches 2017-11-06 18:02 ` Joe Perches 2017-11-07 12:50 ` Michal Hocko 2017-11-07 12:50 ` Michal Hocko 2017-11-07 15:34 ` Joe Perches 2017-11-07 15:34 ` Joe Perches 2017-11-07 15:43 ` Michal Hocko 2017-11-07 15:43 ` Michal Hocko 2017-11-07 16:03 ` Joe Perches 2017-11-07 16:03 ` Joe Perches 2017-11-09 10:05 ` Michal Hocko 2017-11-09 10:05 ` Michal Hocko 2017-11-09 15:49 ` Joe Perches 2017-11-09 15:49 ` Joe Perches 2017-11-09 16:03 ` Michal Hocko 2017-11-09 16:03 ` Michal Hocko 2017-11-09 16:27 ` Joe Perches 2017-11-09 16:27 ` Joe Perches
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.