Re: [PATCH v1 1/1] mm: report per-page metadata information

From: Sourav Panda <souravpanda@google.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: corbet@lwn.net, gregkh@linuxfoundation.org, rafael@kernel.org,
	 akpm@linux-foundation.org, mike.kravetz@oracle.com,
	muchun.song@linux.dev,  rppt@kernel.org, david@redhat.com,
	rdunlap@infradead.org,  chenlinxuan@uniontech.com,
	yang.yang29@zte.com.cn, tomas.mudrunka@gmail.com,
	 bhelgaas@google.com, ivan@cloudflare.com,
	pasha.tatashin@soleen.com,  yosryahmed@google.com,
	hannes@cmpxchg.org, shakeelb@google.com,
	 kirill.shutemov@linux.intel.com, wangkefeng.wang@huawei.com,
	 adobriyan@gmail.com, vbabka@suse.cz, Liam.Howlett@oracle.com,
	 surenb@google.com, linux-kernel@vger.kernel.org,
	 linux-fsdevel@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-mm@kvack.org
Subject: Re: [PATCH v1 1/1] mm: report per-page metadata information
Date: Thu, 14 Sep 2023 14:04:22 -0700	[thread overview]
Message-ID: <CANruzcTYgSHmbYD=uMXuDokOFar+dVHkY1Mm-7BKr7xYW654GQ@mail.gmail.com> (raw)
In-Reply-To: <ZQH3tHbz2ghsyqHG@casper.infradead.org>

[-- Attachment #1: Type: text/plain, Size: 2784 bytes --]

Hi Matthew Wilcox,

Thank you very much for reviewing my patch. Please find my responses below.

On Wed, Sep 13, 2023 at 10:56 AM Matthew Wilcox <willy@infradead.org> wrote:

> On Wed, Sep 13, 2023 at 10:30:00AM -0700, Sourav Panda wrote:
> > @@ -387,8 +390,12 @@ static int alloc_vmemmap_page_list(unsigned long
> start, unsigned long end,
> >
> >       while (nr_pages--) {
> >               page = alloc_pages_node(nid, gfp_mask, 0);
> > -             if (!page)
> > +             if (!page) {
> >                       goto out;
> > +             } else {
> > +                     __mod_node_page_state(NODE_DATA(page_to_nid(page)),
> > +                                           NR_PAGE_METADATA, 1);
> > +             }
> >               list_add_tail(&page->lru, list);
>
> What a strange way of writing this.  Why not simply:
>
>                 if (!page)
>                         goto out;
> +               __mod_node_page_state(NODE_DATA(page_to_nid(page)),
> +                               NR_PAGE_METADATA, 1);
>                 list_add_tail(&page->lru, list);
>

Thank you Matthew Wilcox for your comment. I agree with you and will make
the corresponding change.

>
> > @@ -314,6 +319,10 @@ static void free_page_ext(void *addr)
> >               BUG_ON(PageReserved(page));
> >               kmemleak_free(addr);
> >               free_pages_exact(addr, table_size);
> > +
> > +             __mod_node_page_state(NODE_DATA(page_to_nid(page)),
> NR_PAGE_METADATA,
> > +                                   (long)-1 * (PAGE_ALIGN(table_size)
> >> PAGE_SHIFT));
>
> Why not spell that as "-1L"?
>
> And while I'm asking questions, why NODE_DATA(page_to_nid(page)) instead
> of page_pgdat(page)?
>

Yes, thank you! I shall make both the suggested changes.

>
> > @@ -2274,4 +2275,24 @@ static int __init extfrag_debug_init(void)
> >  }
> >
> >  module_init(extfrag_debug_init);
> > +
> > +// Page metadata size (struct page and page_ext) in pages
>
> Don't use // comments.
>

Thank you. I shall replace them with /* text */ to be uniform with the
document.

>
> > +void __init writeout_early_perpage_metadata(void)
>
> "writeout" is something swap does.  I'm sure this has a better name,
> though I can't think what it might be.
>
> > +{
> > +     int nid;
> > +     struct pglist_data *pgdat;
> > +
> > +     for_each_online_pgdat(pgdat) {
> > +             nid = pgdat->node_id;
> > +             __mod_node_page_state(NODE_DATA(nid), NR_PAGE_METADATA,
> > +                                   early_perpage_metadata[nid]);
> > +     }
> > +}
> >  #endif
> > --
> > 2.42.0.283.g2d96d420d3-goog
> >

Yep, thank you! Does store_early_perpage_metadata seem better to you?

[-- Attachment #2: Type: text/html, Size: 4149 bytes --]