[PATCH 0/2] memcg/hugetlb: Rework memcg hugetlb charging

[PATCH 0/2] memcg/hugetlb: Rework memcg hugetlb charging

[Joshua Hahn]

This series cleans up memcg's hugetlb charging logic by deprecating the
current memcg hugetlb try-charge + {commit, cancel} logic present in
alloc_hugetlb_folio. A single function mem_cgroup_charge_hugetlb takes
its place instead. This makes the code more maintainable by simplifying
the error path, and reduces memcg's footprint in hugetlb logic. 

This patch introduces a few changes in the allocation error path:
(a) Instead of having multiple return paths, we consolidate them into
    a single error path. Failing when memcg's limit is reached no longer
    returns -ENOMEM, but -ENOSPEC like the other errors as well. This
    makes the memory controller error behavior the same as hugeTLB's.
    With this said, no callers handle -ENOMEM separately, so no existing
    behavior is affected by this change.
(b) Previously, the memcg limit is checked before the folio is acquired,
    meaning the hugeTLB folio isn't acquired if the limit is reached.
    This patch performs the charging after the folio is reached, meaning
    if memcg's limit is reached, the acquired folio is freed right away. 

In the first patch, a check for whether memcg accounts hugetlb [1] is
introduced. In the second patch, the charging mechanism is reworked.

This patch builds on earlier work [2] which adds memcg hugeTLB counters.
The request for this rework is also part of the original thread.

Suggested-by: Shakeel Butt <shakeel.butt@linux.dev>
Signed-off-by: Joshua Hahn <joshua.hahnjy@gmail.com>

[1] https://lore.kernel.org/all/20231006184629.155543-1-nphamcs@gmail.com/
[2] https://lore.kernel.org/all/20241101204402.1885383-1-joshua.hahnjy@gmail.com/

Joshua Hahn (2):
  Introduce memcg_accounts_hugetlb
  Deprecate hugetlb memcg try-commit-cancel charging

 include/linux/memcontrol.h |  5 ++--
 mm/hugetlb.c               | 35 ++++++++++------------------
 mm/memcontrol.c            | 47 +++++++++++++++-----------------------
 3 files changed, 33 insertions(+), 54 deletions(-)


base-commit: 34d664f9c954f4bce85be506bd81024f64dd5fda
-- 
2.43.5

[PATCH 1/2] memcg/hugetlb: Introduce memcg_accounts_hugetlb

[Joshua Hahn]

This patch isolates the check for whether memcg accounts hugetlb.
This condition can only be true if the memcg mount option
memory_hugetlb_accounting is on, which includes hugetlb usage
in memory.current.

Signed-of-by: Joshua Hahn <joshua.hahnjy@gmail.com>

---
 include/linux/memcontrol.h |  2 ++
 mm/memcontrol.c            | 12 ++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 34d2da05f2f1..25761d55799e 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -694,6 +694,8 @@ static inline int mem_cgroup_charge(struct folio *folio, struct mm_struct *mm,
 	return __mem_cgroup_charge(folio, mm, gfp);
 }
 
+bool memcg_accounts_hugetlb(void);
+
 int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg, gfp_t gfp,
 		long nr_pages);
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 5444d0e7bb64..59dea0122579 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4497,6 +4497,15 @@ int __mem_cgroup_charge(struct folio *folio, struct mm_struct *mm, gfp_t gfp)
 	return ret;
 }
 
+bool memcg_accounts_hugetlb(void)
+{
+#ifdef CONFIG_HUGETLB_PAGE
+	return cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING;
+#else
+	return false;
+#endif
+}
+
 /**
  * mem_cgroup_hugetlb_try_charge - try to charge the memcg for a hugetlb folio
  * @memcg: memcg to charge.
@@ -4522,8 +4531,7 @@ int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg, gfp_t gfp,
 	 * 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) ||
-		!(cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING))
+		!cgroup_subsys_on_dfl(memory_cgrp_subsys) || !memcg_accounts_hugetlb())
 		return -EOPNOTSUPP;
 
 	if (try_charge(memcg, gfp, nr_pages))
-- 
2.43.5

Re: [PATCH 1/2] memcg/hugetlb: Introduce memcg_accounts_hugetlb

[Shakeel Butt]

On Wed, Nov 06, 2024 at 02:14:33PM -0800, Joshua Hahn wrote:
> This patch isolates the check for whether memcg accounts hugetlb.
> This condition can only be true if the memcg mount option
> memory_hugetlb_accounting is on, which includes hugetlb usage
> in memory.current.
> 
> Signed-of-by: Joshua Hahn <joshua.hahnjy@gmail.com>
> 
> ---
>  include/linux/memcontrol.h |  2 ++
>  mm/memcontrol.c            | 12 ++++++++++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 34d2da05f2f1..25761d55799e 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -694,6 +694,8 @@ static inline int mem_cgroup_charge(struct folio *folio, struct mm_struct *mm,
>  	return __mem_cgroup_charge(folio, mm, gfp);
>  }
>  
> +bool memcg_accounts_hugetlb(void);
> +
>  int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg, gfp_t gfp,
>  		long nr_pages);
>  
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 5444d0e7bb64..59dea0122579 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4497,6 +4497,15 @@ int __mem_cgroup_charge(struct folio *folio, struct mm_struct *mm, gfp_t gfp)
>  	return ret;
>  }
>  
> +bool memcg_accounts_hugetlb(void)

If this is only used in memcontrol.c then no need to add in the header,
just make it static here.

> +{
> +#ifdef CONFIG_HUGETLB_PAGE
> +	return cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING;
> +#else
> +	return false;
> +#endif
> +}
> +
>  /**
>   * mem_cgroup_hugetlb_try_charge - try to charge the memcg for a hugetlb folio
>   * @memcg: memcg to charge.
> @@ -4522,8 +4531,7 @@ int mem_cgroup_hugetlb_try_charge(struct mem_cgroup *memcg, gfp_t gfp,
>  	 * 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) ||
> -		!(cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING))

Why not replace in mem_cgroup_hugetlb_try_charge() as well?

> +		!cgroup_subsys_on_dfl(memory_cgrp_subsys) || !memcg_accounts_hugetlb())
>  		return -EOPNOTSUPP;
>  
>  	if (try_charge(memcg, gfp, nr_pages))
> -- 
> 2.43.5
> 

Re: [PATCH 1/2] memcg/hugetlb: Introduce memcg_accounts_hugetlb

[SeongJae Park]

On Wed, 6 Nov 2024 14:14:33 -0800 Joshua Hahn <joshua.hahnjy@gmail.com> wrote:

> This patch isolates the check for whether memcg accounts hugetlb.
> This condition can only be true if the memcg mount option
> memory_hugetlb_accounting is on, which includes hugetlb usage
> in memory.current.
> 
> Signed-of-by: Joshua Hahn <joshua.hahnjy@gmail.com>

Nit.  s/of/off/


Thanks,
SJ

[...]

Re: [PATCH 1/2] memcg/hugetlb: Introduce memcg_accounts_hugetlb

[Joshua Hahn]

On Wed, Nov 6, 2024 at 7:21 PM SeongJae Park <sj@kernel.org> wrote:
>
> On Wed, 6 Nov 2024 14:14:33 -0800 Joshua Hahn <joshua.hahnjy@gmail.com> wrote:
> >
> > Signed-of-by: Joshua Hahn <joshua.hahnjy@gmail.com>
>
> Nit.  s/of/off/
>
>
> Thanks,
> SJ

Thank you for the catch SJ, I'll include this in my v2.

Have a great day!
Joshua

[PATCH 2/2] memcg/hugetlb: Deprecate hugetlb memcg try-commit-cancel charging

[Joshua Hahn]

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 <joshua.hahnjy@gmail.com>

---
 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);
 
 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);
+	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();
+
+	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

Re: [PATCH 2/2] memcg/hugetlb: Deprecate hugetlb memcg try-commit-cancel charging

[Shakeel Butt]

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 <joshua.hahnjy@gmail.com>
> 
> ---
>  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
> 

Re: [PATCH 2/2] memcg/hugetlb: Deprecate hugetlb memcg try-commit-cancel charging

[Joshua Hahn]

On Wed, Nov 6, 2024 at 6:50 PM Shakeel Butt <shakeel.butt@linux.dev> wrote:
>
> >
> > -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.
>
> >       /*
> >        * 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 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.
>

Hello Shakeel,

Thank you for your feedback on this patch. I will implement the changes you
mentioned in both patches. As for the comment on the other patch about
replacing the accounting check in mem_cgroup_hugetlb_try_charge,
I think this makes more sense. I will move the code from this patch to
the first.

Thank you again, have a great day!
Joshua

Re: [PATCH 2/2] memcg/hugetlb: Deprecate hugetlb memcg try-commit-cancel charging

[Joshua Hahn]

On Wed, Nov 6, 2024 at 6:50 PM Shakeel Butt <shakeel.butt@linux.dev> wrote:
>
> Please cleanup mem_cgroup_cancel_charge() and mem_cgroup_commit_charge()
> as well as there will be no users after this patch.
>

Hi Shakeel,

Thank you for your feedback. Unfortunately, it seems like even after this
patch removes the references from hugetlb.c, there are still some
references from other files.

mem_cgroup_cancel_charge:
  - memcontrol-v1.c~__mem_cgroup_clear_mc(void)

mem_cgroup_commit_charge:
  - memcontrol.c~charge_memcg(struct folio *folio, struct mem_cgroup...)

In fact, in my patch, I add an extra call to charge_memcg. I think for this
case, it makes sense to just extract out the functionality from
mem_cgroup_commit_charge (3 lines) and expand it out inside charge_memcg,
and get rid of mem_cgroup_commit_charge.

On the other hand, handling mem_cgroup_cancel_charge seems to be a bit
different. Now, all of its references are purely in memcontrol-v1.c.
I think in this case, it makes sense to migrate the function declaration
& definition into memcontrol-v1.c, and remove it from memcontrol.c.
Perhaps at a later date, a cleanup in memcontrol-v1 may find that it makes
sense to remove the function, but for now, I think we should just move it.

I hope this makes sense. Thank you again for your feedback!
Joshua

Re: [PATCH 2/2] memcg/hugetlb: Deprecate hugetlb memcg try-commit-cancel charging

[Shakeel Butt]

On Thu, Nov 07, 2024 at 01:27:53PM -0500, Joshua Hahn wrote:
> On Wed, Nov 6, 2024 at 6:50 PM Shakeel Butt <shakeel.butt@linux.dev> wrote:
> >
> > Please cleanup mem_cgroup_cancel_charge() and mem_cgroup_commit_charge()
> > as well as there will be no users after this patch.
> >
> 
> Hi Shakeel,
> 
> Thank you for your feedback. Unfortunately, it seems like even after this
> patch removes the references from hugetlb.c, there are still some
> references from other files.
> 
> mem_cgroup_cancel_charge:
>   - memcontrol-v1.c~__mem_cgroup_clear_mc(void)

__mem_cgroup_clear_mc() is gone. No more reference to
mem_cgroup_cancel_charge after your patch.

> 
> mem_cgroup_commit_charge:
>   - memcontrol.c~charge_memcg(struct folio *folio, struct mem_cgroup...)
> 
> In fact, in my patch, I add an extra call to charge_memcg. I think for this
> case, it makes sense to just extract out the functionality from
> mem_cgroup_commit_charge (3 lines) and expand it out inside charge_memcg,
> and get rid of mem_cgroup_commit_charge.

Yup just inline mem_cgroup_commit_charge into charge_memcg and remove
mem_cgroup_commit_charge.

Re: [PATCH 2/2] memcg/hugetlb: Deprecate hugetlb memcg try-commit-cancel charging

[Joshua Hahn]

On Thu, Nov 7, 2024 at 1:58 PM Shakeel Butt <shakeel.butt@linux.dev> wrote:
>
> On Thu, Nov 07, 2024 at 01:27:53PM -0500, Joshua Hahn wrote:
> > On Wed, Nov 6, 2024 at 6:50 PM Shakeel Butt <shakeel.butt@linux.dev> wrote:
> > >
> > > Please cleanup mem_cgroup_cancel_charge() and mem_cgroup_commit_charge()
> > > as well as there will be no users after this patch.
> > >
> >
> > Hi Shakeel,
> >
> > Thank you for your feedback. Unfortunately, it seems like even after this
> > patch removes the references from hugetlb.c, there are still some
> > references from other files.
> >
> > mem_cgroup_cancel_charge:
> >   - memcontrol-v1.c~__mem_cgroup_clear_mc(void)
>
> __mem_cgroup_clear_mc() is gone. No more reference to
> mem_cgroup_cancel_charge after your patch.

Ah I see, sorry for the confusion. I was looking at 6.12-rc6,
but I see now your "fully deprecate charge moving" patch.
I'll remove it completely in that case.

Thank you for your help!
Joshua