From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 90327D59F7F for ; Wed, 6 Nov 2024 23:50:24 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id EEA2D6B0092; Wed, 6 Nov 2024 18:50:23 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id E4BFB6B0098; Wed, 6 Nov 2024 18:50:23 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CC5C86B0099; Wed, 6 Nov 2024 18:50:23 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id A9EF56B0092 for ; Wed, 6 Nov 2024 18:50:23 -0500 (EST) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 31907140247 for ; Wed, 6 Nov 2024 23:50:23 +0000 (UTC) X-FDA: 82757315886.05.CA412ED Received: from out-176.mta0.migadu.com (out-176.mta0.migadu.com [91.218.175.176]) by imf05.hostedemail.com (Postfix) with ESMTP id D8B50100002 for ; Wed, 6 Nov 2024 23:49:13 +0000 (UTC) Authentication-Results: imf05.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=q2mAxqyV; spf=pass (imf05.hostedemail.com: domain of shakeel.butt@linux.dev designates 91.218.175.176 as permitted sender) smtp.mailfrom=shakeel.butt@linux.dev; dmarc=pass (policy=none) header.from=linux.dev ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1730936884; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=Gg2GvaE+yzZyVFg0LFOVZxNsyEO3RamNhvSGqKggQhQ=; b=i4/rA+/WM0Y6IVmvqWGvcXDxnSQ+WAa7wpCscfFZgawhhfG5Bl3XYW2M05pdl1sK1lZ5UV OCtLfEKRH2YGZDMvWYwMq9ipVOHJwhkZW/PXrWIsHZuQTjbU3iRBkcKmw+JNvfsJBEV5Lc AE6JeZh9UTKsPmfqXsPsXOhQUEXU8/w= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1730936884; a=rsa-sha256; cv=none; b=XVEBGyQ5jP9S7b+T9EVVFuHwoIVZuirSVhfcWr0Lqrjv/uEPy05SFxzGb5UDsOri/5/f/1 avZdQLwpbm0+nsWyD8gUxQso12rYhV0lnQNMiOrv/qXvrSKNeKS58F1mA+AUTjxOiNkCgh ryY2O+/Dx4fROtK7/JGtuDpEOwlKJgg= ARC-Authentication-Results: i=1; imf05.hostedemail.com; dkim=pass header.d=linux.dev header.s=key1 header.b=q2mAxqyV; spf=pass (imf05.hostedemail.com: domain of shakeel.butt@linux.dev designates 91.218.175.176 as permitted sender) smtp.mailfrom=shakeel.butt@linux.dev; dmarc=pass (policy=none) header.from=linux.dev Date: Wed, 6 Nov 2024 15:50:05 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1730937014; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Gg2GvaE+yzZyVFg0LFOVZxNsyEO3RamNhvSGqKggQhQ=; b=q2mAxqyVW0UCvM1RaGeV38bhLQ7TyItjbQ1TtJuQ3PqusZI/nOuATgQroM6kw4U3Oh9qT2 zD8AVKu6rt0VN1UgtMQyxSbnZ9a9yUW2J/w8GYbChQB880hB7TBca+sAftYtldOGMeGrUg ofsWvwFGfKzk/Y2RPGpuFYPTQ5+s4PE= X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Shakeel Butt To: Joshua Hahn Cc: hannes@cmpxchg.org, mhocko@kernel.org, roman.gushchin@linux.dev, muchun.song@linux.dev, akpm@linux-foundation.org, cgroups@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, kernel-team@meta.com Subject: Re: [PATCH 2/2] memcg/hugetlb: Deprecate hugetlb memcg try-commit-cancel charging Message-ID: References: <20241106221434.2029328-1-joshua.hahnjy@gmail.com> <20241106221434.2029328-3-joshua.hahnjy@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20241106221434.2029328-3-joshua.hahnjy@gmail.com> X-Migadu-Flow: FLOW_OUT X-Rspam-User: X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: D8B50100002 X-Stat-Signature: xuyn6pca76ndqsfajsx9ohxkwyfsk39m X-HE-Tag: 1730936953-677343 X-HE-Meta: U2FsdGVkX19dJ+URYWCeZXTkTOJXCH32fDODz+RDGr3RvkNqb8/9G3y56mmPFkyKB6rZb/h5X6Skfiu5nQDBKnKVGJKVSPmI0x2LK5lISA1a6+lmQkQu+YLfVZVlKLsXmuMrJEfpBmkNrKtwhrPReragAEa0q8ctGDtRdYADi/DFBiFe3s3jwzSkvFrpEusACmYrcTBc19vtEfwSwTH26IyFEcZW0RgE+qPPKbgjuyv2VM/03lhvXEtBlfq/1hdXibQTOxsDhrhXBmHjfeOwqmT8vBAvkj6zcCSEDVnJaLUafHqjsOk9OoiLkqdGVzt4Y1199cAsUUb8f+TSXPqaasXBJh4dUTX8E73TqB+wkP2JBqGH3+Z/0DoMSRCsF81IRsh81tRwyPpBk3swW5uqZbOUL/fT3fJbK14KaUDhkZJpgbLFluh82rhBaLrTDvEKazC8QZkvwysgSieNvYuY68sDC5tLBp56jL++TQSxK/KlicPnNy0kLytBw85HXrM9V5B4sN9EUpy0yq13N0aJrxT/qn8pfm2M+81kd9naTR3AJhSX1p+4+6UP8ZloHlq1YbCWbtHOpBoj4QspzaQGqL1Oadla+87WjwjdoKSj3izTfIejgIkx913jvbmkopLQi6e9i+CD5DaK09F/nIB3UukOiSO2c9nQqEiL6FcSK2BDo94LKvkO4SEeBZKTxJJ512TOppB/4YA0LcFsTEXiYSOTw7otw59/s6P9L6yt9C9uxX5Dtbux96zTPFtHD+rN5oD4MQtf7eNlbePFsgqL4vdOjBNn/lUKV/CIAUMAtIKex3dMizzyrLMjIPj3cDdPEAEhN0royNiTtRMM2o6aSg4zZ3erDGRlncfZekPYfjqC9uSpfP76Fcu4hPfL+sVhxGBGkwDYFy/myfkF2VI5mmRNeWWjNw/Q+v6IeiLtPa2ruY4Ex5cz0+mvvtj8397syAs0YZxFhWiHFkF9UOw dEl3HbsO 2X1r52hWlc6cpALy7xPcTkI15naGswPV90mUchwbMhzuxk7gAJ3BdCCtAAj+EuuV9f5v2GX/j5lLWdub5iOL2g8eLzdpe66OXxojZ+6IrR9Hb59Lr8O9StnHJiRCeDmf3kmypqkzFZxDVb5evFVxCrjo+VartGA/6brLzBmZxO9cBlOeqHvJsYbdAzDSilWm30HVub2mDPG1OazpH3qgqKqU7+xtD+6r4oi2ulxriendMX1ph649O/WJd+g== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On Wed, Nov 06, 2024 at 02:14:34PM -0800, Joshua Hahn wrote: > This patch deprecates the memcg try-{commit,cancel} logic used in hugetlb. > Instead of having three points of error for memcg accounting, the error > patch is reduced to just one point at the end, and shares the same path > with the hugeTLB controller as well. > > Please note that the hugeTLB controller still uses the try_charge to > {commit/cancel} protocol. > > Signed-off-by: Joshua Hahn > > --- > include/linux/memcontrol.h | 3 +-- > mm/hugetlb.c | 35 ++++++++++++----------------------- > mm/memcontrol.c | 37 +++++++++---------------------------- > 3 files changed, 22 insertions(+), 53 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index 25761d55799e..0024634d161f 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -696,8 +696,7 @@ static inline int mem_cgroup_charge(struct folio *folio, struct mm_struct *mm, > > bool memcg_accounts_hugetlb(void); > > -int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg, gfp_t gfp, > - long nr_pages); > +int mem_cgroup_charge_hugetlb(struct folio *folio, gfp_t gfp); Please cleanup mem_cgroup_cancel_charge() and mem_cgroup_commit_charge() as well as there will be no users after this patch. > > int mem_cgroup_swapin_charge_folio(struct folio *folio, struct mm_struct *mm, > gfp_t gfp, swp_entry_t entry); > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index fbb10e52d7ea..db9801b16d13 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -2967,21 +2967,13 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma, > struct hugepage_subpool *spool = subpool_vma(vma); > struct hstate *h = hstate_vma(vma); > struct folio *folio; > - long map_chg, map_commit, nr_pages = pages_per_huge_page(h); > + long map_chg, map_commit; > long gbl_chg; > - int memcg_charge_ret, ret, idx; > + int ret, idx; > struct hugetlb_cgroup *h_cg = NULL; > - struct mem_cgroup *memcg; > bool deferred_reserve; > gfp_t gfp = htlb_alloc_mask(h) | __GFP_RETRY_MAYFAIL; > > - memcg = get_mem_cgroup_from_current(); > - memcg_charge_ret = mem_cgroup_hugetlb_try_charge(memcg, gfp, nr_pages); > - if (memcg_charge_ret == -ENOMEM) { > - mem_cgroup_put(memcg); > - return ERR_PTR(-ENOMEM); > - } > - > idx = hstate_index(h); > /* > * Examine the region/reserve map to determine if the process > @@ -2989,12 +2981,8 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma, > * code of zero indicates a reservation exists (no change). > */ > map_chg = gbl_chg = vma_needs_reservation(h, vma, addr); > - if (map_chg < 0) { > - if (!memcg_charge_ret) > - mem_cgroup_cancel_charge(memcg, nr_pages); > - mem_cgroup_put(memcg); > + if (map_chg < 0) > return ERR_PTR(-ENOMEM); > - } > > /* > * Processes that did not create the mapping will have no > @@ -3056,6 +3044,12 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma, > /* Fall through */ > } > > + ret = mem_cgroup_charge_hugetlb(folio, gfp); You can not call this with hugetlb_lock held. > + if (ret == -ENOMEM) > + goto free_folio; > + else if (!ret) > + lruvec_stat_mod_folio(folio, NR_HUGETLB, pages_per_huge_page(h)); > + > hugetlb_cgroup_commit_charge(idx, pages_per_huge_page(h), h_cg, folio); > /* If allocation is not consuming a reservation, also store the > * hugetlb_cgroup pointer on the page. > @@ -3092,13 +3086,11 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma, > } > } > > - if (!memcg_charge_ret) > - mem_cgroup_commit_charge(folio, memcg); > - lruvec_stat_mod_folio(folio, NR_HUGETLB, pages_per_huge_page(h)); > - mem_cgroup_put(memcg); > - > return folio; > > +free_folio: > + spin_unlock_irq(&hugetlb_lock); > + free_huge_folio(folio); > out_uncharge_cgroup: > hugetlb_cgroup_uncharge_cgroup(idx, pages_per_huge_page(h), h_cg); > out_uncharge_cgroup_reservation: > @@ -3110,9 +3102,6 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma, > hugepage_subpool_put_pages(spool, 1); > out_end_reservation: > vma_end_reservation(h, vma, addr); > - if (!memcg_charge_ret) > - mem_cgroup_cancel_charge(memcg, nr_pages); > - mem_cgroup_put(memcg); > return ERR_PTR(-ENOSPC); > } > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 59dea0122579..3b728635d6aa 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -1448,8 +1448,7 @@ static void memcg_stat_format(struct mem_cgroup *memcg, struct seq_buf *s) > u64 size; > > #ifdef CONFIG_HUGETLB_PAGE > - if (unlikely(memory_stats[i].idx == NR_HUGETLB) && > - !(cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING)) > + if (unlikely(memory_stats[i].idx == NR_HUGETLB) && !memcg_accounts_hugetlb()) > continue; > #endif > size = memcg_page_state_output(memcg, memory_stats[i].idx); > @@ -4506,37 +4505,19 @@ bool memcg_accounts_hugetlb(void) > #endif > } > > -/** > - * mem_cgroup_hugetlb_try_charge - try to charge the memcg for a hugetlb folio > - * @memcg: memcg to charge. > - * @gfp: reclaim mode. > - * @nr_pages: number of pages to charge. > - * > - * This function is called when allocating a huge page folio to determine if > - * the memcg has the capacity for it. It does not commit the charge yet, > - * as the hugetlb folio itself has not been obtained from the hugetlb pool. > - * > - * Once we have obtained the hugetlb folio, we can call > - * mem_cgroup_commit_charge() to commit the charge. If we fail to obtain the > - * folio, we should instead call mem_cgroup_cancel_charge() to undo the effect > - * of try_charge(). > - * > - * Returns 0 on success. Otherwise, an error code is returned. > - */ > -int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg, gfp_t gfp, > - long nr_pages) > +int mem_cgroup_charge_hugetlb(struct folio *folio, gfp_t gfp) > { > - /* > - * If hugetlb memcg charging is not enabled, do not fail hugetlb allocation, > - * but do not attempt to commit charge later (or cancel on error) either. > - */ > - if (mem_cgroup_disabled() || !memcg || > - !cgroup_subsys_on_dfl(memory_cgrp_subsys) || !memcg_accounts_hugetlb()) > + struct mem_cgroup *memcg = get_mem_cgroup_from_current(); Leaking the above reference in error paths. > + > + if (mem_cgroup_disabled() || !memcg_accounts_hugetlb() || > + !memcg || !cgroup_subsys_on_dfl(memory_cgrp_subsys)) > return -EOPNOTSUPP; > > - if (try_charge(memcg, gfp, nr_pages)) > + if (charge_memcg(folio, memcg, gfp)) > return -ENOMEM; > > + mem_cgroup_put(memcg); > + > return 0; > } > > -- > 2.43.5 >