linux-kernel - Re: [PATCH v5 10/16] mm: list_lru: allocate list_lru

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 6 Jan 2022 12:00:51 +0100
From:   Michal Koutný <mkoutny@...e.com>
To:     Muchun Song <songmuchun@...edance.com>
Cc:     willy@...radead.org, akpm@...ux-foundation.org, hannes@...xchg.org,
        mhocko@...nel.org, vdavydov.dev@...il.com, shakeelb@...gle.com,
        guro@...com, shy828301@...il.com, alexs@...nel.org,
        richard.weiyang@...il.com, david@...morbit.com,
        trond.myklebust@...merspace.com, anna.schumaker@...app.com,
        jaegeuk@...nel.org, chao@...nel.org, kari.argillander@...il.com,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, linux-nfs@...r.kernel.org,
        zhengqi.arch@...edance.com, duanxiongchun@...edance.com,
        fam.zheng@...edance.com, smuchun@...il.com
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@...edance.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