On Wed, Oct 18, 2023 at 7:08 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
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.
Thank you Matthew Wilcox for pointing this out. I will extract page_pgdat() prior to freeing the memory in v3.
With regards,
Sourav Panda
