From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7D5DEC433F5 for ; Fri, 28 Jan 2022 21:18:52 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 9BFD66B00F2; Fri, 28 Jan 2022 16:18:51 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 96E006B00F4; Fri, 28 Jan 2022 16:18:51 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 80F5B6B00F6; Fri, 28 Jan 2022 16:18:51 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0141.hostedemail.com [216.40.44.141]) by kanga.kvack.org (Postfix) with ESMTP id 6EF6B6B00F2 for ; Fri, 28 Jan 2022 16:18:51 -0500 (EST) Received: from smtpin29.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 2AAE480F3CEB for ; Fri, 28 Jan 2022 21:18:51 +0000 (UTC) X-FDA: 79080960462.29.EAD495A Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by imf15.hostedemail.com (Postfix) with ESMTP id 3B468A0035 for ; Fri, 28 Jan 2022 21:18:49 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1643404730; x=1674940730; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=JuYGt4SS9apu8yRIPO2cO3Qb/Yp2GBDdA0yZrl2jfbQ=; b=Ue9bh5qI3i2Ld37bx1nFEocKwq9iMzmJsD5nuQfpslI8AHhI8aDDqEWD zHNke5vxgiI2vb+SQZ72BeT7MCJt9A26HTubhj4uEagYl9ApuMHob+Ozk yZ7dScAuxwJsPUj0AKV47oRJsORHuiIy9eA+Q2K4rt2d+w3SGWKR6kRVr usMlAlZxDt6gFL5CffxuGTn8G1j5uJ6lVC59uE1sRSF0Fe6HrPLUL/agm urEt2JXZ9dyhgzg86XRz/v1h9LGwT4RDevjNxBT2AoOgWhCmCeH0NK22c kImUREPpZTsns4onUUzNqTczPSIJ23gtRTLtZ56y2fl1P96TStXWIJNyx g==; X-IronPort-AV: E=McAfee;i="6200,9189,10241"; a="247148592" X-IronPort-AV: E=Sophos;i="5.88,325,1635231600"; d="scan'208";a="247148592" Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Jan 2022 13:18:48 -0800 X-IronPort-AV: E=Sophos;i="5.88,325,1635231600"; d="scan'208";a="480867661" Received: from iweiny-desk2.sc.intel.com (HELO localhost) ([10.3.52.147]) by orsmga006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Jan 2022 13:18:48 -0800 Date: Fri, 28 Jan 2022 13:18:48 -0800 From: Ira Weiny To: Waiman Long Cc: Johannes Weiner , Michal Hocko , Vladimir Davydov , Andrew Morton , linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, linux-mm@kvack.org, Rafael Aquini Subject: Re: [PATCH 1/2] mm/page_owner: Introduce SNPRINTF() macro that includes length error check Message-ID: <20220128211848.GH785175@iweiny-DESK2.sc.intel.com> References: <20220128195642.416743-1-longman@redhat.com> <20220128195642.416743-2-longman@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220128195642.416743-2-longman@redhat.com> User-Agent: Mutt/1.11.1 (2018-12-01) X-Rspamd-Server: rspam11 X-Rspamd-Queue-Id: 3B468A0035 X-Stat-Signature: oe3dz4zhyn71jepsjmy6ixidz8a43cb1 Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=Ue9bh5qI; dmarc=pass (policy=none) header.from=intel.com; spf=none (imf15.hostedemail.com: domain of ira.weiny@intel.com has no SPF policy when checking 134.134.136.65) smtp.mailfrom=ira.weiny@intel.com X-Rspam-User: nil X-HE-Tag: 1643404729-696771 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Jan 28, 2022 at 02:56:41PM -0500, Waiman Long wrote: > In print_page_owner(), there is a repetitive check after each snprintf() > to make sure that the final length is less than the buffer size. Simplify > the code and make it easier to read by embedding the buffer length > check and increment inside the macro. This does not follow the kernel coding style of not putting control flow statements in macros.[1] > > Signed-off-by: Waiman Long > --- > mm/page_owner.c | 50 +++++++++++++++++++++++-------------------------- > 1 file changed, 23 insertions(+), 27 deletions(-) And in the end you only saved 4 lines of code to read... Not worth it IMO. Ira [1] https://www.kernel.org/doc/html/v4.17/process/coding-style.html > > diff --git a/mm/page_owner.c b/mm/page_owner.c > index 99e360df9465..c52ce9d6bc3b 100644 > --- a/mm/page_owner.c > +++ b/mm/page_owner.c > @@ -325,12 +325,20 @@ void pagetypeinfo_showmixedcount_print(struct seq_file *m, > seq_putc(m, '\n'); > } > > +#define SNPRINTF(_buf, _size, _len, _err, _fmt, ...) \ > + do { \ > + _len += snprintf(_buf + _len, _size - _len, _fmt, \ > + ##__VA_ARGS__); \ > + if (_len >= _size) \ > + goto _err; \ > + } while (0) > + > static ssize_t > print_page_owner(char __user *buf, size_t count, unsigned long pfn, > struct page *page, struct page_owner *page_owner, > depot_stack_handle_t handle) > { > - int ret, pageblock_mt, page_mt; > + int ret = 0, pageblock_mt, page_mt; > char *kbuf; > > count = min_t(size_t, count, PAGE_SIZE); > @@ -338,44 +346,32 @@ print_page_owner(char __user *buf, size_t count, unsigned long pfn, > if (!kbuf) > return -ENOMEM; > > - ret = snprintf(kbuf, count, > - "Page allocated via order %u, mask %#x(%pGg), pid %d, ts %llu ns, free_ts %llu ns\n", > - page_owner->order, page_owner->gfp_mask, > - &page_owner->gfp_mask, page_owner->pid, > - page_owner->ts_nsec, page_owner->free_ts_nsec); > - > - if (ret >= count) > - goto err; > + SNPRINTF(kbuf, count, ret, err, > + "Page allocated via order %u, mask %#x(%pGg), pid %d, ts %llu ns, free_ts %llu ns\n", > + page_owner->order, page_owner->gfp_mask, > + &page_owner->gfp_mask, page_owner->pid, > + page_owner->ts_nsec, page_owner->free_ts_nsec); > > /* Print information relevant to grouping pages by mobility */ > pageblock_mt = get_pageblock_migratetype(page); > page_mt = gfp_migratetype(page_owner->gfp_mask); > - ret += snprintf(kbuf + ret, count - ret, > - "PFN %lu type %s Block %lu type %s Flags %pGp\n", > - pfn, > - migratetype_names[page_mt], > - pfn >> pageblock_order, > - migratetype_names[pageblock_mt], > - &page->flags); > - > - if (ret >= count) > - goto err; > + SNPRINTF(kbuf, count, ret, err, > + "PFN %lu type %s Block %lu type %s Flags %pGp\n", > + pfn, migratetype_names[page_mt], > + pfn >> pageblock_order, > + migratetype_names[pageblock_mt], > + &page->flags); > > ret += stack_depot_snprint(handle, kbuf + ret, count - ret, 0); > if (ret >= count) > goto err; > > - if (page_owner->last_migrate_reason != -1) { > - ret += snprintf(kbuf + ret, count - ret, > + if (page_owner->last_migrate_reason != -1) > + SNPRINTF(kbuf, count, ret, err, > "Page has been migrated, last migrate reason: %s\n", > migrate_reason_names[page_owner->last_migrate_reason]); > - if (ret >= count) > - goto err; > - } > > - ret += snprintf(kbuf + ret, count - ret, "\n"); > - if (ret >= count) > - goto err; > + SNPRINTF(kbuf, count, ret, err, "\n"); > > if (copy_to_user(buf, kbuf, ret)) > ret = -EFAULT; > -- > 2.27.0 > >