All of lore.kernel.org
 help / color / mirror / Atom feed
From: Johannes Weiner <hannes@cmpxchg.org>
To: Michal Hocko <mhocko@suse.com>
Cc: Roman Gushchin <guro@fb.com>, Miaohe Lin <linmiaohe@huawei.com>,
	vdavydov.dev@gmail.com, akpm@linux-foundation.org,
	shakeelb@google.com, willy@infradead.org, alexs@kernel.org,
	richard.weiyang@gmail.com, songmuchun@bytedance.com,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	cgroups@vger.kernel.org
Subject: Re: [PATCH 2/5] mm, memcg: narrow the scope of percpu_charge_mutex
Date: Tue, 3 Aug 2021 10:15:36 -0400	[thread overview]
Message-ID: <YQlPiLY0ieRb704V@cmpxchg.org> (raw)
In-Reply-To: <YQOhGs3k9rHx3mmT@dhcp22.suse.cz>

On Fri, Jul 30, 2021 at 08:50:02AM +0200, Michal Hocko wrote:
> On Thu 29-07-21 20:06:45, Roman Gushchin wrote:
> > On Thu, Jul 29, 2021 at 08:57:52PM +0800, Miaohe Lin wrote:
> > > Since percpu_charge_mutex is only used inside drain_all_stock(), we can
> > > narrow the scope of percpu_charge_mutex by moving it here.
> > > 
> > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> > > ---
> > >  mm/memcontrol.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index 6580c2381a3e..a03e24e57cd9 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -2050,7 +2050,6 @@ struct memcg_stock_pcp {
> > >  #define FLUSHING_CACHED_CHARGE	0
> > >  };
> > >  static DEFINE_PER_CPU(struct memcg_stock_pcp, memcg_stock);
> > > -static DEFINE_MUTEX(percpu_charge_mutex);
> > >  
> > >  #ifdef CONFIG_MEMCG_KMEM
> > >  static void drain_obj_stock(struct obj_stock *stock);
> > > @@ -2209,6 +2208,7 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
> > >   */
> > >  static void drain_all_stock(struct mem_cgroup *root_memcg)
> > >  {
> > > +	static DEFINE_MUTEX(percpu_charge_mutex);
> > >  	int cpu, curcpu;
> > 
> > It's considered a good practice to protect data instead of code paths. After
> > the proposed change it becomes obvious that the opposite is done here: the mutex
> > is used to prevent a simultaneous execution of the code of the drain_all_stock()
> > function.
> 
> The purpose of the lock was indeed to orchestrate callers more than any
> data structure consistency.

It doesn't seem like we need the lock at all.

The comment says it's so we don't spawn more workers when flushing is
already underway. But a work cannot be queued more than once - if it
were just about that, we'd needlessly duplicate the
test_and_set_bit(WORK_STRUCT_PENDING_BIT) in queue_work_on().

git history shows we tried to remove it once:

commit 8521fc50d433507a7cdc96bec280f9e5888a54cc
Author: Michal Hocko <mhocko@suse.cz>
Date:   Tue Jul 26 16:08:29 2011 -0700

    memcg: get rid of percpu_charge_mutex lock

but it turned out that the lock did in fact protect a data structure:
the stock itself. Specifically stock->cached:

commit 9f50fad65b87a8776ae989ca059ad6c17925dfc3
Author: Michal Hocko <mhocko@suse.cz>
Date:   Tue Aug 9 11:56:26 2011 +0200

    Revert "memcg: get rid of percpu_charge_mutex lock"

    This reverts commit 8521fc50d433507a7cdc96bec280f9e5888a54cc.

    The patch incorrectly assumes that using atomic FLUSHING_CACHED_CHARGE
    bit operations is sufficient but that is not true.  Johannes Weiner has
    reported a crash during parallel memory cgroup removal:

      BUG: unable to handle kernel NULL pointer dereference at 0000000000000018
      IP: [<ffffffff81083b70>] css_is_ancestor+0x20/0x70
      Oops: 0000 [#1] PREEMPT SMP
      Pid: 19677, comm: rmdir Tainted: G        W   3.0.0-mm1-00188-gf38d32b #35 ECS MCP61M-M3/MCP61M-M3
      RIP: 0010:[<ffffffff81083b70>]  css_is_ancestor+0x20/0x70
      RSP: 0018:ffff880077b09c88  EFLAGS: 00010202
      Process rmdir (pid: 19677, threadinfo ffff880077b08000, task ffff8800781bb310)
      Call Trace:
       [<ffffffff810feba3>] mem_cgroup_same_or_subtree+0x33/0x40
       [<ffffffff810feccf>] drain_all_stock+0x11f/0x170
       [<ffffffff81103211>] mem_cgroup_force_empty+0x231/0x6d0
       [<ffffffff811036c4>] mem_cgroup_pre_destroy+0x14/0x20
       [<ffffffff81080559>] cgroup_rmdir+0xb9/0x500
       [<ffffffff81114d26>] vfs_rmdir+0x86/0xe0
       [<ffffffff81114e7b>] do_rmdir+0xfb/0x110
       [<ffffffff81114ea6>] sys_rmdir+0x16/0x20
       [<ffffffff8154d76b>] system_call_fastpath+0x16/0x1b

    We are crashing because we try to dereference cached memcg when we are
    checking whether we should wait for draining on the cache.  The cache is
    already cleaned up, though.

    There is also a theoretical chance that the cached memcg gets freed
    between we test for the FLUSHING_CACHED_CHARGE and dereference it in
    mem_cgroup_same_or_subtree:

            CPU0                    CPU1                         CPU2
      mem=stock->cached
      stock->cached=NULL
                                  clear_bit
                                                            test_and_set_bit
      test_bit()                    ...
      <preempted>             mem_cgroup_destroy
      use after free

    The percpu_charge_mutex protected from this race because sync draining
    is exclusive.

    It is safer to revert now and come up with a more parallel
    implementation later.

I didn't remember this one at all!

However, when you look at the codebase from back then, there was no
rcu-protection for memcg lifetime, and drain_stock() didn't double
check stock->cached inside the work. Hence the crash during a race.

The drain code is different now: drain_local_stock() disables IRQs
which holds up rcu, and then calls drain_stock() and drain_obj_stock()
which both check stock->cached one more time before the deref.

With workqueue managing concurrency, and rcu ensuring memcg lifetime
during the drain, this lock indeed seems unnecessary now.

Unless I'm missing something, it should just be removed instead.

  parent reply	other threads:[~2021-08-03 14:15 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-29 12:57 [PATCH 0/5] Cleanups and fixup for memcontrol Miaohe Lin
2021-07-29 12:57 ` [PATCH 1/5] mm, memcg: remove unused functions Miaohe Lin
2021-07-29 14:07   ` Shakeel Butt
2021-07-29 14:07     ` Shakeel Butt
2021-07-30  2:39   ` Muchun Song
2021-07-30  2:39     ` Muchun Song
2021-07-30  2:57   ` Roman Gushchin
2021-07-30  6:45   ` Michal Hocko
2021-07-29 12:57 ` [PATCH 2/5] mm, memcg: narrow the scope of percpu_charge_mutex Miaohe Lin
2021-07-30  2:42   ` Muchun Song
2021-07-30  2:42     ` Muchun Song
2021-07-30  3:06   ` Roman Gushchin
2021-07-30  6:50     ` Michal Hocko
2021-07-31  2:29       ` Miaohe Lin
2021-08-02  6:49         ` Michal Hocko
2021-08-02  9:54           ` Miaohe Lin
2021-08-03  3:40         ` Roman Gushchin
2021-08-03  6:29           ` Miaohe Lin
2021-08-03  7:11             ` Michal Hocko
2021-08-03  7:13               ` Roman Gushchin
2021-08-03  7:27                 ` Michal Hocko
2021-08-03  9:33             ` Muchun Song
2021-08-03  9:33               ` Muchun Song
2021-08-03 10:50               ` Miaohe Lin
2021-08-03 14:15       ` Johannes Weiner [this message]
2021-08-04  8:20         ` Michal Hocko
2021-08-05  1:44           ` Miaohe Lin
2021-07-30  6:46   ` Michal Hocko
2021-07-29 12:57 ` [PATCH 3/5] mm, memcg: save some atomic ops when flush is already true Miaohe Lin
2021-07-29 14:40   ` Shakeel Butt
2021-07-29 14:40     ` Shakeel Butt
2021-07-30  2:37   ` Muchun Song
2021-07-30  2:37     ` Muchun Song
2021-07-30  3:07   ` Roman Gushchin
2021-07-30  6:51   ` Michal Hocko
2021-07-29 12:57 ` [PATCH 4/5] mm, memcg: avoid possible NULL pointer dereferencing in mem_cgroup_init() Miaohe Lin
2021-07-29 13:52   ` Matthew Wilcox
2021-07-30  1:50     ` Miaohe Lin
2021-07-30  3:12   ` Roman Gushchin
2021-07-30  6:29     ` Miaohe Lin
2021-07-30  6:44     ` Michal Hocko
2021-07-31  2:05       ` Miaohe Lin
2021-08-02  6:43         ` Michal Hocko
2021-08-02 10:00           ` Miaohe Lin
2021-08-02 10:42             ` Michal Hocko
2021-08-02 11:18               ` Miaohe Lin
2021-07-29 12:57 ` [PATCH 5/5] mm, memcg: always call __mod_node_page_state() with preempt disabled Miaohe Lin
2021-07-29 14:39   ` Shakeel Butt
2021-07-29 14:39     ` Shakeel Butt
2021-07-30  1:52     ` Miaohe Lin
2021-07-30  2:33       ` [External] " Muchun Song
2021-07-30  2:33         ` Muchun Song
2021-07-30  2:46         ` Miaohe Lin

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=YQlPiLY0ieRb704V@cmpxchg.org \
    --to=hannes@cmpxchg.org \
    --cc=akpm@linux-foundation.org \
    --cc=alexs@kernel.org \
    --cc=cgroups@vger.kernel.org \
    --cc=guro@fb.com \
    --cc=linmiaohe@huawei.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mhocko@suse.com \
    --cc=richard.weiyang@gmail.com \
    --cc=shakeelb@google.com \
    --cc=songmuchun@bytedance.com \
    --cc=vdavydov.dev@gmail.com \
    --cc=willy@infradead.org \
    --subject='Re: [PATCH 2/5] mm, memcg: narrow the scope of percpu_charge_mutex' \
    /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 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.