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