linux-kernel - Re: [PATCH RFC 00/28] Eliminate Dying Memory Cgroup

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] [day] [month] [year] [list]
Message-Id: <F626EF89-1EA4-4A4E-85D6-93691B681E66@linux.dev>
Date: Fri, 23 May 2025 10:39:58 +0800
From: Muchun Song <muchun.song@...ux.dev>
To: Harry Yoo <harry.yoo@...cle.com>
Cc: Muchun Song <songmuchun@...edance.com>,
 hannes@...xchg.org,
 mhocko@...nel.org,
 roman.gushchin@...ux.dev,
 shakeel.butt@...ux.dev,
 akpm@...ux-foundation.org,
 david@...morbit.com,
 zhengqi.arch@...edance.com,
 yosry.ahmed@...ux.dev,
 nphamcs@...il.com,
 chengming.zhou@...ux.dev,
 linux-kernel@...r.kernel.org,
 cgroups@...r.kernel.org,
 linux-mm@...ck.org,
 hamzamahfooz@...ux.microsoft.com,
 apais@...ux.microsoft.com
Subject: Re: [PATCH RFC 00/28] Eliminate Dying Memory Cgroup



> On May 23, 2025, at 09:23, Harry Yoo <harry.yoo@...cle.com> wrote:
> 
> On Tue, Apr 15, 2025 at 10:45:04AM +0800, Muchun Song wrote:
>> This patchset is based on v6.15-rc2. It functions correctly only when
>> CONFIG_LRU_GEN (Multi-Gen LRU) is disabled. Several issues were encountered
>> during rebasing onto the latest code. For more details and assistance, refer
>> to the "Challenges" section. This is the reason for adding the RFC tag.
>> 
> 
> [...snip...]
> 
>> ## Fundamentals
>> 
>> A folio will no longer pin its corresponding memory cgroup. It is necessary
>> to ensure that the memory cgroup or the lruvec associated with the memory
>> cgroup is not released when a user obtains a pointer to the memory cgroup
>> or lruvec returned by folio_memcg() or folio_lruvec(). Users are required
>> to hold the RCU read lock or acquire a reference to the memory cgroup
>> associated with the folio to prevent its release if they are not concerned
>> about the binding stability between the folio and its corresponding memory
>> cgroup. However, some users of folio_lruvec() (i.e., the lruvec lock)
>> desire a stable binding between the folio and its corresponding memory
>> cgroup. An approach is needed to ensure the stability of the binding while
>> the lruvec lock is held, and to detect the situation of holding the
>> incorrect lruvec lock when there is a race condition during memory cgroup
>> reparenting. The following four steps are taken to achieve these goals.
>> 
>> 1. The first step  to be taken is to identify all users of both functions
>>   (folio_memcg() and folio_lruvec()) who are not concerned about binding
>>   stability and implement appropriate measures (such as holding a RCU read
>>   lock or temporarily obtaining a reference to the memory cgroup for a
>>   brief period) to prevent the release of the memory cgroup.
>> 
>> 2. Secondly, the following refactoring of folio_lruvec_lock() demonstrates
>>   how to ensure the binding stability from the user's perspective of
>>   folio_lruvec().
>> 
>>   struct lruvec *folio_lruvec_lock(struct folio *folio)
>>   {
>>           struct lruvec *lruvec;
>> 
>>           rcu_read_lock();
>>   retry:
>>           lruvec = folio_lruvec(folio);
>>           spin_lock(&lruvec->lru_lock);
>>           if (unlikely(lruvec_memcg(lruvec) != folio_memcg(folio))) {
>>                   spin_unlock(&lruvec->lru_lock);
>>                   goto retry;
>>           }
>> 
>>           return lruvec;
>>   }
> 
> Is it still required to hold RCU read lock after binding stability
> between folio and memcg?

No. The spin lock is enough. The reason is because the introducing
of lock assertion in commit:

  02f4bbefcada ("mm: kmem: add lockdep assertion to obj_cgroup_memcg")

The user may unintentionally call obj_cgroup_memcg() with holding
lruvec lock, if we do not hold rcu read lock, then obj_cgroup_memcg()
will complain about this.

> 
> In the previous version of this series, folio_lruvec_lock() is implemented:
> 
> struct lruvec *folio_lruvec_lock(struct folio *folio)
> {
> 	struct lruvec *lruvec;
> 
> 	rcu_read_lock();
> retry:
> 	lruvec = folio_lruvec(folio);
> 	spin_lock(&lruvec->lru_lock);
> 
> 	if (unlikely(lruvec_memcg(lruvec) != folio_memcg(folio))) {
> 		spin_unlock(&lruvec->lru_lock);
> 		goto retry;
> 	}
> 	rcu_read_unlock();
> 
> 	return lruvec;
> }
> 
> And then this version calls rcu_read_unlock() in lruvec_unlock(),
> instead of folio_lruvec_lock().
> 
> I wonder if this is because the memcg or objcg can be released without
> rcu_read_lock(), or just to silence the warning in
> folio_memcg()->obj_cgroup_memcg()->lockdep_assert_once(rcu_read_lock_is_held())?

The latter is right.

Muchun,
Thanks.

> 
>>   From the perspective of memory cgroup removal, the entire reparenting
>>   process (altering the binding relationship between folio and its memory
>>   cgroup and moving the LRU lists to its parental memory cgroup) should be
>>   carried out under both the lruvec lock of the memory cgroup being removed
>>   and the lruvec lock of its parent.
>> 
>> 3. Thirdly, another lock that requires the same approach is the split-queue
>>   lock of THP.
>> 
>> 4. Finally, transfer the LRU pages to the object cgroup without holding a
>>   reference to the original memory cgroup.
> 
> -- 
> Cheers,
> Harry / Hyeonggon