* [v2 PATCH] mm: list_lru: set shrinker map bit when child nr_items is not zero
@ 2020-12-01 17:44 Yang Shi
2020-12-01 19:52 ` Roman Gushchin
0 siblings, 1 reply; 3+ messages in thread
From: Yang Shi @ 2020-12-01 17:44 UTC (permalink / raw)
To: guro, ktkhai, vdavydov.dev, shakeelb, akpm
Cc: shy828301, linux-mm, linux-kernel, stable
When investigating a slab cache bloat problem, significant amount of
negative dentry cache was seen, but confusingly they neither got shrunk
by reclaimer (the host has very tight memory) nor be shrunk by dropping
cache. The vmcore shows there are over 14M negative dentry objects on lru,
but tracing result shows they were even not scanned at all. The further
investigation shows the memcg's vfs shrinker_map bit is not set. So the
reclaimer or dropping cache just skip calling vfs shrinker. So we have
to reboot the hosts to get the memory back.
I didn't manage to come up with a reproducer in test environment, and the
problem can't be reproduced after rebooting. But it seems there is race
between shrinker map bit clear and reparenting by code inspection. The
hypothesis is elaborated as below.
The memcg hierarchy on our production environment looks like:
root
/ \
system user
The main workloads are running under user slice's children, and it creates
and removes memcg frequently. So reparenting happens very often under user
slice, but no task is under user slice directly.
So with the frequent reparenting and tight memory pressure, the below
hypothetical race condition may happen:
CPU A CPU B CPU C
reparent
dst->nr_items == 0
shrinker:
total_objects == 0
add src->nr_items to dst
set_bit
retrun SHRINK_EMPTY
clear_bit
child memcg offline
replace child's kmemcg_id to
parent's (in memcg_offline_kmem())
list_lru_del()
see parent's kmemcg_id
dec dst->nr_items
reparent again
dst->nr_items may go negative
due to concurrent list_lru_del()
on CPU C
The second run of shrinker:
read nr_items without any
synchronization, so it may
see intermediate negative
nr_items then total_objects
may return 0 conincidently
keep the bit cleared
dst->nr_items != 0
skip set_bit
add scr->nr_item to dst
After this point dst->nr_item may never go zero, so reparenting will not
set shrinker_map bit anymore. And since there is no task under user
slice directly, so no new object will be added to its lru to set the
shrinker map bit either. That bit is kept cleared forever.
How does list_lru_del() race with reparenting? It is because
reparenting replaces childen's kmemcg_id to parent's without protecting
from nlru->lock, so list_lru_del() may see parent's kmemcg_id but
actually deleting items from child's lru, but dec'ing parent's nr_items,
so the parent's nr_items may go negative as commit
2788cf0c401c268b4819c5407493a8769b7007aa ("memcg: reparent list_lrus and
free kmemcg_id on css offline") says.
Can we move kmemcg_id replacement after reparenting? No, because the
race with list_lru_del() may result in negative src->nr_items, but it
will never be fixed. So the shrinker may never return SHRINK_EMPTY then
keep the shrinker map bit set always. The shrinker will be always
called for nonsense.
Can we synchronize list_lru_del() and reparenting? Yes, it could be
done. But it seems we need introduce a new lock or use nlru->lock. But
it sounds complicated to move kmemcg_id replacement code under nlru->lock.
And list_lru_del() may be called quite often to exacerbate some hot
path, i.e. dentry kill.
Since it is impossible that dst->nr_items goes negative and
src->nr_items goes zero at the same time, so it seems we could set the
shrinker map bit iff src->nr_items != 0. We could synchronize
list_lru_count_one() and reparenting with nlru->lock, but it seems
checking src->nr_items in reparenting is the simplest and avoids lock
contention.
Fixes: fae91d6d8be5 ("mm/list_lru.c: set bit in memcg shrinker bitmap on first list_lru item appearance")
Suggested-by: Roman Gushchin <guro@fb.com>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
Cc: Shakeel Butt <shakeelb@google.com>
Cc: <stable@vger.kernel.org> v4.19+
Signed-off-by: Yang Shi <shy828301@gmail.com>
---
v2: * Incorporated Roman's suggestion
* Incorporated Kirill's suggestion
* Changed the subject of patch to get align with the new fix
* Added fixes tag
mm/list_lru.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/mm/list_lru.c b/mm/list_lru.c
index 5aa6e44bc2ae..fe230081690b 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -534,7 +534,6 @@ static void memcg_drain_list_lru_node(struct list_lru *lru, int nid,
struct list_lru_node *nlru = &lru->node[nid];
int dst_idx = dst_memcg->kmemcg_id;
struct list_lru_one *src, *dst;
- bool set;
/*
* Since list_lru_{add,del} may be called under an IRQ-safe lock,
@@ -546,11 +545,12 @@ static void memcg_drain_list_lru_node(struct list_lru *lru, int nid,
dst = list_lru_from_memcg_idx(nlru, dst_idx);
list_splice_init(&src->list, &dst->list);
- set = (!dst->nr_items && src->nr_items);
- dst->nr_items += src->nr_items;
- if (set)
+
+ if (src->nr_items) {
+ dst->nr_items += src->nr_items;
memcg_set_shrinker_bit(dst_memcg, nid, lru_shrinker_id(lru));
- src->nr_items = 0;
+ src->nr_items = 0;
+ }
spin_unlock_irq(&nlru->lock);
}
--
2.26.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [v2 PATCH] mm: list_lru: set shrinker map bit when child nr_items is not zero
2020-12-01 17:44 [v2 PATCH] mm: list_lru: set shrinker map bit when child nr_items is not zero Yang Shi
@ 2020-12-01 19:52 ` Roman Gushchin
2020-12-01 20:12 ` Yang Shi
0 siblings, 1 reply; 3+ messages in thread
From: Roman Gushchin @ 2020-12-01 19:52 UTC (permalink / raw)
To: Yang Shi
Cc: ktkhai, vdavydov.dev, shakeelb, akpm, linux-mm, linux-kernel, stable
On Tue, Dec 01, 2020 at 09:44:49AM -0800, Yang Shi wrote:
> When investigating a slab cache bloat problem, significant amount of
> negative dentry cache was seen, but confusingly they neither got shrunk
> by reclaimer (the host has very tight memory) nor be shrunk by dropping
> cache. The vmcore shows there are over 14M negative dentry objects on lru,
> but tracing result shows they were even not scanned at all. The further
> investigation shows the memcg's vfs shrinker_map bit is not set. So the
> reclaimer or dropping cache just skip calling vfs shrinker. So we have
> to reboot the hosts to get the memory back.
>
> I didn't manage to come up with a reproducer in test environment, and the
> problem can't be reproduced after rebooting. But it seems there is race
> between shrinker map bit clear and reparenting by code inspection. The
> hypothesis is elaborated as below.
>
> The memcg hierarchy on our production environment looks like:
> root
> / \
> system user
>
> The main workloads are running under user slice's children, and it creates
> and removes memcg frequently. So reparenting happens very often under user
> slice, but no task is under user slice directly.
>
> So with the frequent reparenting and tight memory pressure, the below
> hypothetical race condition may happen:
>
> CPU A CPU B CPU C
> reparent
> dst->nr_items == 0
> shrinker:
> total_objects == 0
> add src->nr_items to dst
> set_bit
> retrun SHRINK_EMPTY
> clear_bit
> child memcg offline
> replace child's kmemcg_id to
> parent's (in memcg_offline_kmem())
> list_lru_del()
> see parent's kmemcg_id
> dec dst->nr_items
> reparent again
> dst->nr_items may go negative
> due to concurrent list_lru_del()
> on CPU C
> The second run of shrinker:
> read nr_items without any
> synchronization, so it may
> see intermediate negative
> nr_items then total_objects
> may return 0 conincidently
>
> keep the bit cleared
> dst->nr_items != 0
> skip set_bit
> add scr->nr_item to dst
>
> After this point dst->nr_item may never go zero, so reparenting will not
> set shrinker_map bit anymore. And since there is no task under user
> slice directly, so no new object will be added to its lru to set the
> shrinker map bit either. That bit is kept cleared forever.
>
> How does list_lru_del() race with reparenting? It is because
> reparenting replaces childen's kmemcg_id to parent's without protecting
> from nlru->lock, so list_lru_del() may see parent's kmemcg_id but
> actually deleting items from child's lru, but dec'ing parent's nr_items,
> so the parent's nr_items may go negative as commit
> 2788cf0c401c268b4819c5407493a8769b7007aa ("memcg: reparent list_lrus and
> free kmemcg_id on css offline") says.
>
> Can we move kmemcg_id replacement after reparenting? No, because the
> race with list_lru_del() may result in negative src->nr_items, but it
> will never be fixed. So the shrinker may never return SHRINK_EMPTY then
> keep the shrinker map bit set always. The shrinker will be always
> called for nonsense.
>
> Can we synchronize list_lru_del() and reparenting? Yes, it could be
> done. But it seems we need introduce a new lock or use nlru->lock. But
> it sounds complicated to move kmemcg_id replacement code under nlru->lock.
> And list_lru_del() may be called quite often to exacerbate some hot
> path, i.e. dentry kill.
>
> Since it is impossible that dst->nr_items goes negative and
> src->nr_items goes zero at the same time, so it seems we could set the
> shrinker map bit iff src->nr_items != 0. We could synchronize
> list_lru_count_one() and reparenting with nlru->lock, but it seems
> checking src->nr_items in reparenting is the simplest and avoids lock
> contention.
>
> Fixes: fae91d6d8be5 ("mm/list_lru.c: set bit in memcg shrinker bitmap on first list_lru item appearance")
> Suggested-by: Roman Gushchin <guro@fb.com>
> Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
> Cc: Shakeel Butt <shakeelb@google.com>
> Cc: <stable@vger.kernel.org> v4.19+
> Signed-off-by: Yang Shi <shy828301@gmail.com>
Hi Yang!
Code-wise it looks good to me. Thank you for updating!
I think the commit log can be simplified a bit: you don't really need 3 CPUs
to reproduce the problem. Also, IMO the section about fixing the problem by
introducing an additional synchronization can be dropped, but it's up to you.
With the updated commit log, please feel to add
Reviewed-by: Roman Gushchin <guro@fb.com> .
Thank you!
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [v2 PATCH] mm: list_lru: set shrinker map bit when child nr_items is not zero
2020-12-01 19:52 ` Roman Gushchin
@ 2020-12-01 20:12 ` Yang Shi
0 siblings, 0 replies; 3+ messages in thread
From: Yang Shi @ 2020-12-01 20:12 UTC (permalink / raw)
To: Roman Gushchin
Cc: Kirill Tkhai, Vladimir Davydov, Shakeel Butt, Andrew Morton,
Linux MM, Linux Kernel Mailing List, stable
On Tue, Dec 1, 2020 at 11:52 AM Roman Gushchin <guro@fb.com> wrote:
>
> On Tue, Dec 01, 2020 at 09:44:49AM -0800, Yang Shi wrote:
> > When investigating a slab cache bloat problem, significant amount of
> > negative dentry cache was seen, but confusingly they neither got shrunk
> > by reclaimer (the host has very tight memory) nor be shrunk by dropping
> > cache. The vmcore shows there are over 14M negative dentry objects on lru,
> > but tracing result shows they were even not scanned at all. The further
> > investigation shows the memcg's vfs shrinker_map bit is not set. So the
> > reclaimer or dropping cache just skip calling vfs shrinker. So we have
> > to reboot the hosts to get the memory back.
> >
> > I didn't manage to come up with a reproducer in test environment, and the
> > problem can't be reproduced after rebooting. But it seems there is race
> > between shrinker map bit clear and reparenting by code inspection. The
> > hypothesis is elaborated as below.
> >
> > The memcg hierarchy on our production environment looks like:
> > root
> > / \
> > system user
> >
> > The main workloads are running under user slice's children, and it creates
> > and removes memcg frequently. So reparenting happens very often under user
> > slice, but no task is under user slice directly.
> >
> > So with the frequent reparenting and tight memory pressure, the below
> > hypothetical race condition may happen:
> >
> > CPU A CPU B CPU C
> > reparent
> > dst->nr_items == 0
> > shrinker:
> > total_objects == 0
> > add src->nr_items to dst
> > set_bit
> > retrun SHRINK_EMPTY
> > clear_bit
> > child memcg offline
> > replace child's kmemcg_id to
> > parent's (in memcg_offline_kmem())
> > list_lru_del()
> > see parent's kmemcg_id
> > dec dst->nr_items
> > reparent again
> > dst->nr_items may go negative
> > due to concurrent list_lru_del()
> > on CPU C
> > The second run of shrinker:
> > read nr_items without any
> > synchronization, so it may
> > see intermediate negative
> > nr_items then total_objects
> > may return 0 conincidently
> >
> > keep the bit cleared
> > dst->nr_items != 0
> > skip set_bit
> > add scr->nr_item to dst
> >
> > After this point dst->nr_item may never go zero, so reparenting will not
> > set shrinker_map bit anymore. And since there is no task under user
> > slice directly, so no new object will be added to its lru to set the
> > shrinker map bit either. That bit is kept cleared forever.
> >
> > How does list_lru_del() race with reparenting? It is because
> > reparenting replaces childen's kmemcg_id to parent's without protecting
> > from nlru->lock, so list_lru_del() may see parent's kmemcg_id but
> > actually deleting items from child's lru, but dec'ing parent's nr_items,
> > so the parent's nr_items may go negative as commit
> > 2788cf0c401c268b4819c5407493a8769b7007aa ("memcg: reparent list_lrus and
> > free kmemcg_id on css offline") says.
> >
> > Can we move kmemcg_id replacement after reparenting? No, because the
> > race with list_lru_del() may result in negative src->nr_items, but it
> > will never be fixed. So the shrinker may never return SHRINK_EMPTY then
> > keep the shrinker map bit set always. The shrinker will be always
> > called for nonsense.
> >
> > Can we synchronize list_lru_del() and reparenting? Yes, it could be
> > done. But it seems we need introduce a new lock or use nlru->lock. But
> > it sounds complicated to move kmemcg_id replacement code under nlru->lock.
> > And list_lru_del() may be called quite often to exacerbate some hot
> > path, i.e. dentry kill.
> >
> > Since it is impossible that dst->nr_items goes negative and
> > src->nr_items goes zero at the same time, so it seems we could set the
> > shrinker map bit iff src->nr_items != 0. We could synchronize
> > list_lru_count_one() and reparenting with nlru->lock, but it seems
> > checking src->nr_items in reparenting is the simplest and avoids lock
> > contention.
> >
> > Fixes: fae91d6d8be5 ("mm/list_lru.c: set bit in memcg shrinker bitmap on first list_lru item appearance")
> > Suggested-by: Roman Gushchin <guro@fb.com>
> > Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
> > Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
> > Cc: Shakeel Butt <shakeelb@google.com>
> > Cc: <stable@vger.kernel.org> v4.19+
> > Signed-off-by: Yang Shi <shy828301@gmail.com>
>
> Hi Yang!
>
> Code-wise it looks good to me. Thank you for updating!
>
> I think the commit log can be simplified a bit: you don't really need 3 CPUs
> to reproduce the problem. Also, IMO the section about fixing the problem by
> introducing an additional synchronization can be dropped, but it's up to you.
Yes, don't have to have 3 CPUs. Will simplify the commit log.
>
> With the updated commit log, please feel to add
> Reviewed-by: Roman Gushchin <guro@fb.com> .
Thanks!
>
> Thank you!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-12-01 20:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-01 17:44 [v2 PATCH] mm: list_lru: set shrinker map bit when child nr_items is not zero Yang Shi
2020-12-01 19:52 ` Roman Gushchin
2020-12-01 20:12 ` Yang Shi
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).