linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc
@ 2022-05-25  8:26 Vasily Averin
  2022-05-27  1:21 ` Matthew Wilcox
  0 siblings, 1 reply; 5+ messages in thread
From: Vasily Averin @ 2022-05-25  8:26 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: kernel, linux-kernel, linux-fsdevel

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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc
  2022-05-25  8:26 [PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc Vasily Averin
@ 2022-05-27  1:21 ` Matthew Wilcox
  2022-05-27  1:40   ` Shakeel Butt
  0 siblings, 1 reply; 5+ messages in thread
From: Matthew Wilcox @ 2022-05-27  1:21 UTC (permalink / raw)
  To: Vasily Averin
  Cc: kernel, linux-kernel, linux-fsdevel, Johannes Weiner, Shakeel Butt

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
> 

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc
  2022-05-27  1:21 ` Matthew Wilcox
@ 2022-05-27  1:40   ` Shakeel Butt
  2022-05-27 11:22     ` Vasily Averin
  0 siblings, 1 reply; 5+ messages in thread
From: Shakeel Butt @ 2022-05-27  1:40 UTC (permalink / raw)
  To: Matthew Wilcox
  Cc: Vasily Averin, kernel, LKML, linux-fsdevel, Johannes Weiner

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc
  2022-05-27  1:40   ` Shakeel Butt
@ 2022-05-27 11:22     ` Vasily Averin
  2022-05-27 17:47       ` Roman Gushchin
  0 siblings, 1 reply; 5+ messages in thread
From: Vasily Averin @ 2022-05-27 11:22 UTC (permalink / raw)
  To: Shakeel Butt, Johannes Weiner
  Cc: kernel, LKML, Matthew Wilcox, Cgroups, Roman Gushchin

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc
  2022-05-27 11:22     ` Vasily Averin
@ 2022-05-27 17:47       ` Roman Gushchin
  0 siblings, 0 replies; 5+ messages in thread
From: Roman Gushchin @ 2022-05-27 17:47 UTC (permalink / raw)
  To: Vasily Averin
  Cc: Shakeel Butt, Johannes Weiner, kernel, LKML, Matthew Wilcox, Cgroups

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-05-27 17:48 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-25  8:26 [PATCH] XArray: handle XA_FLAGS_ACCOUNT in xas_split_alloc Vasily Averin
2022-05-27  1:21 ` Matthew Wilcox
2022-05-27  1:40   ` Shakeel Butt
2022-05-27 11:22     ` Vasily Averin
2022-05-27 17:47       ` Roman Gushchin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).