Message-ID: <20230706185436.agobbv72o3hma43z@revolver>
Date:   Thu, 6 Jul 2023 14:54:36 -0400
From:   "Liam R. Howlett" <Liam.Howlett@...cle.com>
To:     John Hsu (許永翰) <John.Hsu@...iatek.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


Apologies for the late response.

* John Hsu (許永翰) <John.Hsu@...iatek.com> [230616 05:19]:
> 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?

What I meant is that both methods use the same call path because there
is not a reason to duplicate the path.  After mas_preallocate() has
allocated the nodes needed, the call to check if there is enough nodes
will be quick.

> 
> > > 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.

The root cause is that there was not enough memory for a store
operation.  Regardless of if we check the allocations in the
mas_store_prealloc() path or not, this would fail.  If we remove the
check for nodes within this path, then we would have to BUG_ON() when we
run out of nodes to use or have a null pointer dereference BUG anyways.

> 
> > > 
> > > 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?

Sorry, I don't understand what you mean by '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.

Which BUG_ON() are you referring to?

If I was to separate the code path for mas_store_prealloc() and
mas_store_gfp(), then a BUG_ON() would still need to exist and still
would have been triggered..  We are in a place in the code where we
should never sleep and we don't have enough memory allocated to do what
was necessary.

Thanks,
Liam

Powered by blists - more mailing lists