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

[Yosry Ahmed <yosryahmed@google.com>]

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.