Message-ID: <20200828113921.15279-1-sjpark@amazon.com>
Date:   Fri, 28 Aug 2020 13:39:21 +0200
From:   SeongJae Park <sjpark@...zon.com>
To:     Alkaid <zgf574564920@...il.com>
CC:     <akpm@...ux-foundation.org>, SeongJae Park <sjpark@...zon.de>,
        <Jonathan.Cameron@...wei.com>, <aarcange@...hat.com>,
        <acme@...nel.org>, <alexander.shishkin@...ux.intel.com>,
        <amit@...nel.org>, <benh@...nel.crashing.org>,
        <brendan.d.gregg@...il.com>, <brendanhiggins@...gle.com>,
        <cai@....pw>, <colin.king@...onical.com>, <corbet@....net>,
        <david@...hat.com>, <dwmw@...zon.com>, <fan.du@...el.com>,
        <foersleo@...zon.de>, <gthelen@...gle.com>, <irogers@...gle.com>,
        <jolsa@...hat.com>, <kirill@...temov.name>, <mark.rutland@....com>,
        <mgorman@...e.de>, <minchan@...nel.org>, <mingo@...hat.com>,
        <namhyung@...nel.org>, <peterz@...radead.org>,
        <rdunlap@...radead.org>, <riel@...riel.com>, <rientjes@...gle.com>,
        <rostedt@...dmis.org>, <rppt@...nel.org>, <sblbir@...zon.com>,
        <shakeelb@...gle.com>, <shuah@...nel.org>, <sj38.park@...il.com>,
        <snu@...zon.de>, <vbabka@...e.cz>, <vdavydov.dev@...il.com>,
        <yang.shi@...ux.alibaba.com>, <ying.huang@...el.com>,
        <linux-damon@...zon.com>, <linux-mm@...ck.org>,
        <linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        SeongJae Park <sjpark@...zon.com>
Subject: Re: [RFC v7 06/10] mm/damon: Implement callbacks for physical memory monitoring

On Fri, 28 Aug 2020 11:53:15 +0200 SeongJae Park <sjpark@...zon.com> wrote:

> On Fri, 28 Aug 2020 04:11:56 -0400 Alkaid <zgf574564920@...il.com> wrote:
> 
> > 
> > [-- Attachment #1: Type: text/plain, Size: 2677 bytes --]
> > 
> > Hi SeongJae,
> > 
> > I think there are potential memory leaks in the following execution paths
> 
> Agreed, definitely memory leaks exists.  Thank you for let me know this!  I
> will post a patch for this soon.

And, below is the patch.  The complete tree is available at:

    https://github.com/sjp38/linux/tree/damon/next


Thanks,
SeongJae Park


==================================== >8 =======================================
>From 8f605d807c55b536ab5b0f87306ac78033dc4499 Mon Sep 17 00:00:00 2001
From: SeongJae Park <sjpark@...zon.de>
Date: Fri, 28 Aug 2020 11:29:30 +0000
Subject: [PATCH] mm/damon/paddr: Add missed 'put_page()' calls

Exceptional cases handlings in 'damon_phys_mkold()' and
'damon_phys_young()' doesn't properly put pages.  This commit fixes the
problem by adding the 'put_page()' call.

Reported-by: Alkaid <zgf574564920@...il.com>
Signed-off-by: SeongJae Park <sjpark@...zon.de>
---
 mm/damon.c | 53 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 25 deletions(-)

diff --git a/mm/damon.c b/mm/damon.c
index d0d55656553b..74a10ea54958 100644
--- a/mm/damon.c
+++ b/mm/damon.c
@@ -836,12 +836,16 @@ unsigned int kdamond_check_vm_accesses(struct damon_ctx *ctx)
 /* access check functions for physical address based regions */
 
 /*
- * Get a page by pfn if it is in the LRU list.  Otherwise, returns NULL.
+ * Get a page by @pfn if it is in the LRU list and mapped.  If the page needs
+ * locked, do the lock and save the result in @locked.  Otherwise, returns
+ * NULL.
  *
- * The body of this function is stollen from the 'page_idle_get_page()'.  We
- * steal rather than reuse it because the code is quite simple .
+ * The body of this function is mostly stollen from the 'page_idle_get_page()'
+ * and 'page_idle_clear_pte_refs()'.  We steal rather than reuse it not because
+ * we are great artists but the code is quite simple and we need to unify parts
+ * of the two functions.
  */
-static struct page *damon_phys_get_page(unsigned long pfn)
+static struct page *damon_phys_get_page(unsigned long pfn, bool *locked)
 {
 	struct page *page = pfn_to_online_page(pfn);
 	pg_data_t *pgdat;
@@ -854,9 +858,22 @@ static struct page *damon_phys_get_page(unsigned long pfn)
 	spin_lock_irq(&pgdat->lru_lock);
 	if (unlikely(!PageLRU(page))) {
 		put_page(page);
-		page = NULL;
+		spin_unlock_irq(&pgdat->lru_lock);
+		return NULL;
 	}
 	spin_unlock_irq(&pgdat->lru_lock);
+
+	if (!page_mapped(page) || !page_rmapping(page)) {
+		put_page(page);
+		return NULL;
+	}
+
+	*locked = !PageAnon(page) || PageKsm(page);
+	if (*locked && !trylock_page(page)) {
+		put_page(page);
+		return NULL;
+	}
+
 	return page;
 }
 
@@ -869,26 +886,19 @@ static bool damon_page_mkold(struct page *page, struct vm_area_struct *vma,
 
 static void damon_phys_mkold(unsigned long paddr)
 {
-	struct page *page = damon_phys_get_page(PHYS_PFN(paddr));
+	bool locked;
+	struct page *page = damon_phys_get_page(PHYS_PFN(paddr), &locked);
 	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;
-
-	need_lock = !PageAnon(page) || PageKsm(page);
-	if (need_lock && !trylock_page(page))
-		return;
-
 	rmap_walk(page, &rwc);
 
-	if (need_lock)
+	if (locked)
 		unlock_page(page);
 	put_page(page);
 }
@@ -930,7 +940,8 @@ static bool damon_page_accessed(struct page *page, struct vm_area_struct *vma,
 
 static bool damon_phys_young(unsigned long paddr, unsigned long *page_sz)
 {
-	struct page *page = damon_phys_get_page(PHYS_PFN(paddr));
+	bool locked;
+	struct page *page = damon_phys_get_page(PHYS_PFN(paddr), &locked);
 	struct damon_phys_access_chk_result result = {
 		.page_sz = PAGE_SIZE,
 		.accessed = false,
@@ -940,21 +951,13 @@ static bool damon_phys_young(unsigned long paddr, unsigned long *page_sz)
 		.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;
-
-	need_lock = !PageAnon(page) || PageKsm(page);
-	if (need_lock && !trylock_page(page))
-		return false;
-
 	rmap_walk(page, &rwc);
 
-	if (need_lock)
+	if (locked)
 		unlock_page(page);
 	put_page(page);
 
-- 
2.17.1

Powered by blists - more mailing lists