diff mbox series

mm/khugepaged: alloc_charge_hpage() take care of mem charge errors

Message ID 20230221214344.609226-1-peterx@redhat.com
State New
Headers
Series mm/khugepaged: alloc_charge_hpage() take care of mem charge errors |

Commit Message

Peter Xu Feb. 21, 2023, 9:43 p.m. UTC 
  If memory charge failed, the caller shouldn't call mem_cgroup_uncharge().
Let alloc_charge_hpage() handle the error itself and clear hpage properly
if mem charge fails.

Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Yang Shi <shy828301@gmail.com>
Cc: David Stevens <stevensd@chromium.org>
Cc: stable@vger.kernel.org
Fixes: 9d82c69438d0 ("mm: memcontrol: convert anon and file-thp to new mem_cgroup_charge() API")
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 mm/khugepaged.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

David Stevens Feb. 22, 2023, 4:03 a.m. UTC | #1 
On Wed, Feb 22, 2023 at 6:43 AM Peter Xu <peterx@redhat.com> wrote:
>
> If memory charge failed, the caller shouldn't call mem_cgroup_uncharge().
> Let alloc_charge_hpage() handle the error itself and clear hpage properly
> if mem charge fails.
>
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Yang Shi <shy828301@gmail.com>
> Cc: David Stevens <stevensd@chromium.org>
> Cc: stable@vger.kernel.org
> Fixes: 9d82c69438d0 ("mm: memcontrol: convert anon and file-thp to new mem_cgroup_charge() API")
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: David Stevens <stevensd@chromium.org>
Johannes Weiner Feb. 22, 2023, 5:06 p.m. UTC | #2 
Hello,

On Tue, Feb 21, 2023 at 04:43:44PM -0500, Peter Xu wrote:
> If memory charge failed, the caller shouldn't call mem_cgroup_uncharge().
> Let alloc_charge_hpage() handle the error itself and clear hpage properly
> if mem charge fails.

I'm a bit confused by this patch.

There isn't anything wrong with calling mem_cgroup_uncharge() on an
uncharged page, functionally. It checks and bails out.

It's an unnecessary call of course, but since it's an error path it's
also not a cost issue, either.

I could see an argument for improving the code, but this is actually
more code, and the caller still has the uncharge-and-put branch anyway
for when the collapse fails later on.

So I'm not sure I understand the benefit of this change.
Peter Xu Feb. 22, 2023, 5:38 p.m. UTC | #3 
On Wed, Feb 22, 2023 at 12:06:20PM -0500, Johannes Weiner wrote:
> Hello,
> 
> On Tue, Feb 21, 2023 at 04:43:44PM -0500, Peter Xu wrote:
> > If memory charge failed, the caller shouldn't call mem_cgroup_uncharge().
> > Let alloc_charge_hpage() handle the error itself and clear hpage properly
> > if mem charge fails.
> 
> I'm a bit confused by this patch.
> 
> There isn't anything wrong with calling mem_cgroup_uncharge() on an
> uncharged page, functionally. It checks and bails out.

Indeed, I didn't really notice there's zero side effect of calling that,
sorry.  In that case both "Fixes" and "Cc: stable" do not apply.

> 
> It's an unnecessary call of course, but since it's an error path it's
> also not a cost issue, either.
> 
> I could see an argument for improving the code, but this is actually
> more code, and the caller still has the uncharge-and-put branch anyway
> for when the collapse fails later on.
> 
> So I'm not sure I understand the benefit of this change.

Yes, the benefit is having a clear interface for alloc_charge_hpage() with
no prone to leaking huge page.

The patch comes from a review for David's other patch here:

https://lore.kernel.org/all/Y%2FU9fBxVJdhxiZ1v@x1n/

I've attached a new version just to reword and remove the inproper tags.
Do you think that's acceptable?

Thanks,
Johannes Weiner Feb. 22, 2023, 7:22 p.m. UTC | #4 
On Wed, Feb 22, 2023 at 12:38:36PM -0500, Peter Xu wrote:
> On Wed, Feb 22, 2023 at 12:06:20PM -0500, Johannes Weiner wrote:
> > Hello,
> > 
> > On Tue, Feb 21, 2023 at 04:43:44PM -0500, Peter Xu wrote:
> > > If memory charge failed, the caller shouldn't call mem_cgroup_uncharge().
> > > Let alloc_charge_hpage() handle the error itself and clear hpage properly
> > > if mem charge fails.
> > 
> > I'm a bit confused by this patch.
> > 
> > There isn't anything wrong with calling mem_cgroup_uncharge() on an
> > uncharged page, functionally. It checks and bails out.
> 
> Indeed, I didn't really notice there's zero side effect of calling that,
> sorry.  In that case both "Fixes" and "Cc: stable" do not apply.
> 
> > 
> > It's an unnecessary call of course, but since it's an error path it's
> > also not a cost issue, either.
> > 
> > I could see an argument for improving the code, but this is actually
> > more code, and the caller still has the uncharge-and-put branch anyway
> > for when the collapse fails later on.
> > 
> > So I'm not sure I understand the benefit of this change.
> 
> Yes, the benefit is having a clear interface for alloc_charge_hpage() with
> no prone to leaking huge page.
> 
> The patch comes from a review for David's other patch here:
> 
> https://lore.kernel.org/all/Y%2FU9fBxVJdhxiZ1v@x1n/

Ah, that makes sense. Thanks for the context.

> From 0595acbd688b60ff7b2821a073c0fe857a4ae0ee Mon Sep 17 00:00:00 2001
> From: Peter Xu <peterx@redhat.com>
> Date: Tue, 21 Feb 2023 16:43:44 -0500
> Subject: [PATCH] mm/khugepaged: alloc_charge_hpage() take care of mem charge
>  errors
> 
> If memory charge failed, instead of returning the hpage but with an error,
> allow the function to cleanup the folio properly, which is normally what a
> function should do in this case - either return successfully, or return
> with no side effect of partial runs with an indicated error.
> 
> This will also avoid the caller calling mem_cgroup_uncharge() unnecessarily
> with either anon or shmem path (even if it's safe to do so).
> 
> Cc: Johannes Weiner <hannes@cmpxchg.org>
> Cc: Yang Shi <shy828301@gmail.com>
> Cc: David Stevens <stevensd@chromium.org>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>
diff mbox series

Patch

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 8dbc39896811..941d1c7ea910 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1063,12 +1063,19 @@  static int alloc_charge_hpage(struct page **hpage, struct mm_struct *mm,
 	gfp_t gfp = (cc->is_khugepaged ? alloc_hugepage_khugepaged_gfpmask() :
 		     GFP_TRANSHUGE);
 	int node = hpage_collapse_find_target_node(cc);
+	struct folio *folio;
 
 	if (!hpage_collapse_alloc_page(hpage, gfp, node, &cc->alloc_nmask))
 		return SCAN_ALLOC_HUGE_PAGE_FAIL;
-	if (unlikely(mem_cgroup_charge(page_folio(*hpage), mm, gfp)))
+
+	folio = page_folio(*hpage);
+	if (unlikely(mem_cgroup_charge(folio, mm, gfp))) {
+		folio_put(folio);
+		*hpage = NULL;
 		return SCAN_CGROUP_CHARGE_FAIL;
+	}
 	count_memcg_page_event(*hpage, THP_COLLAPSE_ALLOC);
+
 	return SCAN_SUCCEED;
 }