Re: [PATCH 1/2] mm/memcg: use kmem_cache when alloc memcg

[Huan Yang <link@vivo.com>]

Hi Shakeel

在 2025/4/24 11:31, Shakeel Butt 写道:
> On Thu, Apr 24, 2025 at 10:45:05AM +0800, Huan Yang wrote:
>> Hi Shakeel
>>
>> 在 2025/4/24 06:13, Shakeel Butt 写道:
>>> On Wed, Apr 23, 2025 at 02:59:12PM -0700, Andrew Morton wrote:
>>>> On Wed, 23 Apr 2025 16:43:04 +0800 Huan Yang <link@vivo.com> wrote:
>>>>
>>>>> @@ -3652,7 +3654,10 @@ static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent)
>>>>>    	int __maybe_unused i;
>>>>>    	long error;
>>>>> -	memcg = kzalloc(struct_size(memcg, nodeinfo, nr_node_ids), GFP_KERNEL);
>>>>> +	memcg = likely(memcg_cachep) ?
>>>>> +			kmem_cache_zalloc(memcg_cachep, GFP_KERNEL) :
>>>>> +			kzalloc(struct_size(memcg, nodeinfo, nr_node_ids),
>>>>> +				GFP_KERNEL);
>>>> Why are we testing for memcg_cachep=NULL?
>>>>
>>>>> @@ -5055,6 +5061,10 @@ static int __init mem_cgroup_init(void)
>>>>>    		INIT_WORK(&per_cpu_ptr(&memcg_stock, cpu)->work,
>>>>>    			  drain_local_stock);
>>>>> +	memcg_size = struct_size_t(struct mem_cgroup, nodeinfo, nr_node_ids);
>>>>> +	memcg_cachep = kmem_cache_create("mem_cgroup", memcg_size, 0,
>>>>> +					 SLAB_PANIC | SLAB_HWCACHE_ALIGN, NULL);
>>>> If it's because this allocation might have failed then let's not
>>>> bother.  If an __init-time allocation failed, this kernel is unusable
>>>> anyway.
>>>>
>>>> +1 to Andrew's point. SLAB_PANIC is used here, so, memcg_cachep can't be
>>>> NULL later.
>> I see cgroup_init(in start_kernel) ahead of initcall( which in rest_init->kernel_init->...->initcall). So, root_mem_cgroup create use
>>
>> cachep will trigger NULL pointer access.
> So, either create a new function which creates this kmem cache before
> cgroup_init() or just call mem_cgroup_init() before cgroup_init()
> (similar to cpuset_init()).

Do you mean like this:

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 5264d148bdd9..e9a4fc5aabc7 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -348,6 +348,7 @@ enum objext_flags {

  #ifdef CONFIG_MEMCG

+extern int mem_cgroup_init(void);
  static inline bool folio_memcg_kmem(struct folio *folio);

  /*
@@ -1059,6 +1060,8 @@ static inline u64 cgroup_id_from_mm(struct mm_struct *mm)

  #else /* CONFIG_MEMCG */

+static inline int mem_cgroup_init(void) { return 0; }
+
  #define MEM_CGROUP_ID_SHIFT    0

  static inline struct mem_cgroup *folio_memcg(struct folio *folio)
diff --git a/init/main.c b/init/main.c
index 6b14e6116a1f..d9c646960fdd 100644
--- a/init/main.c
+++ b/init/main.c
@@ -50,6 +50,7 @@
  #include <linux/writeback.h>
  #include <linux/cpu.h>
  #include <linux/cpuset.h>
+#include <linux/memcontrol.h>
  #include <linux/cgroup.h>
  #include <linux/efi.h>
  #include <linux/tick.h>
@@ -1087,6 +1088,7 @@ void start_kernel(void)
      nsfs_init();
      pidfs_init();
      cpuset_init();
+    mem_cgroup_init();
      cgroup_init();
      taskstats_init_early();
      delayacct_init();
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 5e2ea8b8a898..1537562c01c2 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -5029,14 +5029,12 @@ static int __init cgroup_memory(char *s)
  __setup("cgroup.memory=", cgroup_memory);

  /*
- * subsys_initcall() for memory controller.
- *
   * Some parts like memcg_hotplug_cpu_dead() have to be initialized from this
   * context because of lock dependencies (cgroup_lock -> cpu hotplug) but
   * basically everything that doesn't depend on a specific mem_cgroup structure
   * should be initialized from here.
   */
-static int __init mem_cgroup_init(void)
+int __init mem_cgroup_init(void)
  {
      int cpu;

@@ -5057,7 +5055,6 @@ static int __init mem_cgroup_init(void)

      return 0;
  }
-subsys_initcall(mem_cgroup_init);

  #ifdef CONFIG_SWAP
  /**

Anyway, I'll test it.:)

Thanks for your suggestion.

Thank you,

Huan