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

[Matthew Wilcox <willy@infradead.org>]

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
>