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

[Alkaid <zgf574564920@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 4136 bytes --]