From: Johannes Weiner <hannes@cmpxchg.org> To: Joonsoo Kim <js1304@gmail.com>, Alex Shi <alex.shi@linux.alibaba.com> Cc: 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@kvack.org, cgroups@vger.kernel.org, linux-kernel@vger.kernel.org, kernel-team@fb.com Subject: [PATCH 02/18] mm: memcontrol: fix theoretical race in charge moving Date: Mon, 20 Apr 2020 18:11:10 -0400 [thread overview] Message-ID: <20200420221126.341272-3-hannes@cmpxchg.org> (raw) In-Reply-To: <20200420221126.341272-1-hannes@cmpxchg.org> The move_lock is a per-memcg lock, but the VM accounting code that needs to acquire it comes from the page and follows page->mem_cgroup under RCU protection. That means that the page becomes unlocked not when we drop the move_lock, but when we update page->mem_cgroup. And that assignment doesn't imply any memory ordering. If that pointer write gets reordered against the reads of the page state - page_mapped, PageDirty etc. the state may change while we rely on it being stable and we can end up corrupting the counters. Place an SMP memory barrier to make sure we're done with all page state by the time the new page->mem_cgroup becomes visible. Also replace the open-coded move_lock with a lock_page_memcg() to make it more obvious what we're serializing against. Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> --- mm/memcontrol.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 5beea03dd58a..41f5ed79272e 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -5372,7 +5372,6 @@ static int mem_cgroup_move_account(struct page *page, { struct lruvec *from_vec, *to_vec; struct pglist_data *pgdat; - unsigned long flags; unsigned int nr_pages = compound ? hpage_nr_pages(page) : 1; int ret; bool anon; @@ -5399,18 +5398,13 @@ static int mem_cgroup_move_account(struct page *page, from_vec = mem_cgroup_lruvec(from, pgdat); to_vec = mem_cgroup_lruvec(to, pgdat); - spin_lock_irqsave(&from->move_lock, flags); + lock_page_memcg(page); if (!anon && page_mapped(page)) { __mod_lruvec_state(from_vec, NR_FILE_MAPPED, -nr_pages); __mod_lruvec_state(to_vec, NR_FILE_MAPPED, nr_pages); } - /* - * move_lock grabbed above and caller set from->moving_account, so - * mod_memcg_page_state will serialize updates to PageDirty. - * So mapping should be stable for dirty pages. - */ if (!anon && PageDirty(page)) { struct address_space *mapping = page_mapping(page); @@ -5426,15 +5420,23 @@ static int mem_cgroup_move_account(struct page *page, } /* + * All state has been migrated, let's switch to the new memcg. + * * It is safe to change page->mem_cgroup here because the page - * is referenced, charged, and isolated - we can't race with - * uncharging, charging, migration, or LRU putback. + * is referenced, charged, isolated, and locked: we can't race + * with (un)charging, migration, LRU putback, or anything else + * that would rely on a stable page->mem_cgroup. + * + * Note that lock_page_memcg is a memcg lock, not a page lock, + * to save space. As soon as we switch page->mem_cgroup to a + * new memcg that isn't locked, the above state can change + * concurrently again. Make sure we're truly done with it. */ + smp_mb(); - /* caller should have done css_get */ - page->mem_cgroup = to; + page->mem_cgroup = to; /* caller should have done css_get */ - spin_unlock_irqrestore(&from->move_lock, flags); + __unlock_page_memcg(from); ret = 0; -- 2.26.0
WARNING: multiple messages have this Message-ID (diff)
From: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org> To: Joonsoo Kim <js1304-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Alex Shi <alex.shi-KPsoFbNs7GizrGE5bRqYAgC/G2K4zDHf@public.gmane.org> Cc: Shakeel Butt <shakeelb-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>, Hugh Dickins <hughd-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>, Michal Hocko <mhocko-IBi9RG/b67k@public.gmane.org>, "Kirill A. Shutemov" <kirill-oKw7cIdHH8eLwutG50LtGA@public.gmane.org>, Roman Gushchin <guro-b10kYP2dOMg@public.gmane.org>, linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, kernel-team-b10kYP2dOMg@public.gmane.org Subject: [PATCH 02/18] mm: memcontrol: fix theoretical race in charge moving Date: Mon, 20 Apr 2020 18:11:10 -0400 [thread overview] Message-ID: <20200420221126.341272-3-hannes@cmpxchg.org> (raw) In-Reply-To: <20200420221126.341272-1-hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org> The move_lock is a per-memcg lock, but the VM accounting code that needs to acquire it comes from the page and follows page->mem_cgroup under RCU protection. That means that the page becomes unlocked not when we drop the move_lock, but when we update page->mem_cgroup. And that assignment doesn't imply any memory ordering. If that pointer write gets reordered against the reads of the page state - page_mapped, PageDirty etc. the state may change while we rely on it being stable and we can end up corrupting the counters. Place an SMP memory barrier to make sure we're done with all page state by the time the new page->mem_cgroup becomes visible. Also replace the open-coded move_lock with a lock_page_memcg() to make it more obvious what we're serializing against. Signed-off-by: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org> --- mm/memcontrol.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 5beea03dd58a..41f5ed79272e 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -5372,7 +5372,6 @@ static int mem_cgroup_move_account(struct page *page, { struct lruvec *from_vec, *to_vec; struct pglist_data *pgdat; - unsigned long flags; unsigned int nr_pages = compound ? hpage_nr_pages(page) : 1; int ret; bool anon; @@ -5399,18 +5398,13 @@ static int mem_cgroup_move_account(struct page *page, from_vec = mem_cgroup_lruvec(from, pgdat); to_vec = mem_cgroup_lruvec(to, pgdat); - spin_lock_irqsave(&from->move_lock, flags); + lock_page_memcg(page); if (!anon && page_mapped(page)) { __mod_lruvec_state(from_vec, NR_FILE_MAPPED, -nr_pages); __mod_lruvec_state(to_vec, NR_FILE_MAPPED, nr_pages); } - /* - * move_lock grabbed above and caller set from->moving_account, so - * mod_memcg_page_state will serialize updates to PageDirty. - * So mapping should be stable for dirty pages. - */ if (!anon && PageDirty(page)) { struct address_space *mapping = page_mapping(page); @@ -5426,15 +5420,23 @@ static int mem_cgroup_move_account(struct page *page, } /* + * All state has been migrated, let's switch to the new memcg. + * * It is safe to change page->mem_cgroup here because the page - * is referenced, charged, and isolated - we can't race with - * uncharging, charging, migration, or LRU putback. + * is referenced, charged, isolated, and locked: we can't race + * with (un)charging, migration, LRU putback, or anything else + * that would rely on a stable page->mem_cgroup. + * + * Note that lock_page_memcg is a memcg lock, not a page lock, + * to save space. As soon as we switch page->mem_cgroup to a + * new memcg that isn't locked, the above state can change + * concurrently again. Make sure we're truly done with it. */ + smp_mb(); - /* caller should have done css_get */ - page->mem_cgroup = to; + page->mem_cgroup = to; /* caller should have done css_get */ - spin_unlock_irqrestore(&from->move_lock, flags); + __unlock_page_memcg(from); ret = 0; -- 2.26.0
next prev parent reply other threads:[~2020-04-20 22:11 UTC|newest] Thread overview: 140+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-04-20 22:11 [PATCH 00/18] mm: memcontrol: charge swapin pages on instantiation Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-20 22:11 ` [PATCH 01/18] mm: fix NUMA node file count error in replace_page_cache() Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-21 8:28 ` Alex Shi 2020-04-21 8:28 ` Alex Shi 2020-04-21 19:13 ` Shakeel Butt 2020-04-21 19:13 ` Shakeel Butt 2020-04-21 19:13 ` Shakeel Butt 2020-04-22 6:34 ` Joonsoo Kim 2020-04-22 6:34 ` Joonsoo Kim 2020-04-20 22:11 ` Johannes Weiner [this message] 2020-04-20 22:11 ` [PATCH 02/18] mm: memcontrol: fix theoretical race in charge moving Johannes Weiner 2020-04-22 6:36 ` Joonsoo Kim 2020-04-22 6:36 ` Joonsoo Kim 2020-04-22 16:51 ` Shakeel Butt 2020-04-22 16:51 ` Shakeel Butt 2020-04-22 16:51 ` Shakeel Butt 2020-04-22 17:42 ` Johannes Weiner 2020-04-22 17:42 ` Johannes Weiner 2020-04-22 18:01 ` Shakeel Butt 2020-04-22 18:01 ` Shakeel Butt 2020-04-22 18:01 ` Shakeel Butt 2020-04-22 18:02 ` Shakeel Butt 2020-04-22 18:02 ` Shakeel Butt 2020-04-20 22:11 ` [PATCH 03/18] mm: memcontrol: drop @compound parameter from memcg charging API Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-21 9:11 ` Alex Shi 2020-04-21 9:11 ` Alex Shi 2020-04-22 6:37 ` Joonsoo Kim 2020-04-22 6:37 ` Joonsoo Kim 2020-04-22 17:30 ` Shakeel Butt 2020-04-22 17:30 ` Shakeel Butt 2020-04-22 17:30 ` Shakeel Butt 2020-04-20 22:11 ` [PATCH 04/18] mm: memcontrol: move out cgroup swaprate throttling Johannes Weiner 2020-04-21 9:11 ` Alex Shi 2020-04-21 9:11 ` Alex Shi 2020-04-22 6:37 ` Joonsoo Kim 2020-04-22 6:37 ` Joonsoo Kim 2020-04-22 22:20 ` Shakeel Butt 2020-04-22 22:20 ` Shakeel Butt 2020-04-22 22:20 ` Shakeel Butt 2020-04-20 22:11 ` [PATCH 05/18] mm: memcontrol: convert page cache to a new mem_cgroup_charge() API Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-21 9:12 ` Alex Shi 2020-04-21 9:12 ` Alex Shi 2020-04-22 6:40 ` Joonsoo Kim 2020-04-22 6:40 ` Joonsoo Kim 2020-04-22 12:09 ` Johannes Weiner 2020-04-22 12:09 ` Johannes Weiner 2020-04-23 5:25 ` Joonsoo Kim 2020-05-08 16:01 ` Johannes Weiner 2020-05-11 1:57 ` Joonsoo Kim 2020-05-11 7:38 ` Hugh Dickins 2020-05-11 7:38 ` Hugh Dickins 2020-05-11 7:38 ` Hugh Dickins 2020-05-11 15:06 ` Johannes Weiner 2020-05-11 16:32 ` Hugh Dickins 2020-05-11 16:32 ` Hugh Dickins 2020-05-11 16:32 ` Hugh Dickins 2020-05-11 18:10 ` Johannes Weiner 2020-05-11 18:10 ` Johannes Weiner 2020-05-11 18:12 ` Johannes Weiner 2020-05-11 18:44 ` Hugh Dickins 2020-05-11 18:44 ` Hugh Dickins 2020-05-11 18:44 ` Hugh Dickins 2020-04-20 22:11 ` [PATCH 06/18] mm: memcontrol: prepare uncharging for removal of private page type counters Johannes Weiner 2020-04-21 9:12 ` Alex Shi 2020-04-21 9:12 ` Alex Shi 2020-04-22 6:41 ` Joonsoo Kim 2020-04-22 6:41 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 07/18] mm: memcontrol: prepare move_account " Johannes Weiner 2020-04-21 9:13 ` Alex Shi 2020-04-21 9:13 ` Alex Shi 2020-04-22 6:41 ` Joonsoo Kim 2020-04-22 6:41 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 08/18] mm: memcontrol: prepare cgroup vmstat infrastructure for native anon counters Johannes Weiner 2020-04-22 6:42 ` Joonsoo Kim 2020-04-22 6:42 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 09/18] mm: memcontrol: switch to native NR_FILE_PAGES and NR_SHMEM counters Johannes Weiner 2020-04-22 6:42 ` Joonsoo Kim 2020-04-22 6:42 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 10/18] mm: memcontrol: switch to native NR_ANON_MAPPED counter Johannes Weiner 2020-04-22 6:51 ` Joonsoo Kim 2020-04-22 12:28 ` Johannes Weiner 2020-04-23 5:27 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 11/18] mm: memcontrol: switch to native NR_ANON_THPS counter Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-24 0:29 ` Joonsoo Kim 2020-04-24 0:29 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 12/18] mm: memcontrol: convert anon and file-thp to new mem_cgroup_charge() API Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-24 0:29 ` Joonsoo Kim 2020-04-24 0:29 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 13/18] mm: memcontrol: drop unused try/commit/cancel charge API Johannes Weiner 2020-04-24 0:30 ` Joonsoo Kim 2020-04-24 0:30 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 14/18] mm: memcontrol: prepare swap controller setup for integration Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-24 0:30 ` Joonsoo Kim 2020-04-24 0:30 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 15/18] mm: memcontrol: make swap tracking an integral part of memory control Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-21 9:27 ` Alex Shi 2020-04-21 9:27 ` Alex Shi 2020-04-21 14:39 ` Johannes Weiner 2020-04-21 14:39 ` Johannes Weiner 2020-04-22 3:14 ` Alex Shi 2020-04-22 3:14 ` Alex Shi 2020-04-22 13:30 ` Johannes Weiner 2020-04-22 13:30 ` Johannes Weiner 2020-04-22 13:40 ` Alex Shi 2020-04-22 13:43 ` Alex Shi 2020-04-24 0:30 ` Joonsoo Kim 2020-04-24 0:30 ` Joonsoo Kim 2020-04-24 3:01 ` Johannes Weiner 2020-04-20 22:11 ` [PATCH 16/18] mm: memcontrol: charge swapin pages on instantiation Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-21 9:21 ` Alex Shi 2020-04-21 9:21 ` Alex Shi 2020-04-24 0:44 ` Joonsoo Kim 2020-04-24 2:51 ` Johannes Weiner 2020-04-24 2:51 ` Johannes Weiner 2020-04-28 6:49 ` Joonsoo Kim 2020-04-28 6:49 ` Joonsoo Kim 2020-04-28 6:49 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 17/18] mm: memcontrol: delete unused lrucare handling Johannes Weiner 2020-04-20 22:11 ` Johannes Weiner 2020-04-24 0:46 ` Joonsoo Kim 2020-04-24 0:46 ` Joonsoo Kim 2020-04-20 22:11 ` [PATCH 18/18] mm: memcontrol: update page->mem_cgroup stability rules Johannes Weiner 2020-04-21 9:20 ` Alex Shi 2020-04-21 9:20 ` Alex Shi 2020-04-24 0:48 ` Joonsoo Kim 2020-04-24 0:48 ` Joonsoo Kim 2020-04-21 9:10 ` Hillf Danton 2020-04-21 14:34 ` Johannes Weiner 2020-04-21 14:34 ` Johannes Weiner 2020-04-21 9:32 ` [PATCH 00/18] mm: memcontrol: charge swapin pages on instantiation Alex Shi 2020-04-21 9:32 ` Alex Shi
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=20200420221126.341272-3-hannes@cmpxchg.org \ --to=hannes@cmpxchg.org \ --cc=alex.shi@linux.alibaba.com \ --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 \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.