Messages in this thread | | | Subject | Re: [PATCH v5 13/16] mm: memcontrol: reuse memory cgroup ID for kmem ID | From | Mika Penttilä <> | Date | Mon, 20 Dec 2021 11:27:03 +0200 |
| |
Hi,
On 20.12.2021 10.56, Muchun Song wrote: > There are two idrs being used by memory cgroup, one is for kmem ID, > another is for memory cgroup ID. The maximum ID of both is 64Ki. > Both of them can limit the total number of memory cgroups. Actually, > we can reuse memory cgroup ID for kmem ID to simplify the code. > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > --- > include/linux/memcontrol.h | 1 + > mm/memcontrol.c | 46 ++++++++-------------------------------------- > 2 files changed, 9 insertions(+), 38 deletions(-) > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index 3fc437162add..7b472f805d77 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -56,6 +56,7 @@ struct mem_cgroup_reclaim_cookie { > #ifdef CONFIG_MEMCG > > #define MEM_CGROUP_ID_SHIFT 16 > +#define MEM_CGROUP_ID_MIN 1 > #define MEM_CGROUP_ID_MAX USHRT_MAX > > struct mem_cgroup_id { > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 28d6d2564f9d..04f75055f518 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -348,23 +348,6 @@ static void memcg_reparent_objcgs(struct mem_cgroup *memcg, > } > > /* > - * This will be used as a shrinker list's index. > - * The main reason for not using cgroup id for this: > - * this works better in sparse environments, where we have a lot of memcgs, > - * but only a few kmem-limited. > - */ > -static DEFINE_IDA(memcg_cache_ida); > - > -/* > - * MAX_SIZE should be as large as the number of cgrp_ids. Ideally, we could get > - * this constant directly from cgroup, but it is understandable that this is > - * better kept as an internal representation in cgroup.c. In any case, the > - * cgrp_id space is not getting any smaller, and we don't have to necessarily > - * increase ours as well if it increases. > - */ > -#define MEMCG_CACHES_MAX_SIZE MEM_CGROUP_ID_MAX > - > -/* > * A lot of the calls to the cache allocation functions are expected to be > * inlined by the compiler. Since the calls to memcg_slab_pre_alloc_hook() are > * conditional to this static branch, we'll have to allow modules that does > @@ -3528,10 +3511,12 @@ static u64 mem_cgroup_read_u64(struct cgroup_subsys_state *css, > } > > #ifdef CONFIG_MEMCG_KMEM > +#define MEM_CGROUP_KMEM_ID_MIN -1
Maybe
#define MEM_CGROUP_KMEM_ID_MIN 0
to be the first allocated id, so MEM_CGROUP_ID_DIFF is 1, not 2
> +#define MEM_CGROUP_ID_DIFF (MEM_CGROUP_ID_MIN - MEM_CGROUP_KMEM_ID_MIN) > + > static int memcg_online_kmem(struct mem_cgroup *memcg) > { > struct obj_cgroup *objcg; > - int memcg_id; > > if (cgroup_memory_nokmem) > return 0; > @@ -3539,22 +3524,16 @@ static int memcg_online_kmem(struct mem_cgroup *memcg) > if (unlikely(mem_cgroup_is_root(memcg))) > return 0; > > - memcg_id = ida_alloc_max(&memcg_cache_ida, MEMCG_CACHES_MAX_SIZE - 1, > - GFP_KERNEL); > - if (memcg_id < 0) > - return memcg_id; > - > objcg = obj_cgroup_alloc(); > - if (!objcg) { > - ida_free(&memcg_cache_ida, memcg_id); > + if (!objcg) > return -ENOMEM; > - } > + > objcg->memcg = memcg; > rcu_assign_pointer(memcg->objcg, objcg); > > static_branch_enable(&memcg_kmem_enabled_key); > > - memcg->kmemcg_id = memcg_id; > + memcg->kmemcg_id = memcg->id.id - MEM_CGROUP_ID_DIFF; > > return 0; > } > @@ -3562,7 +3541,6 @@ static int memcg_online_kmem(struct mem_cgroup *memcg) > static void memcg_offline_kmem(struct mem_cgroup *memcg) > { > struct mem_cgroup *parent; > - int kmemcg_id; > > if (cgroup_memory_nokmem) > return; > @@ -3577,20 +3555,12 @@ static void memcg_offline_kmem(struct mem_cgroup *memcg) > memcg_reparent_objcgs(memcg, parent); > > /* > - * memcg_reparent_list_lrus() can change memcg->kmemcg_id. > - * Cache it to local @kmemcg_id. > - */ > - kmemcg_id = memcg->kmemcg_id; > - > - /* > * After we have finished memcg_reparent_objcgs(), all list_lrus > * corresponding to this cgroup are guaranteed to remain empty. > * The ordering is imposed by list_lru_node->lock taken by > * memcg_reparent_list_lrus(). > */ > memcg_reparent_list_lrus(memcg, parent); > - > - ida_free(&memcg_cache_ida, kmemcg_id); > } > #else > static int memcg_online_kmem(struct mem_cgroup *memcg) > @@ -5043,7 +5013,7 @@ static struct mem_cgroup *mem_cgroup_alloc(void) > return ERR_PTR(error); > > memcg->id.id = idr_alloc(&mem_cgroup_idr, NULL, > - 1, MEM_CGROUP_ID_MAX, > + MEM_CGROUP_ID_MIN, MEM_CGROUP_ID_MAX, > GFP_KERNEL); > if (memcg->id.id < 0) { > error = memcg->id.id; > @@ -5071,7 +5041,7 @@ static struct mem_cgroup *mem_cgroup_alloc(void) > spin_lock_init(&memcg->event_list_lock); > memcg->socket_pressure = jiffies; > #ifdef CONFIG_MEMCG_KMEM > - memcg->kmemcg_id = -1; > + memcg->kmemcg_id = MEM_CGROUP_KMEM_ID_MIN; > INIT_LIST_HEAD(&memcg->objcg_list); > #endif > #ifdef CONFIG_CGROUP_WRITEBACK
| |