Date:   Fri, 16 Jun 2023 09:18:37 +0000
From:   John Hsu (許永翰) <John.Hsu@...iatek.com>
To:     "Liam.Howlett@...cle.com" <Liam.Howlett@...cle.com>
CC:     Andrew Yang (楊智強) 
        <Andrew.Yang@...iatek.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Qun-wei Lin (林群崴) 
        <Qun-wei.Lin@...iatek.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        Chinwen Chang (張錦文) 
        <chinwen.chang@...iatek.com>,
        Kuan-Ying Lee (李冠穎) 
        <Kuan-Ying.Lee@...iatek.com>,
        Casper Li (李中榮) <casper.li@...iatek.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "yuzhao@...gle.com" <yuzhao@...gle.com>,
        "maple-tree@...ts.infradead.org" <maple-tree@...ts.infradead.org>
Subject: Re: [BUG] trigger BUG_ON in mas_store_prealloc when low memory

On Wed, 2023-06-14 at 11:58 -0400, Liam R. Howlett wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  * John Hsu (許永翰) <John.Hsu@...iatek.com> [230614 03:06]:
> > Hi Liam, thanks for your reply.
> 
> Sorry, your email response with top posting is hard to follow so I
> will
> do my best to answer your questions.

Sorry for the wrong format....

> > 
> > 
> > 
> > version 6.1 or 6.1.x?  Which exact version (git id or version
> number)
> > 
> > Our environment is kernel-6.1.25-mainline-android14-5-
> gdea04bf2c398d.
> 
> Okay, I can have a look at 6.1.25 then.

OK, thanks.

> > 
> > 
> > This BUG_ON() is necessary since this function should _never_ run
> out of
> > 
> > memory; this function does not return an error code.
> mas_preallocate()
> > 
> > should have gotten you the memory necessary (or returned an
> -ENOMEM)
> > 
> > prior to the call to mas_store_prealloc(), so this is probably an
> > 
> > internal tree problem.
> > 
> > There is a tree operation being performed here.  mprotect is
> merging a
> > 
> > vma by the looks of the call stack.  Why do you think no tree
> operation
> > 
> > is necessary?
> > 
> > As you mentioned, mas_preallocate() should allocate enough node,
> but there is such functions mas_node_count() in mas_store_prealloc().
> > In mas_node_count() checks whether the *mas* has enough nodes, and
> allocate memory for node if there was no enough nodes in mas.
> 
> Right, we call mas_node_count() so that both code paths are used for
> preallocations and regular mas_store()/mas_store_gfp().  It shouldn't
> take a significant amount of time to verify there is enough nodes.

Yap..., it didn't take a significant amount of time to verify whether
there is enough nodes. The problem is why the flow in mas_node_count
will alloc nodes if there was no enough nodes in mas?

> > I think that if mas_preallocate() allocate enough node, why we
> check the node count and allocate nodes if there was no enough nodes
> in mas in mas_node_count()?
> 
> We check for the above reason.
> 

OK..., this is one of the root cause of this BUG.

> > 
> > We have seen that there may be some maple_tree operations in
> merge_vma...
> 
> If merge_vma() does anything, then there was an operation to the
> maple
> tree.
> 
> > 
> > Moreover, would maple_tree provides an API for assigning user's gfp
> flag for allocating node?
> 
> mas_preallocate() and mas_store_gfp() has gfp flags as an
> argument.  In
> your call stack, it will be called in __vma_adjust() as such:
> 
> if (mas_preallocate(&mas, vma, GFP_KERNEL))
> return -ENOMEM;
> 
> line 715 in v6.1.25
> 
> > In rb_tree, we allocate vma_area_struct (rb_node is in this
> struct.) with GFP_KERNEL, and maple_tree allocate node with
> GFP_NOWAIT and __GFP_NOWARN.
> 
> We use GFP_KERNEL as I explained above for the VMA tree.

Got it! But the mas_node_count() always use GFP_NOWAIT and __GFP_NOWARN
in inserting tree flow. Do you consider the performance of maintaining
the structure of maple_tree?

> It also will drop the lock and retry with GFP_KERNEL on failure
> when not using the external lock.  The mmap_lock is configured as an
> external lock.
> 
> > Allocation will not wait for reclaiming and compacting when there
> is no enough available memory.
> > Is there any concern for this design?
> 
> This has been addressed above, but let me know if I missed anything
> here.
> 

I think that the mas_node_count() has higher rate of triggering
BUG_ON() when allocating nodes with GFP_NOWAIT and __GFP_NOWARN. If
mas_node_count() use GFP_KERNEL as mas_preallocate() in the mmap.c, the
allocation fail rate may be lower than use GFP_NOWAIT.

> > 
> > 
> > I see this is arm64.  Do you have a reproducer?  If you don't have
> a
> > 
> > reproducer, I can try stress-ng on amr64 to simulate your workload
> using
> > 
> > mprotect, but I need to know the exact kernel version as this issue
> may
> > 
> > have been fixed in a later stable release.
> > 
> > It is offen occur under low memory condiction. Maybe you can try
> stress-ng on arm64 under high memory stress(e.g. reserved lots of
> memory).
> 
> Okay, I will try arm64 with v6.1.25.

OK, thanks.
> ...
> > > following are the backtrace:
> > 
> > > mas_store_prealloc+0x23c/0x484
> > 
> > > vma_mas_store+0xe4/0x2d0
> > 
> > > __vma_adjust+0xab0/0x1470
> > 
> > > vma_merge+0x5b8/0x5d4
> > 
> > > mprotect_fixup+0x1f4/0x478
> > 
> > > __arm64_sys_mprotect+0x6b0/0x8f0
> > 
> > > invoke_syscall+0x84/0x264
> > 
> > > el0_svc_common+0x118/0x1f0
> > 
> > > do_el0_svc+0x5c/0x184
> > 
> > > el0_svc+0x38/0x98
> > 
> 
> Thanks,
> Liam

BRs,
John Hsu

Powered by blists - more mailing lists