On Wed, Dec 13, 2023 at 12:58 PM Shakeel Butt <shakeelb@xxxxxxxxxx> wrote: > > 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. 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?
