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

Matthew Wilcox
2023-09-13T17:56:48Z

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);

> @@ -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)?

> @@ -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.

> +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
>