From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.6 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C1245C433E6 for ; Fri, 28 Aug 2020 08:12:34 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 627BC20665 for ; Fri, 28 Aug 2020 08:12:34 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="rbG4QgmQ" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 627BC20665 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 0D7608E0006; Fri, 28 Aug 2020 04:12:34 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0879B8E0003; Fri, 28 Aug 2020 04:12:34 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EB8228E0006; Fri, 28 Aug 2020 04:12:33 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0149.hostedemail.com [216.40.44.149]) by kanga.kvack.org (Postfix) with ESMTP id D5F9D8E0003 for ; Fri, 28 Aug 2020 04:12:33 -0400 (EDT) Received: from smtpin01.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 8BD1F8245578 for ; Fri, 28 Aug 2020 08:12:33 +0000 (UTC) X-FDA: 77199260586.01.watch12_0308ea927074 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin01.hostedemail.com (Postfix) with ESMTP id 55EF51004FE9B for ; Fri, 28 Aug 2020 08:12:33 +0000 (UTC) X-HE-Tag: watch12_0308ea927074 X-Filterd-Recvd-Size: 11912 Received: from mail-vs1-f66.google.com (mail-vs1-f66.google.com [209.85.217.66]) by imf07.hostedemail.com (Postfix) with ESMTP for ; Fri, 28 Aug 2020 08:12:32 +0000 (UTC) Received: by mail-vs1-f66.google.com with SMTP id j13so158262vsr.5 for ; Fri, 28 Aug 2020 01:12:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=VDx61SfKEnXaN69pCtQEPONa6VAOViTraaTUflAhZ2k=; b=rbG4QgmQRkt/49K3UDwtA6zEBGFuqVLhgYqW+thzNeVq39d9dQKsQiMXw9HyWBr5ea +5vHG3VTQ5bQakaNsWf+CDNrVoR0zPn0Dz4oat58f91B/lOVBq9KK2LY94ShEvKNEA/9 F+xg5keUFt3qPtFdU7+yvmK/Yruc4DAe+6iwlSwh2pcLYquEm5wP4zDi6Nfa3hanOHXe fNFweuQPN52W9PFsfy3DDGS7v5TVAaEDemJFVm1FIZjzFzbjW4+ymJ+8ARlOjq+Nhrv1 hZh9qE6XgvklBMM0H7DOeqvknlekAjxVyi1i+OBxJm5mtFMZJTG5LQBREKejRPHLf7dw FhKw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=VDx61SfKEnXaN69pCtQEPONa6VAOViTraaTUflAhZ2k=; b=dkSyru8mIkCiPGQI68RuXFirjFv3YdjSYwGJ01eMlKIy7QMQOk5u/vCrEaOjHc0N9H gdFl64efuAhMb3S6CvtkjtqhpRju7dZediF9Ag7WWNT26nh9EBCzozf5XmmsyAN8yX9T iLsqB3aKcWpUqOsUXA+dcPQ+bywrXGgwkGLS6YnvMM3XYyzMExElM3fBo8iclFNMfEZs TxqzWPPmO8i4S6OODoENdY26rCFxamrluf45JpDslkSg/1E70ctu+sXlQZW2RJ7R/6eu udMwH+wq/NCGTJP7U4pqyg38yg8XrRKAfhNzmP96h41RfXNQeR+e2KVSFUzGKM0KQ2Mf 1Mug== X-Gm-Message-State: AOAM530S9zqh+I6gpnmhy9ZDPDqKzZXEcik0o188oF1EbWd8VFp29Dg+ awxzr9miDjvkZe4U9bAXbi/n3JJSwuhEsMRgo/U= X-Google-Smtp-Source: ABdhPJyiF8j4ZMrFQLjvjWuuovkDmR+PP/AODNbWSMo3Rbut20UUXsy1LMjjhRLAAdXefXBAF0rgEOXFPJRnQiFP0Po= X-Received: by 2002:a67:eb8b:: with SMTP id e11mr110044vso.223.1598602352279; Fri, 28 Aug 2020 01:12:32 -0700 (PDT) MIME-Version: 1.0 References: <20200818072501.30396-1-sjpark@amazon.com> <20200818072501.30396-7-sjpark@amazon.com> In-Reply-To: <20200818072501.30396-7-sjpark@amazon.com> From: Alkaid Date: Fri, 28 Aug 2020 04:11:56 -0400 Message-ID: Subject: Re: [RFC v7 06/10] mm/damon: Implement callbacks for physical memory monitoring To: SeongJae Park Cc: akpm@linux-foundation.org, SeongJae Park , Jonathan.Cameron@huawei.com, aarcange@redhat.com, acme@kernel.org, alexander.shishkin@linux.intel.com, amit@kernel.org, benh@kernel.crashing.org, brendan.d.gregg@gmail.com, brendanhiggins@google.com, cai@lca.pw, colin.king@canonical.com, corbet@lwn.net, david@redhat.com, dwmw@amazon.com, fan.du@intel.com, foersleo@amazon.de, gthelen@google.com, irogers@google.com, jolsa@redhat.com, kirill@shutemov.name, mark.rutland@arm.com, mgorman@suse.de, minchan@kernel.org, mingo@redhat.com, namhyung@kernel.org, peterz@infradead.org, rdunlap@infradead.org, riel@surriel.com, rientjes@google.com, rostedt@goodmis.org, rppt@kernel.org, sblbir@amazon.com, shakeelb@google.com, shuah@kernel.org, sj38.park@gmail.com, snu@amazon.de, vbabka@suse.cz, vdavydov.dev@gmail.com, yang.shi@linux.alibaba.com, ying.huang@intel.com, linux-damon@amazon.com, linux-mm@kvack.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org Content-Type: multipart/alternative; boundary="00000000000058464705adeb9d46" X-Rspamd-Queue-Id: 55EF51004FE9B X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam04 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: --00000000000058464705adeb9d46 Content-Type: text/plain; charset="UTF-8" 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 --00000000000058464705adeb9d46 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi=C2=A0SeongJae,

I think th= ere are potential memory leaks in the following execution paths
<= br>
+static bool damon_page_mkold(struct page *page, struct vm_area_struct *vma= ,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long addr,= void *arg)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0damon_mkold(vma->vm_mm, addr);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return true;
+}
+
+static void damon_phys_mkold(unsigned long paddr)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct page *page =3D damon_phys_get_page(PHYS_= PFN(paddr));
get_page() here=C2=A0
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct rmap_walk_control rwc =3D {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.rmap_one =3D damon= _page_mkold,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.anon_lock =3D page= _lock_anon_vma_read,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0};
+=C2=A0 =C2=A0 =C2=A0 =C2=A0bool need_lock;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (!page)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (!page_mapped(page) || !page_rmapping(page))=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return;
return without put_page()=C2=A0
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0need_lock =3D !PageAnon(page) || PageKsm(page);=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (need_lock && !trylock_page(page)) +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0rmap_walk(page, &rwc);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (need_lock)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unlock_page(page);<= br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0put_page(page);
+}
+
+static bool damon_phys_young(unsigned long paddr, unsigned long *page_sz)<= br> +{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct page *page =3D damon_phys_get_page(PHYS_= PFN(paddr));
get_page() here=C2=A0
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct damon_phys_access_chk_result result =3D = {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.page_sz =3D PAGE_S= IZE,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.accessed =3D false= ,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0};
+=C2=A0 =C2=A0 =C2=A0 =C2=A0struct rmap_walk_control rwc =3D {
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.arg =3D &resul= t,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.rmap_one =3D damon= _page_accessed,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0.anon_lock =3D page= _lock_anon_vma_read,
+=C2=A0 =C2=A0 =C2=A0 =C2=A0};
+=C2=A0 =C2=A0 =C2=A0 =C2=A0bool need_lock;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (!page)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return false;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (!page_mapped(page) || !page_rmapping(page))=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return false;
return without put_page()=C2=A0=C2=A0
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0need_lock =3D !PageAnon(page) || PageKsm(page);=
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (need_lock && !trylock_page(page)) +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return false;
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0rmap_walk(page, &rwc);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (need_lock)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0unlock_page(page);<= br> +=C2=A0 =C2=A0 =C2=A0 =C2=A0put_page(page);
+
+=C2=A0 =C2=A0 =C2=A0 =C2=A0*page_sz =3D result.page_sz;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return result.accessed;
+}
I observed the memory leak problem by running your = userprog [1] in a kvm vm. Compare /proc/meminfo before and after running da= mon=C2=A0+ userprog for about 30min then I observe a noticeable amount of D= RAM is not freed even if userprog exits.
I = tried to add two put_page() and the problem went away.=C2=A0I am not exactl= y 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.

<= /div>
--00000000000058464705adeb9d46--