[lkml]   [2020]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] mm: list_lru: hold nlru lock to avoid reading transient negative nr_items
    On 30.11.2020 23:09, Roman Gushchin wrote:
    > On Mon, Nov 30, 2020 at 10:45:14AM -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
    >> list_lru_del()
    >> reparent again
    >> dst->nr_items may go negative
    >> due to current 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.
    >> So, it sounds acceptable to synchronize reading nr_items to avoid seeing
    >> intermediate negative nr_items given the simplicity and it is typically
    >> just called by shrinkers when counting the freeable objects.
    >> The patch is tested with some shrinker intensive workloads, no
    >> noticeable regression is soptted.
    > Hi Yang!
    > It's really tricky, thank you for digging in! It's a perfect analysis!
    > I wonder though, if it's better to just always set the shrinker bit on reparenting
    > if we do reparent some items? Then we'll avoid adding new synchronization
    > to the hot path. What do you think?
    > --
    > @@ -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,9 +545,8 @@ 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)
    > memcg_set_shrinker_bit(dst_memcg, nid, lru_shrinker_id(lru));
    > src->nr_items = 0;

    This looks like a good fix.

    To make a code more clear, we may also want to group neighbouring lines
    under the same "if" branch in Yang's v2 resend.


     \ /
      Last update: 2020-12-01 11:29    [W:2.265 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site