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

[Sourav Panda <souravpanda@google.com>]

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