Re: [External] Re: [PATCH v2] mm/vmscan: fix infinite loop in drop_slab_node

[Chunxin Zang <zangchunxin@bytedance.com>]

[-- Attachment #1: Type: text/plain, Size: 4430 bytes --]

On Mon, Sep 14, 2020 at 5:30 PM Michal Hocko <mhocko@suse.com> wrote:

> The subject is misleading because this patch doesn't fix an infinite
> loop, right? It just allows the userspace to interrupt the operation.
>
>
Yes,  so we are making a separate patch follow Vlastimil's recommendations.
Use
double of threshold to end the loop.

On Thu, Sep 10, 2020 at 1:59 AM Vlastimil Babka <vbabka@suse.cz> wrote:
> > From: Chunxin Zang <zangchunxin@bytedance.com>
> >
> ...
> - IMHO it's still worth to bail out in your scenario even without a
> signal, e.g.
> by the doubling of threshold. But it can be a separate patch.
> Thanks!
> ...



On Wed 09-09-20 23:20:47, zangchunxin@bytedance.com wrote:
> > From: Chunxin Zang <zangchunxin@bytedance.com>
> >
> > On our server, there are about 10k memcg in one machine. They use memory
> > very frequently. When I tigger drop caches，the process will infinite loop
> > in drop_slab_node.
>
> Is this really an infinite loop, or it just takes a lot of time to
> process all the metadata in that setup? If this is really an infinite
> loop then we should look at it. My current understanding is that the
> operation would finish at some time it just takes painfully long to get
> there.
>

Yes,  it's really an infinite loop.  Every loop spends a lot of time. In
this time,
memcg will alloc/free memory,  so the next loop, the total of  'freed'
always bigger than 10.


> > There are two reasons:
> > 1.We have too many memcgs, even though one object freed in one memcg, the
> >   sum of object is bigger than 10.
> >
> > 2.We spend a lot of time in traverse memcg once. So, the memcg who
> >   traversed at the first have been freed many objects. Traverse memcg
> next
> >   time, the freed count bigger than 10 again.
> >
> > We can get the following info through 'ps':
> >
> >   root:~# ps -aux | grep drop
> >   root  357956 ... R    Aug25 21119854:55 echo 3 >
> /proc/sys/vm/drop_caches
> >   root 1771385 ... R    Aug16 21146421:17 echo 3 >
> /proc/sys/vm/drop_caches
> >   root 1986319 ... R    18:56 117:27 echo 3 > /proc/sys/vm/drop_caches
> >   root 2002148 ... R    Aug24 5720:39 echo 3 > /proc/sys/vm/drop_caches
> >   root 2564666 ... R    18:59 113:58 echo 3 > /proc/sys/vm/drop_caches
> >   root 2639347 ... R    Sep03 2383:39 echo 3 > /proc/sys/vm/drop_caches
> >   root 3904747 ... R    03:35 993:31 echo 3 > /proc/sys/vm/drop_caches
> >   root 4016780 ... R    Aug21 7882:18 echo 3 > /proc/sys/vm/drop_caches
> >
> > Use bpftrace follow 'freed' value in drop_slab_node:
> >
> >   root:~# bpftrace -e 'kprobe:drop_slab_node+70 {@ret=hist(reg("bp")); }'
> >   Attaching 1 probe...
> >   ^B^C
> >
> >   @ret:
> >   [64, 128)        1 |
>   |
> >   [128, 256)      28 |
>   |
> >   [256, 512)     107 |@
>  |
> >   [512, 1K)      298 |@@@
>  |
> >   [1K, 2K)       613 |@@@@@@@
>  |
> >   [2K, 4K)      4435
> |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|
> >   [4K, 8K)       442 |@@@@@
>  |
> >   [8K, 16K)      299 |@@@
>  |
> >   [16K, 32K)     100 |@
>  |
> >   [32K, 64K)     139 |@
>  |
> >   [64K, 128K)     56 |
>   |
> >   [128K, 256K)    26 |
>   |
> >   [256K, 512K)     2 |
>   |
> >
> > In the while loop, we can check whether the TASK_KILLABLE signal is set,
> > if so, we should break the loop.
>
> I would make it explicit that this is not fixing the above scenario. It
> just helps to cancel to operation which is a good thing in general.
>
> > Signed-off-by: Chunxin Zang <zangchunxin@bytedance.com>
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>
> With updated changelog
> Acked-by: Michal Hocko <mhocko@suse.com>
>
> > ---
> >       changelogs in v2:
> >       1) Via check TASK_KILLABLE signal break loop.
> >
> >  mm/vmscan.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index b6d84326bdf2..c3ed8b45d264 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -704,6 +704,9 @@ void drop_slab_node(int nid)
> >       do {
> >               struct mem_cgroup *memcg = NULL;
> >
> > +             if (fatal_signal_pending(current))
> > +                     return;
> > +
> >               freed = 0;
> >               memcg = mem_cgroup_iter(NULL, NULL, NULL);
> >               do {
> > --
> > 2.11.0
> >
>
> --
> Best wishes
> Chunxin
>

[-- Attachment #2: Type: text/html, Size: 7231 bytes --]