On Mon, May 12, 2025 at 06:31:30PM +0200, Alexander Gordeev wrote:
On Tue, May 13, 2025 at 12:33:35AM +0900, Harry Yoo wrote:
Thanks for the update, but I don't think nr_populated is sufficient here. If nr_populated in the last iteration is smaller than its value in any previous iteration, it could lead to a memory leak.
That's why I suggested (PAGE_SIZE / sizeof(data.pages[0])). ...but on second thought maybe touching the whole array is not efficient either.
Yes, I did not like it and wanted to limit the number of pages, but did not realize that using nr_populated still could produce leaks. In addition I could simply do:
max_populted = max(max_populted, nr_populated); ... free_pages_bulk(data.pages, max_populated);
Yeah that could work, but given that it already confused you, I think we should focus on fixing the bug and defer further improvements later, since it will be backported to -stable.
If this ends up making things complicated probably we should just merge v6 instead (v6 looks good)? micro-optimizing vmalloc shadow memory population doesn't seem worth it if it comes at the cost of complexity :)
v6 is okay, except that in v7 I use break instead of return:
ret = apply_to_page_range(...); if (ret) break;
and as result can call the final:
free_page((unsigned long)data.pages);
Uh, I didn't realize that while reviewing.
I think at this stage (-rc6) Andrew will prefer a fixup patch on top of v6. I think this [1] could fix it, but could you please verify it's correct and send a fixup patch (as a reply to v6)?
[1] https://lore.kernel.org/mm-commits/aCKJYHPL_3xAewUB@hyeyoo