[PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc

[PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc

[Vasily Averin]

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.

Signed-off-by: Vasily Averin <vvs@openvz.org>
---
 lib/xarray.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/xarray.c b/lib/xarray.c
index 54e646e8e6ee..5f5b42e6f842 100644
--- a/lib/xarray.c
+++ b/lib/xarray.c
@@ -1013,6 +1013,8 @@ void xas_split_alloc(struct xa_state *xas, void *entry, unsigned int order,
 	if (xas->xa_shift + XA_CHUNK_SHIFT > order)
 		return;
 
+	if (xas->xa->xa_flags & XA_FLAGS_ACCOUNT)
+		gfp |= __GFP_ACCOUNT;
 	do {
 		unsigned int i;
 		void *sibling = NULL;
-- 
2.31.1

Re: [PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc

[Matthew Wilcox]

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.

> Signed-off-by: Vasily Averin <vvs@openvz.org>
> ---
>  lib/xarray.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/xarray.c b/lib/xarray.c
> index 54e646e8e6ee..5f5b42e6f842 100644
> --- a/lib/xarray.c
> +++ b/lib/xarray.c
> @@ -1013,6 +1013,8 @@ void xas_split_alloc(struct xa_state *xas, void *entry, unsigned int order,
>  	if (xas->xa_shift + XA_CHUNK_SHIFT > order)
>  		return;
>  
> +	if (xas->xa->xa_flags & XA_FLAGS_ACCOUNT)
> +		gfp |= __GFP_ACCOUNT;
>  	do {
>  		unsigned int i;
>  		void *sibling = NULL;
> -- 
> 2.31.1
> 

Re: [PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc

[Shakeel Butt]

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.

> > Signed-off-by: Vasily Averin <vvs@openvz.org>
> > ---
> >  lib/xarray.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/lib/xarray.c b/lib/xarray.c
> > index 54e646e8e6ee..5f5b42e6f842 100644
> > --- a/lib/xarray.c
> > +++ b/lib/xarray.c
> > @@ -1013,6 +1013,8 @@ void xas_split_alloc(struct xa_state *xas, void *entry, unsigned int order,
> >       if (xas->xa_shift + XA_CHUNK_SHIFT > order)
> >               return;
> >
> > +     if (xas->xa->xa_flags & XA_FLAGS_ACCOUNT)
> > +             gfp |= __GFP_ACCOUNT;
> >       do {
> >               unsigned int i;
> >               void *sibling = NULL;
> > --
> > 2.31.1
> >

Re: [PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc

[Vasily Averin]

On 5/27/22 04:40, Shakeel Butt wrote:
> On Thu, May 26, 2022 at 6:21 PM Matthew Wilcox <willy-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.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?

Thank you,
	Vasily Averin

Re: [PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc

[Vasily Averin]

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?

Thank you,
	Vasily Averin

Re: [PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc

[Roman Gushchin]

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-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.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

Re: [PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc

[Roman Gushchin]

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