Messages in this thread | | | Date | Thu, 6 Jan 2022 12:00:51 +0100 | From | Michal Koutný <> | Subject | Re: [PATCH v5 10/16] mm: list_lru: allocate list_lru_one only when needed |
| |
On Mon, Dec 20, 2021 at 04:56:43PM +0800, Muchun Song <songmuchun@bytedance.com> wrote: (Thanks for pointing me here.)
> -void memcg_drain_all_list_lrus(int src_idx, struct mem_cgroup *dst_memcg) > +void memcg_drain_all_list_lrus(struct mem_cgroup *src, struct mem_cgroup *dst) > { > + struct cgroup_subsys_state *css; > struct list_lru *lru; > + int src_idx = src->kmemcg_id; > + > + /* > + * Change kmemcg_id of this cgroup and all its descendants to the > + * parent's id, and then move all entries from this cgroup's list_lrus > + * to ones of the parent. > + * > + * After we have finished, all list_lrus corresponding to this cgroup > + * are guaranteed to remain empty. So we can safely free this cgroup's > + * list lrus in memcg_list_lru_free(). > + * > + * Changing ->kmemcg_id to the parent can prevent memcg_list_lru_alloc() > + * from allocating list lrus for this cgroup after memcg_list_lru_free() > + * call. > + */ > + rcu_read_lock(); > + css_for_each_descendant_pre(css, &src->css) { > + struct mem_cgroup *memcg; > + > + memcg = mem_cgroup_from_css(css); > + memcg->kmemcg_id = dst->kmemcg_id; > + } > + rcu_read_unlock();
Do you envision using this function anywhere else beside offlining? If not, you shouldn't need traversing whole subtree because normally parents are offlined only after children (see cgroup_subsys_state.online_cnt).
> > mutex_lock(&list_lrus_mutex); > list_for_each_entry(lru, &memcg_list_lrus, list) > - memcg_drain_list_lru(lru, src_idx, dst_memcg); > + memcg_drain_list_lru(lru, src_idx, dst); > mutex_unlock(&list_lrus_mutex);
If you do, then here you only drain list_lru of the subtree root but not the descendants anymore.
So I do get that mem_cgroup.kmemcg_id refernces the "effective" kmemcg_id after offlining, so that proper list_lrus are used afterwards.
I wonder -- is this necessary when objcgs are reparented too? IOW would anyone query the offlined child's kmemcg_id? (Maybe it's worth explaining better in the commit message, I think even current approach is OK (better safe than sorry).)
Thanks, Michal
| |