[PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Yosry Ahmed]

memcg_kmem_uncharge_page() is an inline wrapper around
__memcg_kmem_uncharge_page() that checks memcg_kmem_online() before
making the function call. Internally, __memcg_kmem_uncharge_page() has a
folio_memcg_kmem() check.

The only direct user of __memcg_kmem_uncharge_page(),
free_pages_prepare(), checks PageMemcgKmem() before calling it to avoid
the function call if possible. Move the folio_memcg_kmem() check from
__memcg_kmem_uncharge_page() to memcg_kmem_uncharge_page() as
PageMemcgKmem() -- which does the same thing under the hood. Now
free_pages_prepare() can also use memcg_kmem_uncharge_page().

No functional change intended.

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
---
 include/linux/memcontrol.h | 2 +-
 mm/memcontrol.c            | 3 ---
 mm/page_alloc.c            | 3 +--
 3 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index a308c8eacf20d..2009ca508271a 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -1844,7 +1844,7 @@ static inline int memcg_kmem_charge_page(struct page *page, gfp_t gfp,
 
 static inline void memcg_kmem_uncharge_page(struct page *page, int order)
 {
-	if (memcg_kmem_online())
+	if (memcg_kmem_online() && PageMemcgKmem(page))
 		__memcg_kmem_uncharge_page(page, order);
 }
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 69b0ad4552425..09efbfa2733e8 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3301,9 +3301,6 @@ void __memcg_kmem_uncharge_page(struct page *page, int order)
 	struct obj_cgroup *objcg;
 	unsigned int nr_pages = 1 << order;
 
-	if (!folio_memcg_kmem(folio))
-		return;
-
 	objcg = __folio_objcg(folio);
 	obj_cgroup_uncharge_pages(objcg, nr_pages);
 	folio->memcg_data = 0;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 7ea9c33320bf1..f72693d91ac22 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1086,8 +1086,7 @@ static __always_inline bool free_pages_prepare(struct page *page,
 	trace_mm_page_free(page, order);
 	kmsan_free_page(page, order);
 
-	if (memcg_kmem_online() && PageMemcgKmem(page))
-		__memcg_kmem_uncharge_page(page, order);
+	memcg_kmem_uncharge_page(page, order);
 
 	if (unlikely(PageHWPoison(page)) && !order) {
 		/* Do not let hwpoison pages hit pcplists/buddy */
-- 
2.43.0.472.g3155946c3a-goog

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Muchun Song]

> On Dec 13, 2023, at 21:04, Yosry Ahmed <yosryahmed@google.com> wrote:
> 
> memcg_kmem_uncharge_page() is an inline wrapper around
> __memcg_kmem_uncharge_page() that checks memcg_kmem_online() before
> making the function call. Internally, __memcg_kmem_uncharge_page() has a
> folio_memcg_kmem() check.
> 
> The only direct user of __memcg_kmem_uncharge_page(),
> free_pages_prepare(), checks PageMemcgKmem() before calling it to avoid
> the function call if possible. Move the folio_memcg_kmem() check from
> __memcg_kmem_uncharge_page() to memcg_kmem_uncharge_page() as
> PageMemcgKmem() -- which does the same thing under the hood. Now
> free_pages_prepare() can also use memcg_kmem_uncharge_page().
> 
> No functional change intended.
> 
> Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

Thanks

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Matthew Wilcox]

On Wed, Dec 13, 2023 at 01:04:14PM +0000, Yosry Ahmed wrote:
> memcg_kmem_uncharge_page() is an inline wrapper around
> __memcg_kmem_uncharge_page() that checks memcg_kmem_online() before
> making the function call. Internally, __memcg_kmem_uncharge_page() has a
> folio_memcg_kmem() check.
> 
> The only direct user of __memcg_kmem_uncharge_page(),
> free_pages_prepare(), checks PageMemcgKmem() before calling it to avoid
> the function call if possible. Move the folio_memcg_kmem() check from
> __memcg_kmem_uncharge_page() to memcg_kmem_uncharge_page() as
> PageMemcgKmem() -- which does the same thing under the hood. Now
> free_pages_prepare() can also use memcg_kmem_uncharge_page().

I think you've just pessimised all the other places which call
memcg_kmem_uncharge_page().  It's a matter of probabilities.  In
free_pages_prepare(), most of the pages being freed are not accounted
to memcg.  Whereas in fork() we are absolutely certain that the pages
were accounted because we accounted them.

I think this is a bad change.

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Yosry Ahmed]

On Wed, Dec 13, 2023 at 7:01 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Dec 13, 2023 at 01:04:14PM +0000, Yosry Ahmed wrote:
> > memcg_kmem_uncharge_page() is an inline wrapper around
> > __memcg_kmem_uncharge_page() that checks memcg_kmem_online() before
> > making the function call. Internally, __memcg_kmem_uncharge_page() has a
> > folio_memcg_kmem() check.
> >
> > The only direct user of __memcg_kmem_uncharge_page(),
> > free_pages_prepare(), checks PageMemcgKmem() before calling it to avoid
> > the function call if possible. Move the folio_memcg_kmem() check from
> > __memcg_kmem_uncharge_page() to memcg_kmem_uncharge_page() as
> > PageMemcgKmem() -- which does the same thing under the hood. Now
> > free_pages_prepare() can also use memcg_kmem_uncharge_page().
>
> I think you've just pessimised all the other places which call
> memcg_kmem_uncharge_page().  It's a matter of probabilities.  In
> free_pages_prepare(), most of the pages being freed are not accounted
> to memcg.  Whereas in fork() we are absolutely certain that the pages
> were accounted because we accounted them.

The check was already there for other callers, but it was inside
__memcg_kmem_uncharge_page(). IIUC, the only change for other callers
is an extra call to compound_head(), and they are not hot paths AFAICT
so it shouldn't be noticeable.

Am I missing something? Perhaps your point is about how branch
prediction works across function call boundaries? or is this not about
performance at all?

>
> I think this is a bad change.

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Matthew Wilcox]

On Wed, Dec 13, 2023 at 07:08:52AM -0800, Yosry Ahmed wrote:
> On Wed, Dec 13, 2023 at 7:01 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Wed, Dec 13, 2023 at 01:04:14PM +0000, Yosry Ahmed wrote:
> > > memcg_kmem_uncharge_page() is an inline wrapper around
> > > __memcg_kmem_uncharge_page() that checks memcg_kmem_online() before
> > > making the function call. Internally, __memcg_kmem_uncharge_page() has a
> > > folio_memcg_kmem() check.
> > >
> > > The only direct user of __memcg_kmem_uncharge_page(),
> > > free_pages_prepare(), checks PageMemcgKmem() before calling it to avoid
> > > the function call if possible. Move the folio_memcg_kmem() check from
> > > __memcg_kmem_uncharge_page() to memcg_kmem_uncharge_page() as
> > > PageMemcgKmem() -- which does the same thing under the hood. Now
> > > free_pages_prepare() can also use memcg_kmem_uncharge_page().
> >
> > I think you've just pessimised all the other places which call
> > memcg_kmem_uncharge_page().  It's a matter of probabilities.  In
> > free_pages_prepare(), most of the pages being freed are not accounted
> > to memcg.  Whereas in fork() we are absolutely certain that the pages
> > were accounted because we accounted them.
> 
> The check was already there for other callers, but it was inside
> __memcg_kmem_uncharge_page(). IIUC, the only change for other callers
> is an extra call to compound_head(), and they are not hot paths AFAICT
> so it shouldn't be noticeable.

How can you seriously claim that fork() is not a hot path?

> Am I missing something? Perhaps your point is about how branch
> prediction works across function call boundaries? or is this not about
> performance at all?
> 
> >
> > I think this is a bad change.

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Yosry Ahmed]

On Wed, Dec 13, 2023 at 7:38 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Dec 13, 2023 at 07:08:52AM -0800, Yosry Ahmed wrote:
> > On Wed, Dec 13, 2023 at 7:01 AM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 01:04:14PM +0000, Yosry Ahmed wrote:
> > > > memcg_kmem_uncharge_page() is an inline wrapper around
> > > > __memcg_kmem_uncharge_page() that checks memcg_kmem_online() before
> > > > making the function call. Internally, __memcg_kmem_uncharge_page() has a
> > > > folio_memcg_kmem() check.
> > > >
> > > > The only direct user of __memcg_kmem_uncharge_page(),
> > > > free_pages_prepare(), checks PageMemcgKmem() before calling it to avoid
> > > > the function call if possible. Move the folio_memcg_kmem() check from
> > > > __memcg_kmem_uncharge_page() to memcg_kmem_uncharge_page() as
> > > > PageMemcgKmem() -- which does the same thing under the hood. Now
> > > > free_pages_prepare() can also use memcg_kmem_uncharge_page().
> > >
> > > I think you've just pessimised all the other places which call
> > > memcg_kmem_uncharge_page().  It's a matter of probabilities.  In
> > > free_pages_prepare(), most of the pages being freed are not accounted
> > > to memcg.  Whereas in fork() we are absolutely certain that the pages
> > > were accounted because we accounted them.
> >
> > The check was already there for other callers, but it was inside
> > __memcg_kmem_uncharge_page(). IIUC, the only change for other callers
> > is an extra call to compound_head(), and they are not hot paths AFAICT
> > so it shouldn't be noticeable.
>
> How can you seriously claim that fork() is not a hot path?

It's only called in fork() when an error happens. It's normally called
when a process is exiting.

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Matthew Wilcox]

On Wed, Dec 13, 2023 at 07:42:44AM -0800, Yosry Ahmed wrote:
> On Wed, Dec 13, 2023 at 7:38 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Wed, Dec 13, 2023 at 07:08:52AM -0800, Yosry Ahmed wrote:
> > > On Wed, Dec 13, 2023 at 7:01 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > >
> > > > On Wed, Dec 13, 2023 at 01:04:14PM +0000, Yosry Ahmed wrote:
> > > > > memcg_kmem_uncharge_page() is an inline wrapper around
> > > > > __memcg_kmem_uncharge_page() that checks memcg_kmem_online() before
> > > > > making the function call. Internally, __memcg_kmem_uncharge_page() has a
> > > > > folio_memcg_kmem() check.
> > > > >
> > > > > The only direct user of __memcg_kmem_uncharge_page(),
> > > > > free_pages_prepare(), checks PageMemcgKmem() before calling it to avoid
> > > > > the function call if possible. Move the folio_memcg_kmem() check from
> > > > > __memcg_kmem_uncharge_page() to memcg_kmem_uncharge_page() as
> > > > > PageMemcgKmem() -- which does the same thing under the hood. Now
> > > > > free_pages_prepare() can also use memcg_kmem_uncharge_page().
> > > >
> > > > I think you've just pessimised all the other places which call
> > > > memcg_kmem_uncharge_page().  It's a matter of probabilities.  In
> > > > free_pages_prepare(), most of the pages being freed are not accounted
> > > > to memcg.  Whereas in fork() we are absolutely certain that the pages
> > > > were accounted because we accounted them.
> > >
> > > The check was already there for other callers, but it was inside
> > > __memcg_kmem_uncharge_page(). IIUC, the only change for other callers
> > > is an extra call to compound_head(), and they are not hot paths AFAICT
> > > so it shouldn't be noticeable.
> >
> > How can you seriously claim that fork() is not a hot path?
> 
> It's only called in fork() when an error happens. It's normally called
> when a process is exiting.

process exit is also a hot path.  at least, there have been regressions
reported that it's "too slow".

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Yosry Ahmed]

On Wed, Dec 13, 2023 at 8:23 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Dec 13, 2023 at 07:42:44AM -0800, Yosry Ahmed wrote:
> > On Wed, Dec 13, 2023 at 7:38 AM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 07:08:52AM -0800, Yosry Ahmed wrote:
> > > > On Wed, Dec 13, 2023 at 7:01 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > > >
> > > > > On Wed, Dec 13, 2023 at 01:04:14PM +0000, Yosry Ahmed wrote:
> > > > > > memcg_kmem_uncharge_page() is an inline wrapper around
> > > > > > __memcg_kmem_uncharge_page() that checks memcg_kmem_online() before
> > > > > > making the function call. Internally, __memcg_kmem_uncharge_page() has a
> > > > > > folio_memcg_kmem() check.
> > > > > >
> > > > > > The only direct user of __memcg_kmem_uncharge_page(),
> > > > > > free_pages_prepare(), checks PageMemcgKmem() before calling it to avoid
> > > > > > the function call if possible. Move the folio_memcg_kmem() check from
> > > > > > __memcg_kmem_uncharge_page() to memcg_kmem_uncharge_page() as
> > > > > > PageMemcgKmem() -- which does the same thing under the hood. Now
> > > > > > free_pages_prepare() can also use memcg_kmem_uncharge_page().
> > > > >
> > > > > I think you've just pessimised all the other places which call
> > > > > memcg_kmem_uncharge_page().  It's a matter of probabilities.  In
> > > > > free_pages_prepare(), most of the pages being freed are not accounted
> > > > > to memcg.  Whereas in fork() we are absolutely certain that the pages
> > > > > were accounted because we accounted them.
> > > >
> > > > The check was already there for other callers, but it was inside
> > > > __memcg_kmem_uncharge_page(). IIUC, the only change for other callers
> > > > is an extra call to compound_head(), and they are not hot paths AFAICT
> > > > so it shouldn't be noticeable.
> > >
> > > How can you seriously claim that fork() is not a hot path?
> >
> > It's only called in fork() when an error happens. It's normally called
> > when a process is exiting.
>
> process exit is also a hot path.  at least, there have been regressions
> reported that it's "too slow".

I doubt an extra compound_head() will matter in that path, but if you
feel strongly about it that's okay. It's a nice cleanup that's all.

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Matthew Wilcox]

On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote:
> I doubt an extra compound_head() will matter in that path, but if you
> feel strongly about it that's okay. It's a nice cleanup that's all.

i don't even understand why you think it's a nice cleanup.

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Yosry Ahmed]

On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote:
> > I doubt an extra compound_head() will matter in that path, but if you
> > feel strongly about it that's okay. It's a nice cleanup that's all.
>
> i don't even understand why you think it's a nice cleanup.

free_pages_prepare() is directly calling __memcg_kmem_uncharge_page()
instead of memcg_kmem_uncharge_page(), and open-coding checks that
already exist in both of them to avoid the unnecessary function call
if possible. I think this should be the job of
memcg_kmem_uncharge_page(), but it's currently missing the
PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()).

So I think moving that check to the wrapper allows
free_pages_prepare() to call memcg_kmem_uncharge_page() and without
worrying about those memcg-specific checks.

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Shakeel Butt]

On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote:
> On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote:
> > > I doubt an extra compound_head() will matter in that path, but if you
> > > feel strongly about it that's okay. It's a nice cleanup that's all.
> >
> > i don't even understand why you think it's a nice cleanup.
> 
> free_pages_prepare() is directly calling __memcg_kmem_uncharge_page()
> instead of memcg_kmem_uncharge_page(), and open-coding checks that
> already exist in both of them to avoid the unnecessary function call
> if possible. I think this should be the job of
> memcg_kmem_uncharge_page(), but it's currently missing the
> PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()).
> 
> So I think moving that check to the wrapper allows
> free_pages_prepare() to call memcg_kmem_uncharge_page() and without
> worrying about those memcg-specific checks.

There is a (performance) reason these open coded check are present in
page_alloc.c and that is very clear for __memcg_kmem_charge_page() but
not so much for __memcg_kmem_uncharge_page(). So, for uncharge path,
this seems ok. Now to resolve Willy's concern for the fork() path, I
think we can open code the checks there.

Willy, any concern with that approach?

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Matthew Wilcox]

On Wed, Dec 13, 2023 at 08:23:25PM +0000, Shakeel Butt wrote:
> On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote:
> > On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote:
> > > > I doubt an extra compound_head() will matter in that path, but if you
> > > > feel strongly about it that's okay. It's a nice cleanup that's all.
> > >
> > > i don't even understand why you think it's a nice cleanup.
> > 
> > free_pages_prepare() is directly calling __memcg_kmem_uncharge_page()
> > instead of memcg_kmem_uncharge_page(), and open-coding checks that
> > already exist in both of them to avoid the unnecessary function call
> > if possible. I think this should be the job of
> > memcg_kmem_uncharge_page(), but it's currently missing the
> > PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()).
> > 
> > So I think moving that check to the wrapper allows
> > free_pages_prepare() to call memcg_kmem_uncharge_page() and without
> > worrying about those memcg-specific checks.
> 
> There is a (performance) reason these open coded check are present in
> page_alloc.c and that is very clear for __memcg_kmem_charge_page() but
> not so much for __memcg_kmem_uncharge_page(). So, for uncharge path,
> this seems ok. Now to resolve Willy's concern for the fork() path, I
> think we can open code the checks there.
> 
> Willy, any concern with that approach?

The justification for this change is insufficient.  Or really any change
in this area.  It's fine the way it is.  "The check is done twice" is
really weak, when the check is so cheap (much cheaper than calling
compound_head!)

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Shakeel Butt]

On Wed, Dec 13, 2023 at 12:27 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Wed, Dec 13, 2023 at 08:23:25PM +0000, Shakeel Butt wrote:
> > On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote:
> > > On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > >
> > > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote:
> > > > > I doubt an extra compound_head() will matter in that path, but if you
> > > > > feel strongly about it that's okay. It's a nice cleanup that's all.
> > > >
> > > > i don't even understand why you think it's a nice cleanup.
> > >
> > > free_pages_prepare() is directly calling __memcg_kmem_uncharge_page()
> > > instead of memcg_kmem_uncharge_page(), and open-coding checks that
> > > already exist in both of them to avoid the unnecessary function call
> > > if possible. I think this should be the job of
> > > memcg_kmem_uncharge_page(), but it's currently missing the
> > > PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()).
> > >
> > > So I think moving that check to the wrapper allows
> > > free_pages_prepare() to call memcg_kmem_uncharge_page() and without
> > > worrying about those memcg-specific checks.
> >
> > There is a (performance) reason these open coded check are present in
> > page_alloc.c and that is very clear for __memcg_kmem_charge_page() but
> > not so much for __memcg_kmem_uncharge_page(). So, for uncharge path,
> > this seems ok. Now to resolve Willy's concern for the fork() path, I
> > think we can open code the checks there.
> >
> > Willy, any concern with that approach?
>
> The justification for this change is insufficient.  Or really any change
> in this area.  It's fine the way it is.  "The check is done twice" is
> really weak, when the check is so cheap (much cheaper than calling
> compound_head!)

I think that is what Yosry is trying i.e. reducing two calls to
page_folio() to one in the page free path.

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Shakeel Butt]

On Wed, Dec 13, 2023 at 12:41 PM Shakeel Butt <shakeelb@google.com> wrote:
>
> On Wed, Dec 13, 2023 at 12:27 PM Matthew Wilcox <willy@infradead.org> wrote:
> >
> > On Wed, Dec 13, 2023 at 08:23:25PM +0000, Shakeel Butt wrote:
> > > On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote:
> > > > On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > > >
> > > > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote:
> > > > > > I doubt an extra compound_head() will matter in that path, but if you
> > > > > > feel strongly about it that's okay. It's a nice cleanup that's all.
> > > > >
> > > > > i don't even understand why you think it's a nice cleanup.
> > > >
> > > > free_pages_prepare() is directly calling __memcg_kmem_uncharge_page()
> > > > instead of memcg_kmem_uncharge_page(), and open-coding checks that
> > > > already exist in both of them to avoid the unnecessary function call
> > > > if possible. I think this should be the job of
> > > > memcg_kmem_uncharge_page(), but it's currently missing the
> > > > PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()).
> > > >
> > > > So I think moving that check to the wrapper allows
> > > > free_pages_prepare() to call memcg_kmem_uncharge_page() and without
> > > > worrying about those memcg-specific checks.
> > >
> > > There is a (performance) reason these open coded check are present in
> > > page_alloc.c and that is very clear for __memcg_kmem_charge_page() but
> > > not so much for __memcg_kmem_uncharge_page(). So, for uncharge path,
> > > this seems ok. Now to resolve Willy's concern for the fork() path, I
> > > think we can open code the checks there.
> > >
> > > Willy, any concern with that approach?
> >
> > The justification for this change is insufficient.  Or really any change
> > in this area.  It's fine the way it is.  "The check is done twice" is
> > really weak, when the check is so cheap (much cheaper than calling
> > compound_head!)
>
> I think that is what Yosry is trying i.e. reducing two calls to
> page_folio() to one in the page free path.

Actually no, there will still be two calls to page_folio() even after
Yosry's change. One for PageMemcgKmem() and second in
__memcg_kmem_uncharge_page().

I think I agree with Willy that this patch is actually adding one more
page_folio() call to the fork code path.

Maybe we just need to remove PageMemcgKmem() check in the
free_pages_prepare() and that's all.

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Yosry Ahmed]

On Wed, Dec 13, 2023 at 12:58 PM Shakeel Butt <shakeelb@google.com> wrote:
>
> On Wed, Dec 13, 2023 at 12:41 PM Shakeel Butt <shakeelb@google.com> wrote:
> >
> > On Wed, Dec 13, 2023 at 12:27 PM Matthew Wilcox <willy@infradead.org> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 08:23:25PM +0000, Shakeel Butt wrote:
> > > > On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote:
> > > > > On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > >
> > > > > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote:
> > > > > > > I doubt an extra compound_head() will matter in that path, but if you
> > > > > > > feel strongly about it that's okay. It's a nice cleanup that's all.
> > > > > >
> > > > > > i don't even understand why you think it's a nice cleanup.
> > > > >
> > > > > free_pages_prepare() is directly calling __memcg_kmem_uncharge_page()
> > > > > instead of memcg_kmem_uncharge_page(), and open-coding checks that
> > > > > already exist in both of them to avoid the unnecessary function call
> > > > > if possible. I think this should be the job of
> > > > > memcg_kmem_uncharge_page(), but it's currently missing the
> > > > > PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()).
> > > > >
> > > > > So I think moving that check to the wrapper allows
> > > > > free_pages_prepare() to call memcg_kmem_uncharge_page() and without
> > > > > worrying about those memcg-specific checks.
> > > >
> > > > There is a (performance) reason these open coded check are present in
> > > > page_alloc.c and that is very clear for __memcg_kmem_charge_page() but
> > > > not so much for __memcg_kmem_uncharge_page(). So, for uncharge path,
> > > > this seems ok. Now to resolve Willy's concern for the fork() path, I
> > > > think we can open code the checks there.
> > > >
> > > > Willy, any concern with that approach?
> > >
> > > The justification for this change is insufficient.  Or really any change
> > > in this area.  It's fine the way it is.  "The check is done twice" is
> > > really weak, when the check is so cheap (much cheaper than calling
> > > compound_head!)
> >
> > I think that is what Yosry is trying i.e. reducing two calls to
> > page_folio() to one in the page free path.
>
> Actually no, there will still be two calls to page_folio() even after
> Yosry's change. One for PageMemcgKmem() and second in
> __memcg_kmem_uncharge_page().
>
> I think I agree with Willy that this patch is actually adding one more
> page_folio() call to the fork code path.

It is adding one more page_folio(), yes, but to the process exit path.

>
> Maybe we just need to remove PageMemcgKmem() check in the
> free_pages_prepare() and that's all.

You mean call memcg_kmem_charge_page() directly in
free_pages_prepare() without the PageMemcgKmem()? I think we can do
that. My understanding is that this is not the case today because we
want to avoid the function call if !PageMemcgKmem(). Do you believe
the cost of the function call is negligible?

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Shakeel Butt]

On Wed, Dec 13, 2023 at 2:05 PM Yosry Ahmed <yosryahmed@google.com> wrote:
>
> On Wed, Dec 13, 2023 at 12:58 PM Shakeel Butt <shakeelb@google.com> wrote:
> >
> > On Wed, Dec 13, 2023 at 12:41 PM Shakeel Butt <shakeelb@google.com> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 12:27 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > >
> > > > On Wed, Dec 13, 2023 at 08:23:25PM +0000, Shakeel Butt wrote:
> > > > > On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote:
> > > > > > On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > > >
> > > > > > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote:
> > > > > > > > I doubt an extra compound_head() will matter in that path, but if you
> > > > > > > > feel strongly about it that's okay. It's a nice cleanup that's all.
> > > > > > >
> > > > > > > i don't even understand why you think it's a nice cleanup.
> > > > > >
> > > > > > free_pages_prepare() is directly calling __memcg_kmem_uncharge_page()
> > > > > > instead of memcg_kmem_uncharge_page(), and open-coding checks that
> > > > > > already exist in both of them to avoid the unnecessary function call
> > > > > > if possible. I think this should be the job of
> > > > > > memcg_kmem_uncharge_page(), but it's currently missing the
> > > > > > PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()).
> > > > > >
> > > > > > So I think moving that check to the wrapper allows
> > > > > > free_pages_prepare() to call memcg_kmem_uncharge_page() and without
> > > > > > worrying about those memcg-specific checks.
> > > > >
> > > > > There is a (performance) reason these open coded check are present in
> > > > > page_alloc.c and that is very clear for __memcg_kmem_charge_page() but
> > > > > not so much for __memcg_kmem_uncharge_page(). So, for uncharge path,
> > > > > this seems ok. Now to resolve Willy's concern for the fork() path, I
> > > > > think we can open code the checks there.
> > > > >
> > > > > Willy, any concern with that approach?
> > > >
> > > > The justification for this change is insufficient.  Or really any change
> > > > in this area.  It's fine the way it is.  "The check is done twice" is
> > > > really weak, when the check is so cheap (much cheaper than calling
> > > > compound_head!)
> > >
> > > I think that is what Yosry is trying i.e. reducing two calls to
> > > page_folio() to one in the page free path.
> >
> > Actually no, there will still be two calls to page_folio() even after
> > Yosry's change. One for PageMemcgKmem() and second in
> > __memcg_kmem_uncharge_page().
> >
> > I think I agree with Willy that this patch is actually adding one more
> > page_folio() call to the fork code path.
>
> It is adding one more page_folio(), yes, but to the process exit path.
>
> >
> > Maybe we just need to remove PageMemcgKmem() check in the
> > free_pages_prepare() and that's all.
>
> You mean call memcg_kmem_charge_page() directly in
> free_pages_prepare() without the PageMemcgKmem()? I think we can do
> that. My understanding is that this is not the case today because we
> want to avoid the function call if !PageMemcgKmem(). Do you believe
> the cost of the function call is negligible?

The compiler can potentially inline that function but on the other
hand we will do twice reads of page->compound_head due to READ_ONCE().

We don't have data to support one option or the other. Unless we can
show perf difference between the two, I think doing nothing (leave it
as is) will be the better use of our time.

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Yosry Ahmed]

On Wed, Dec 13, 2023 at 2:12 PM Shakeel Butt <shakeelb@google.com> wrote:
>
> On Wed, Dec 13, 2023 at 2:05 PM Yosry Ahmed <yosryahmed@google.com> wrote:
> >
> > On Wed, Dec 13, 2023 at 12:58 PM Shakeel Butt <shakeelb@google.com> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 12:41 PM Shakeel Butt <shakeelb@google.com> wrote:
> > > >
> > > > On Wed, Dec 13, 2023 at 12:27 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > > >
> > > > > On Wed, Dec 13, 2023 at 08:23:25PM +0000, Shakeel Butt wrote:
> > > > > > On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote:
> > > > > > > On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > > > >
> > > > > > > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote:
> > > > > > > > > I doubt an extra compound_head() will matter in that path, but if you
> > > > > > > > > feel strongly about it that's okay. It's a nice cleanup that's all.
> > > > > > > >
> > > > > > > > i don't even understand why you think it's a nice cleanup.
> > > > > > >
> > > > > > > free_pages_prepare() is directly calling __memcg_kmem_uncharge_page()
> > > > > > > instead of memcg_kmem_uncharge_page(), and open-coding checks that
> > > > > > > already exist in both of them to avoid the unnecessary function call
> > > > > > > if possible. I think this should be the job of
> > > > > > > memcg_kmem_uncharge_page(), but it's currently missing the
> > > > > > > PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()).
> > > > > > >
> > > > > > > So I think moving that check to the wrapper allows
> > > > > > > free_pages_prepare() to call memcg_kmem_uncharge_page() and without
> > > > > > > worrying about those memcg-specific checks.
> > > > > >
> > > > > > There is a (performance) reason these open coded check are present in
> > > > > > page_alloc.c and that is very clear for __memcg_kmem_charge_page() but
> > > > > > not so much for __memcg_kmem_uncharge_page(). So, for uncharge path,
> > > > > > this seems ok. Now to resolve Willy's concern for the fork() path, I
> > > > > > think we can open code the checks there.
> > > > > >
> > > > > > Willy, any concern with that approach?
> > > > >
> > > > > The justification for this change is insufficient.  Or really any change
> > > > > in this area.  It's fine the way it is.  "The check is done twice" is
> > > > > really weak, when the check is so cheap (much cheaper than calling
> > > > > compound_head!)
> > > >
> > > > I think that is what Yosry is trying i.e. reducing two calls to
> > > > page_folio() to one in the page free path.
> > >
> > > Actually no, there will still be two calls to page_folio() even after
> > > Yosry's change. One for PageMemcgKmem() and second in
> > > __memcg_kmem_uncharge_page().
> > >
> > > I think I agree with Willy that this patch is actually adding one more
> > > page_folio() call to the fork code path.
> >
> > It is adding one more page_folio(), yes, but to the process exit path.
> >
> > >
> > > Maybe we just need to remove PageMemcgKmem() check in the
> > > free_pages_prepare() and that's all.
> >
> > You mean call memcg_kmem_charge_page() directly in
> > free_pages_prepare() without the PageMemcgKmem()? I think we can do
> > that. My understanding is that this is not the case today because we
> > want to avoid the function call if !PageMemcgKmem(). Do you believe
> > the cost of the function call is negligible?
>
> The compiler can potentially inline that function but on the other
> hand we will do twice reads of page->compound_head due to READ_ONCE().
>
> We don't have data to support one option or the other. Unless we can
> show perf difference between the two, I think doing nothing (leave it
> as is) will be the better use of our time.

Ack, let's just leave it for now. FWIW, I believe what this patch is
doing will eventually be the right thing to do once the code is
folio-ized and the calls to page_folio() disappear naturally anyway.

Re: [PATCH] mm: memcg: remove direct use of __memcg_kmem_uncharge_page

[Roman Gushchin]

On Wed, Dec 13, 2023 at 02:12:35PM -0800, Shakeel Butt wrote:
> On Wed, Dec 13, 2023 at 2:05 PM Yosry Ahmed <yosryahmed@google.com> wrote:
> >
> > On Wed, Dec 13, 2023 at 12:58 PM Shakeel Butt <shakeelb@google.com> wrote:
> > >
> > > On Wed, Dec 13, 2023 at 12:41 PM Shakeel Butt <shakeelb@google.com> wrote:
> > > >
> > > > On Wed, Dec 13, 2023 at 12:27 PM Matthew Wilcox <willy@infradead.org> wrote:
> > > > >
> > > > > On Wed, Dec 13, 2023 at 08:23:25PM +0000, Shakeel Butt wrote:
> > > > > > On Wed, Dec 13, 2023 at 08:31:00AM -0800, Yosry Ahmed wrote:
> > > > > > > On Wed, Dec 13, 2023 at 8:27 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > > > > > >
> > > > > > > > On Wed, Dec 13, 2023 at 08:24:04AM -0800, Yosry Ahmed wrote:
> > > > > > > > > I doubt an extra compound_head() will matter in that path, but if you
> > > > > > > > > feel strongly about it that's okay. It's a nice cleanup that's all.
> > > > > > > >
> > > > > > > > i don't even understand why you think it's a nice cleanup.
> > > > > > >
> > > > > > > free_pages_prepare() is directly calling __memcg_kmem_uncharge_page()
> > > > > > > instead of memcg_kmem_uncharge_page(), and open-coding checks that
> > > > > > > already exist in both of them to avoid the unnecessary function call
> > > > > > > if possible. I think this should be the job of
> > > > > > > memcg_kmem_uncharge_page(), but it's currently missing the
> > > > > > > PageMemcgKmem() check (which is in __memcg_kmem_uncharge_page()).
> > > > > > >
> > > > > > > So I think moving that check to the wrapper allows
> > > > > > > free_pages_prepare() to call memcg_kmem_uncharge_page() and without
> > > > > > > worrying about those memcg-specific checks.
> > > > > >
> > > > > > There is a (performance) reason these open coded check are present in
> > > > > > page_alloc.c and that is very clear for __memcg_kmem_charge_page() but
> > > > > > not so much for __memcg_kmem_uncharge_page(). So, for uncharge path,
> > > > > > this seems ok. Now to resolve Willy's concern for the fork() path, I
> > > > > > think we can open code the checks there.
> > > > > >
> > > > > > Willy, any concern with that approach?
> > > > >
> > > > > The justification for this change is insufficient.  Or really any change
> > > > > in this area.  It's fine the way it is.  "The check is done twice" is
> > > > > really weak, when the check is so cheap (much cheaper than calling
> > > > > compound_head!)
> > > >
> > > > I think that is what Yosry is trying i.e. reducing two calls to
> > > > page_folio() to one in the page free path.
> > >
> > > Actually no, there will still be two calls to page_folio() even after
> > > Yosry's change. One for PageMemcgKmem() and second in
> > > __memcg_kmem_uncharge_page().
> > >
> > > I think I agree with Willy that this patch is actually adding one more
> > > page_folio() call to the fork code path.
> >
> > It is adding one more page_folio(), yes, but to the process exit path.
> >
> > >
> > > Maybe we just need to remove PageMemcgKmem() check in the
> > > free_pages_prepare() and that's all.
> >
> > You mean call memcg_kmem_charge_page() directly in
> > free_pages_prepare() without the PageMemcgKmem()? I think we can do
> > that. My understanding is that this is not the case today because we
> > want to avoid the function call if !PageMemcgKmem(). Do you believe
> > the cost of the function call is negligible?
> 
> The compiler can potentially inline that function but on the other
> hand we will do twice reads of page->compound_head due to READ_ONCE().
> 
> We don't have data to support one option or the other. Unless we can
> show perf difference between the two, I think doing nothing (leave it
> as is) will be the better use of our time.

+1, especially given how controversial the change is.