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

[Matthew Wilcox <willy@infradead.org>]

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