From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756854AbdKGPeb (ORCPT ); Tue, 7 Nov 2017 10:34:31 -0500 Received: from smtprelay0103.hostedemail.com ([216.40.44.103]:39569 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1756777AbdKGPe3 (ORCPT ); Tue, 7 Nov 2017 10:34:29 -0500 X-Session-Marker: 6A6F6540706572636865732E636F6D X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,joe@perches.com,:::::,RULES_HIT:41:355:379:541:599:960:968:973:988:989:1260:1277:1311:1313:1314:1345:1359:1373:1437:1515:1516:1518:1534:1542:1593:1594:1711:1730:1747:1777:1792:2393:2559:2562:2691:2693:2828:3138:3139:3140:3141:3142:3355:3622:3865:3866:3867:3870:3871:3872:3873:3874:4250:4321:5007:6119:10004:10400:10848:11026:11232:11658:11914:12043:12296:12438:12740:12760:12895:13439:14181:14659:14721:21080:21434:21451:21611:21627:30012:30034:30054:30056:30091,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:,MSBL:0,DNSBL:none,Custom_rules:0:0:0,LFtime:2,LUA_SUMMARY:none X-HE-Tag: thing75_7e3837d57aa4a X-Filterd-Recvd-Size: 3745 Message-ID: <1510068865.1000.19.camel@perches.com> Subject: Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc From: Joe Perches To: Michal Hocko Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org Date: Tue, 07 Nov 2017 07:34:25 -0800 In-Reply-To: <20171107125055.cl5pyp2zwon44x5l@dhcp22.suse.cz> References: <20171107125055.cl5pyp2zwon44x5l@dhcp22.suse.cz> Content-Type: text/plain; charset="ISO-8859-1" X-Mailer: Evolution 3.26.1-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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) 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-f70.google.com (mail-it0-f70.google.com [209.85.214.70]) by kanga.kvack.org (Postfix) with ESMTP id E7BD1280264 for ; Tue, 7 Nov 2017 10:34:29 -0500 (EST) Received: by mail-it0-f70.google.com with SMTP id a188so2485583itc.2 for ; Tue, 07 Nov 2017 07:34:29 -0800 (PST) Received: from smtprelay.hostedemail.com (smtprelay0009.hostedemail.com. [216.40.44.9]) by mx.google.com with ESMTPS id n67si1269472ion.184.2017.11.07.07.34.28 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 07 Nov 2017 07:34:28 -0800 (PST) Message-ID: <1510068865.1000.19.camel@perches.com> Subject: Re: [PATCH] mm/page_alloc: Avoid KERN_CONT uses in warn_alloc From: Joe Perches Date: Tue, 07 Nov 2017 07:34:25 -0800 In-Reply-To: <20171107125055.cl5pyp2zwon44x5l@dhcp22.suse.cz> References: <20171107125055.cl5pyp2zwon44x5l@dhcp22.suse.cz> Content-Type: text/plain; charset="ISO-8859-1" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Michal Hocko Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org 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) 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: email@kvack.org