Re: [PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc

From: Roman Gushchin <roman.gushchin@linux.dev>
To: Vasily Averin <vvs@openvz.org>
Cc: Shakeel Butt <shakeelb@google.com>,
	Johannes Weiner <hannes@cmpxchg.org>,
	kernel@openvz.org, LKML <linux-kernel@vger.kernel.org>,
	Matthew Wilcox <willy@infradead.org>,
	Cgroups <cgroups@vger.kernel.org>
Subject: Re: [PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc
Date: Fri, 27 May 2022 10:47:46 -0700	[thread overview]
Message-ID: <YpEOwrBe2UzJZr9E@carbon> (raw)
In-Reply-To: <d4e81087-d057-4edb-5df0-47d99a1c72d9@openvz.org>

On Fri, May 27, 2022 at 02:22:19PM +0300, Vasily Averin wrote:
> On 5/27/22 04:40, Shakeel Butt wrote:
> > On Thu, May 26, 2022 at 6:21 PM Matthew Wilcox <willy@infradead.org> wrote:
> >>
> >> On Wed, May 25, 2022 at 11:26:37AM +0300, Vasily Averin wrote:
> >>> Commit 7b785645e8f1 ("mm: fix page cache convergence regression")
> >>> added support of new XA_FLAGS_ACCOUNT flag into all Xarray allocation
> >>> functions. Later commit 8fc75643c5e1 ("XArray: add xas_split")
> >>> introduced xas_split_alloc() but missed about XA_FLAGS_ACCOUNT
> >>> processing.
> >>
> >> Thanks, Vasily.
> >>
> >> Johannes, Shakeel, is this right?  I don't fully understand the accounting
> >> stuff.
> >>
> > 
> > If called from __filemap_add_folio() then this is correct.
> > 
> > However from split_huge_page_to_list(), we can not use the memcg from
> > current as that codepath is called from reclaim which can be triggered
> > by processes of other memcgs.
> Btw, Shakeel, Johannes,
> I would like to understand, when Xarray should use XA_FLAGS_ACCOUNT ?
> 
> From my point of view, this should be useless:
> a) if Xarray stores some index (idr?) - his memory is quite small,
> and his accounting can be ignored.
> b) if Xarray stores some accounted - the size of the corresponding Xarray
> infrastructure is usually significantly smaller than the size of the stored object,
> sо his accounting can be skipped too.
> c) if Xarray stores some non-accounted objects - it makes no sense to account 
> corresponding Xarray infrastructure. In case of necessary it makes much more sense
> to enable accounting for stored objects (and return to case b).
> 
> Am I missed something important perhaps?
> 
> I looked for the description of 7b785645e8f1, but o be honest I'm still not sure
> that I understand correctly why XA_FLAGS_ACCOUNT flag solved the described problem.
> 
> Could you please explain this in more details?
> 
> Was it because the non-accounted Xarray kept a reference to the stored object
> and thus prevents it from being reclaimed?
> 
> If so, was it some special case, or should it affect all such cases,
> and my b) statement above is not correct?

It's all about shadow entries, which are small, so b) is not true for them.
There is a good description on how it works on top of mm/workingset.c