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 X-Spam-Level: X-Spam-Status: No, score=-2.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 21BBEC433DB for ; Mon, 1 Feb 2021 13:50:38 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 8B58F64E97 for ; Mon, 1 Feb 2021 13:50:37 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8B58F64E97 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id D65836B0074; Mon, 1 Feb 2021 08:50:36 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id D3DA36B0080; Mon, 1 Feb 2021 08:50:36 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C51956B0081; Mon, 1 Feb 2021 08:50:36 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0060.hostedemail.com [216.40.44.60]) by kanga.kvack.org (Postfix) with ESMTP id B03516B0074 for ; Mon, 1 Feb 2021 08:50:36 -0500 (EST) Received: from smtpin09.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 70F12180AD81D for ; Mon, 1 Feb 2021 13:50:36 +0000 (UTC) X-FDA: 77769834072.09.02156DA Received: from mail-il1-f173.google.com (mail-il1-f173.google.com [209.85.166.173]) by imf01.hostedemail.com (Postfix) with ESMTP id 1A8E020001DE for ; Mon, 1 Feb 2021 13:50:34 +0000 (UTC) Received: by mail-il1-f173.google.com with SMTP id d6so15588036ilo.6 for ; Mon, 01 Feb 2021 05:50:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=4ABF6jWhcQFCEmEQUi+MWDc4uEldLt/+v1YpG4pvAfY=; b=l8FX33RFY+6UjFhdAfGlB2xSl5/mqnQgdobUWZng8utCR1RZNMAheLlLps7aHUwYZG QZ0DrmVuB+ST+5fy72xMxpyOvgKVs8+HLWE34/OV8EMkwh/Rmh0UXRWveOSmxbPR9J8X Y1cBQ1d8FEOiEy4twZMCxF3AMAW2XrpVLqesm36k6L7ll5bzapx3zfhshDdoQ+/5e1gc xXM7ww1xbrAYIZLXWTJr7c9neQDD25yCzwOO/navm8zmVZPkubPGakLuW/H8qwwNANTt 8aXt22Q1TiFTQhJd6tGNjGZeD0EJQwKtTO2fY3HXaD0tdBY/KRbb8XdQbQ0Tal+A/mu+ BxEA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=4ABF6jWhcQFCEmEQUi+MWDc4uEldLt/+v1YpG4pvAfY=; b=r4QwKdIOoVQ3elSzTbgpjuqqV6bdpQjHvYC9GlHLVEhJ1Vvf9WAvBLjTWlkXnln3av bTOCclAuIiwZIF21m2uW28UfEPCijMr2Ic1NmCPLDlU+Hd8CbtcMoIAK010LvmRwxRFv SMLrHOSLBXnRbHzCMlpq3FFmlepTqFoC8LKjZVJ9OStsZ19SvD/RFBsk9GlSliTZMmDT UcfO8DNlC93gkimXa1s+2sbBwYYSFfwIZF6D6H8ApoHpHS6X54nTgRVWE70MWF5V3MlT BcHlxuhNxn2SDs2xIxCoMvVCItYIM3E1GHNxp4/b1QjgbUncpn4+q3N/OrlkzFya3Ym8 brjw== X-Gm-Message-State: AOAM530Tqo8fg7x5NduVJanZOUJmot+S2q5Lqx6bQv6fg2jwjSHU3z68 mrMvIMlfj3o5M9JQOSVNLAAkKoKxvAplbfWhn/s= X-Google-Smtp-Source: ABdhPJznUESnOIbGJwf1+ormRBd1+0OM0Q1fktPuC8QeY4ftQlGam+1WVF7x5yGNPlq1dMmzU5ZhpS8iKMDqKiunzYA= X-Received: by 2002:a92:d3c7:: with SMTP id c7mr12410988ilh.137.1612187435360; Mon, 01 Feb 2021 05:50:35 -0800 (PST) MIME-Version: 1.0 References: <20210201115610.87808-1-laoar.shao@gmail.com> <20210201115610.87808-4-laoar.shao@gmail.com> In-Reply-To: From: Yafang Shao Date: Mon, 1 Feb 2021 21:49:59 +0800 Message-ID: Subject: Re: [PATCH v2 3/3] vsprintf: dump full information of page flags in pGp To: Andy Shevchenko Cc: David Hildenbrand , Vlastimil Babka , Miaohe Lin , Christoph Lameter , penberg@kernel.org, David Rientjes , iamjoonsoo.kim@lge.com, Andrew Morton , Petr Mladek , Steven Rostedt , Sergey Senozhatsky , Rasmus Villemoes , Linux MM , LKML , Joe Perches Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: 1A8E020001DE X-Stat-Signature: 88jsj4r18xd9t5hp7jdi4ctehkopagq1 Received-SPF: none (gmail.com>: No applicable sender policy available) receiver=imf01; identity=mailfrom; envelope-from=""; helo=mail-il1-f173.google.com; client-ip=209.85.166.173 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1612187434-233860 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 Mon, Feb 1, 2021 at 9:27 PM Andy Shevchenko wrote: > > On Mon, Feb 01, 2021 at 07:56:10PM +0800, Yafang Shao wrote: > > Currently the pGp only shows the names of page flags, rather than > > the full information including section, node, zone, last cpupid and > > kasan tag. While it is not easy to parse these information manually > > because there're so many flavors. Let's interpret them in pGp as well. > > > > - Before the patch, > > [ 6343.396602] Slab 0x000000004382e02b objects=33 used=3 fp=0x000000009ae06ffc flags=0x17ffffc0010200(slab|head) > > > > - After the patch, > > [ 6871.296131] Slab 0x00000000c0e19a37 objects=33 used=3 fp=0x00000000c4902159 flags=0x17ffffc0010200(Node 0,Zone 2,Lastcpupid 0x1fffff,slab|head) > > > > The Documentation and test cases are also updated. > > Thanks for an update, my comments below. > > ... > > > - %pGp referenced|uptodate|lru|active|private > > + %pGp Node 0,Zone 2,referenced|uptodate|lru|active|private > > Since of the nature of printf() buffer, I wonder if these should be at the end. > I.o.w. the question is is the added material more important to user to see than > the existed one? > The existing one should be more important than the added one. But the order of output will not match with the value for page->flags. E.g. flags=0x17ffffc0010200(slab|head,Node 0,Zone 2,Lastcpupid 0x1fffff) It may be strange to compare the value with the string. > ... > > > +static void __init > > +page_flags_test(int section, int node, int zone, int last_cpupid, > > + int kasan_tag, int flags, const char *name, char *cmp_buf) > > +{ > > + unsigned long page_flags = 0; > > + unsigned long size = 0; > > + > > +#ifdef SECTION_IN_PAGE_FLAGS > > + page_flags |= (sec & SECTIONS_MASK) << SECTIONS_PGSHIFT; > > + snprintf(cmp_buf, BUF_SIZE, "Section %#x,", sec); > > I would keep it in the same form as latter ones, i.e. > > snprintf(cmp_buf + size, BUF_SIZE - size, "Section %#x,", sec); > > In this case it will be easier if at some point we might need to reshuffle. > Good suggestion. > > + size = strlen(cmp_buf); > > +#endif > > + > > + page_flags |= (node & NODES_MASK) << NODES_PGSHIFT; > > + snprintf(cmp_buf + size, BUF_SIZE - size, "Node %d", node); > > + size = strlen(cmp_buf); > > + > > + page_flags |= (zone & ZONES_MASK) << ZONES_PGSHIFT; > > + snprintf(cmp_buf + size, BUF_SIZE - size, ",Zone %d", zone); > > + size = strlen(cmp_buf); > > + > > +#ifndef LAST_CPUPID_NOT_IN_PAGE_FLAGS > > + page_flags |= (last_cpupid & LAST_CPUPID_MASK) << LAST_CPUPID_PGSHIFT; > > + snprintf(cmp_buf + size, BUF_SIZE - size, ",Lastcpupid %#x", last_cpupid); > > + size = strlen(cmp_buf); > > +#endif > > + > > +#if defined(CONFIG_KASAN_SW_TAGS) || defined(CONFIG_KASAN_HW_TAGS) > > + page_flags |= (tag & KASAN_TAG_MASK) << KASAN_TAG_PGSHIFT; > > + snprintf(cmp_buf + size, BUF_SIZE - size, ",Kasantag %#x", tag); > > + size = strlen(cmp_buf); > > +#endif > > + > > + test(cmp_buf, "%pGp", &page_flags); > > + > > + if (flags) { > > If below will be always for flags set, I would rewrite this condition as > > if (!flags) > return; > > but it's up to you. > > > + page_flags |= flags; > > + snprintf(cmp_buf + size, BUF_SIZE - size, ",%s", name); > > + test(cmp_buf, "%pGp", &page_flags); > > + } > > +} > > ... > > > - flags = 0; > > > - flags |= 1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru > > - | 1UL << PG_active | 1UL << PG_swapbacked; > > I would leave this untouched and reuse below... > > > + cmp_buffer = kmalloc(BUF_SIZE, GFP_KERNEL); > > + if (!cmp_buffer) > > + return; > > ...as > > > + page_flags_test(0, 0, 0, 0, 0, 0, NULL, cmp_buffer); > > flags = 0; > page_flags_test(0, 0, 0, 0, 0, flags, NULL, cmp_buffer); > > > + page_flags_test(1, 1, 1, 0x1ffff, 1, > > + (1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru > > + | 1UL << PG_active | 1UL << PG_swapbacked), > > + "uptodate|dirty|lru|active|swapbacked", > > + cmp_buffer); > > flags |= 1UL << PG_uptodate | 1UL << PG_dirty | 1UL << PG_lru > | 1UL << PG_active | 1UL << PG_swapbacked; > page_flags_test(1, 1, 1, 0x1ffff, 1, flags, > "uptodate|dirty|lru|active|swapbacked", > cmp_buffer); > Sure, I will change them. > ... > > > +static const struct page_flags_layout pfl[] = { > > + {SECTIONS_WIDTH, SECTIONS_PGSHIFT, SECTIONS_MASK, > > + &default_dec_spec, "Section "}, > > + {NODES_WIDTH, NODES_PGSHIFT, NODES_MASK, > > + &default_dec_spec, "Node "}, > > + {ZONES_WIDTH, ZONES_PGSHIFT, ZONES_MASK, > > + &default_dec_spec, "Zone "}, > > + {LAST_CPUPID_WIDTH, LAST_CPUPID_PGSHIFT, LAST_CPUPID_MASK, > > + &default_flag_spec, "Lastcpupid "}, > > + {KASAN_TAG_WIDTH, KASAN_TAG_PGSHIFT, KASAN_TAG_MASK, > > + &default_flag_spec, "Kasantag "}, > > +}; > > Please add trailing space only once where it's needed (below in the code). > > ... > > > +static > > +char *format_page_flags(char *buf, char *end, unsigned long page_flags) > > +{ > > + unsigned long flags = page_flags & ((1UL << NR_PAGEFLAGS) - 1); > > + int size = ARRAY_SIZE(pfl); > > + bool separator = false; > > + int i; > > + > > + for (i = 0; i < size; i++) { > > + if (pfl[i].width == 0) > > + continue; > > + > > + if (separator) { > > + if (buf < end) > > + *buf = ','; > > + buf++; > > + } > > > + > > + > > One blank line is enough. > Thanks for pointing it out. BTW, it seems we need to improve the check_patch to catch this kind of issue automatically. > > + buf = string(buf, end, pfl[i].name, *pfl[i].spec); > > + > > + buf = number(buf, end, (page_flags >> pfl[i].shift) & pfl[i].mask, > > + *pfl[i].spec); > > + separator = true; > > + } > > + > > + if (flags) { > > + if (buf < end) > > + *buf = ','; > > + buf++; > > + } > > I think you may optimize above to avoid using the separator variable. > > DECLARE_BITMAP(mask, ARRAY_SIZE(pfl)); > unsigned long flags; > unsigned long last; > > for (i = 0; i < ARRAY_SIZE(pfl); i++) > __assign_bit(mask, pfl[i].width); > > last = find_last_bit(mask, ARRAY_SIZE(pfl)); > > for_each_set_bit(i, mask, ARRAY_SIZE(pfl)) { > flags = (page_flags >> pfl[i].shift) & pfl[i].mask; > > /* Format: Flag Name + ' ' (space) + Number + ',' (separator) */ > buf = string(buf, end, pfl[i].name, *pfl[i].spec); > > if (buf < end) > *buf = ' '; > buf++; > > buf = number(buf, end, flags, *pfl[i].spec); > > /* No separator for the last entry */ > if ((page_flags & (BIT(NR_PAGEFLAGS) - 1)) || (i != last)) { > if (buf < end) > *buf = ','; > buf++; > } > } > Good suggestion. I will think about it. -- Thanks Yafang