linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Johannes Weiner <hannes@cmpxchg.org>
To: Qian Cai <cai@lca.pw>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Alex Shi <alex.shi@linux.alibaba.com>,
	Joonsoo Kim <js1304@gmail.com>,
	Shakeel Butt <shakeelb@google.com>,
	Hugh Dickins <hughd@google.com>, Michal Hocko <mhocko@suse.com>,
	"Kirill A. Shutemov" <kirill@shutemov.name>,
	Roman Gushchin <guro@fb.com>, Linux-MM <linux-mm@kvack.org>,
	cgroups@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
	kernel-team@fb.com
Subject: Re: [PATCH 12/19] mm: memcontrol: convert anon and file-thp to new mem_cgroup_charge() API
Date: Tue, 12 May 2020 17:58:13 -0400	[thread overview]
Message-ID: <20200512215813.GA487759@cmpxchg.org> (raw)
In-Reply-To: <45AA36A9-0C4D-49C2-BA3C-08753BBC30FB@lca.pw>

On Tue, May 12, 2020 at 10:38:54AM -0400, Qian Cai wrote:
> > On May 8, 2020, at 2:30 PM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> > 
> > With the page->mapping requirement gone from memcg, we can charge anon
> > and file-thp pages in one single step, right after they're allocated.
> > 
> > This removes two out of three API calls - especially the tricky commit
> > step that needed to happen at just the right time between when the
> > page is "set up" and when it's "published" - somewhat vague and fluid
> > concepts that varied by page type. All we need is a freshly allocated
> > page and a memcg context to charge.
> > 
> > v2: prevent double charges on pre-allocated hugepages in khugepaged
> > 
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> > Reviewed-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > ---
> > include/linux/mm.h      |  4 +---
> > kernel/events/uprobes.c | 11 +++--------
> > mm/filemap.c            |  2 +-
> > mm/huge_memory.c        |  9 +++------
> > mm/khugepaged.c         | 35 ++++++++++-------------------------
> > mm/memory.c             | 36 ++++++++++--------------------------
> > mm/migrate.c            |  5 +----
> > mm/swapfile.c           |  6 +-----
> > mm/userfaultfd.c        |  5 +----
> > 9 files changed, 31 insertions(+), 82 deletions(-)
> []
> > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > 
> > @@ -1198,10 +1193,11 @@ static void collapse_huge_page(struct mm_struct *mm,
> > out_up_write:
> > 	up_write(&mm->mmap_sem);
> > out_nolock:
> > +	if (*hpage)
> > +		mem_cgroup_uncharge(*hpage);
> > 	trace_mm_collapse_huge_page(mm, isolated, result);
> > 	return;
> > out:
> > -	mem_cgroup_cancel_charge(new_page, memcg);
> > 	goto out_up_write;
> > }
> []
> 
> Some memory pressure will crash this new code. It looks like somewhat racy.
> 
> if (!page->mem_cgroup)
> 
> where page == NULL in mem_cgroup_uncharge().

Thanks for the report, sorry about the inconvenience.

Hm, the page is exclusive at this point, nobody else should be
touching it. After all, khugepaged might reuse the preallocated page
for another pmd if this one fails to collapse.

Looking at the code, I think it's page itself that's garbage, not
page->mem_cgroup changing. If you have CONFIG_NUMA and the allocation
fails, *hpage could contain an ERR_PTR instead of being NULL.

I think we need the following fixlet:

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index f2e0a5e5cfbb..f6161e17da26 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1193,7 +1193,7 @@ static void collapse_huge_page(struct mm_struct *mm,
 out_up_write:
 	up_write(&mm->mmap_sem);
 out_nolock:
-	if (*hpage)
+	if (!IS_ERR_OR_NULL(*hpage))
 		mem_cgroup_uncharge(*hpage);
 	trace_mm_collapse_huge_page(mm, isolated, result);
 	return;
@@ -1928,7 +1928,7 @@ static void collapse_file(struct mm_struct *mm,
 	unlock_page(new_page);
 out:
 	VM_BUG_ON(!list_empty(&pagelist));
-	if (*hpage)
+	if (!IS_ERR_OR_NULL(*hpage))
 		mem_cgroup_uncharge(*hpage);
 	/* TODO: tracepoints */
 }


  parent reply	other threads:[~2020-05-12 21:58 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-08 18:30 [PATCH 00/19 V2] mm: memcontrol: charge swapin pages on instantiation Johannes Weiner
2020-05-08 18:30 ` [PATCH 01/19] mm: fix NUMA node file count error in replace_page_cache() Johannes Weiner
2020-05-18 11:18   ` Balbir Singh
2020-05-08 18:30 ` [PATCH 02/19] mm: memcontrol: fix stat-corrupting race in charge moving Johannes Weiner
2020-05-08 18:30 ` [PATCH 03/19] mm: memcontrol: drop @compound parameter from memcg charging API Johannes Weiner
2020-05-08 18:30 ` [PATCH 04/19] mm: memcontrol: move out cgroup swaprate throttling Johannes Weiner
2020-05-08 18:30 ` [PATCH 05/19] mm: memcontrol: convert page cache to a new mem_cgroup_charge() API Johannes Weiner
2020-06-10 16:09   ` Michal Hocko
2020-05-08 18:30 ` [PATCH 06/19] mm: memcontrol: prepare uncharging for removal of private page type counters Johannes Weiner
2020-05-08 18:30 ` [PATCH 07/19] mm: memcontrol: prepare move_account " Johannes Weiner
2020-05-08 18:30 ` [PATCH 08/19] mm: memcontrol: prepare cgroup vmstat infrastructure for native anon counters Johannes Weiner
2020-05-08 18:30 ` [PATCH 09/19] mm: memcontrol: switch to native NR_FILE_PAGES and NR_SHMEM counters Johannes Weiner
2020-06-10 16:42   ` Michal Hocko
2020-05-08 18:30 ` [PATCH 10/19] mm: memcontrol: switch to native NR_ANON_MAPPED counter Johannes Weiner
2020-05-08 18:30 ` [PATCH 11/19] mm: memcontrol: switch to native NR_ANON_THPS counter Johannes Weiner
2020-05-08 18:30 ` [PATCH 12/19] mm: memcontrol: convert anon and file-thp to new mem_cgroup_charge() API Johannes Weiner
2020-05-12 14:38   ` Qian Cai
2020-05-12 17:11     ` Qian Cai
2020-05-12 21:58     ` Johannes Weiner [this message]
2020-05-12 23:58       ` Qian Cai
2020-05-08 18:31 ` [PATCH 13/19] mm: memcontrol: drop unused try/commit/cancel charge API Johannes Weiner
2020-06-22 17:06   ` Ben Widawsky
2020-05-08 18:31 ` [PATCH 14/19] mm: memcontrol: prepare swap controller setup for integration Johannes Weiner
2020-05-08 18:31 ` [PATCH 15/19] mm: memcontrol: make swap tracking an integral part of memory control Johannes Weiner
2020-05-08 18:31 ` [PATCH 16/19] mm: memcontrol: charge swapin pages on instantiation Johannes Weiner
2020-06-11  9:35   ` Michal Hocko
2020-06-17  8:49     ` [PATCH for 5.8] mm: do_swap_page fix up the error code instantiation Michal Hocko
2020-06-17  9:02       ` Michal Hocko
2020-06-17 13:34         ` Johannes Weiner
2020-05-08 18:31 ` [PATCH 17/19] mm: memcontrol: document the new swap control behavior Johannes Weiner
2020-05-08 18:31 ` [PATCH 18/19] mm: memcontrol: delete unused lrucare handling Johannes Weiner
2020-05-08 18:31 ` [PATCH 19/19] mm: memcontrol: update page->mem_cgroup stability rules Johannes Weiner
2020-06-11  9:40   ` Michal Hocko
2020-05-13 11:30 ` [PATCH 00/19 V2] mm: memcontrol: charge swapin pages on instantiation Balbir Singh
2020-05-13 12:35   ` Johannes Weiner
2020-05-14 11:04     ` Balbir Singh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200512215813.GA487759@cmpxchg.org \
    --to=hannes@cmpxchg.org \
    --cc=akpm@linux-foundation.org \
    --cc=alex.shi@linux.alibaba.com \
    --cc=cai@lca.pw \
    --cc=cgroups@vger.kernel.org \
    --cc=guro@fb.com \
    --cc=hughd@google.com \
    --cc=js1304@gmail.com \
    --cc=kernel-team@fb.com \
    --cc=kirill@shutemov.name \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.com \
    --cc=shakeelb@google.com \
    --subject='Re: [PATCH 12/19] mm: memcontrol: convert anon and file-thp to new mem_cgroup_charge() API' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).