On Wed, Dec 13, 2023 at 12:41 PM Shakeel Butt <shakeelb@xxxxxxxxxx> wrote: > > On Wed, Dec 13, 2023 at 12:27 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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.
