Re: [RFC v7 06/10] mm/damon: Implement callbacks for physical memory monitoring

Hi SeongJae,

I think there are potential memory leaks in the following execution paths

+static bool damon_page_mkold(struct page *page, struct vm_area_struct *vma,

+               unsigned long addr, void *arg)
+{
+       damon_mkold(vma->vm_mm, addr);
+       return true;
+}
+
+static void damon_phys_mkold(unsigned long paddr)
+{
+       struct page *page = damon_phys_get_page(PHYS_PFN(paddr));

get_page() here

+       struct rmap_walk_control rwc = {
+               .rmap_one = damon_page_mkold,
+               .anon_lock = page_lock_anon_vma_read,
+       };
+       bool need_lock;
+
+       if (!page)
+               return;
+
+       if (!page_mapped(page) || !page_rmapping(page))
+               return;

return without put_page()

+
+       need_lock = !PageAnon(page) || PageKsm(page);
+       if (need_lock && !trylock_page(page))
+               return;
+
+       rmap_walk(page, &rwc);
+
+       if (need_lock)
+               unlock_page(page);
+       put_page(page);
+}
+
+static bool damon_phys_young(unsigned long paddr, unsigned long *page_sz)
+{
+       struct page *page = damon_phys_get_page(PHYS_PFN(paddr));

get_page() here

+       struct damon_phys_access_chk_result result = {
+               .page_sz = PAGE_SIZE,
+               .accessed = false,
+       };
+       struct rmap_walk_control rwc = {
+               .arg = &result,
+               .rmap_one = damon_page_accessed,
+               .anon_lock = page_lock_anon_vma_read,
+       };
+       bool need_lock;
+
+       if (!page)
+               return false;
+
+       if (!page_mapped(page) || !page_rmapping(page))
+               return false;

return without put_page()

+
+       need_lock = !PageAnon(page) || PageKsm(page);
+       if (need_lock && !trylock_page(page))
+               return false;
+
+       rmap_walk(page, &rwc);
+
+       if (need_lock)
+               unlock_page(page);
+       put_page(page);
+
+       *page_sz = result.page_sz;
+       return result.accessed;
+}

I observed the memory leak problem by running your userprog [1] in a kvm
vm. Compare /proc/meminfo before and after running damon + userprog for
about 30min then I observe a noticeable amount of DRAM is not freed even if
userprog exits.
I tried to add two put_page() and the problem went away. I am not exactly
sure whether adding a put_page before return is the desired behavior, nor
can I think of a case to explain why put_page is necessary here for
"unmapped" pages.

[1] https://github.com/sjp38/idle_page_tracking/blob/master/userprog.c