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

Yosry Ahmed
2023-12-13T16:24:47Z

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.