[PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc

[PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc

[Joe Perches]

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>

Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc

[Michal Hocko]

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>

Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc

[Joe Perches]

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>

Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc

[Michal Hocko]

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>

Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc

[Joe Perches]

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>

Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc

[Michal Hocko]

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

Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc

[Joe Perches]

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>

Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc

[Michal Hocko]

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

Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc

[Joe Perches]

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>