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

[Muchun Song <muchun.song@linux.dev>]

> On May 23, 2025, at 09:23, Harry Yoo <harry.yoo@oracle.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