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

[Matthew Wilcox]

On Tue, Oct 17, 2023 at 05:55:48PM -0700, Sourav Panda wrote:
> +		mod_node_early_perpage_metadata(pgdat->node_id,
> +						PAGE_ALIGN(size) >> PAGE_SHIFT);

What a curious way of writing DIV_ROUND_UP(size, PAGE_SIZE)
(throughout).  A quick grep shows about 230 DIV_ROUND_UP and 110 of what
you wrote, so it's not something you invented, but DIV_ROUND_UP is
clearer.

> @@ -303,18 +308,25 @@ static int __meminit init_section_page_ext(unsigned long pfn, int nid)
>  
>  static void free_page_ext(void *addr)
>  {
> +	size_t table_size;
> +	struct page *page;
> +
> +	table_size = page_ext_size * PAGES_PER_SECTION;
> +
>  	if (is_vmalloc_addr(addr)) {
> +		page = vmalloc_to_page(addr);
>  		vfree(addr);
>  	} else {
> -		struct page *page = virt_to_page(addr);
> -		size_t table_size;
> -
> -		table_size = page_ext_size * PAGES_PER_SECTION;
> +		page = virt_to_page(addr);
>  
>  		BUG_ON(PageReserved(page));
>  		kmemleak_free(addr);
>  		free_pages_exact(addr, table_size);
>  	}
> +
> +	__mod_node_page_state(page_pgdat(page), NR_PAGE_METADATA,
> +			      -1L * (PAGE_ALIGN(table_size) >> PAGE_SHIFT));

This troubles me.  We're freeing the memory and then dereferencing
the page that was freed.  I know that struct pages don't go away when
they're freed, and they don't change which node they're allocated to,
but it feels wrong.  I'd be happier if the page_pgdat() were extracted
prior to freeing the memory.