Re: [PATCH v3] mm: mglru: fix stale batch updates after memcg reparenting

[Harry Yoo <harry@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3711 bytes --]



On 6/26/26 4:04 PM, Qi Zheng wrote:
> On 6/26/26 2:48 PM, Harry Yoo wrote:
>> On 6/26/26 3:24 PM, Qi Zheng wrote:
>>> On 6/26/26 12:59 PM, Harry Yoo wrote:
>>>> On 6/26/26 1:48 PM, Qi Zheng wrote:
>>>>> On 6/26/26 12:43 PM, Harry Yoo wrote:
>>>>>> On 6/26/26 11:27 AM, Qi Zheng wrote:
>>>>>>> On 6/26/26 2:41 AM, Johannes Weiner wrote:
>>>>>>>> On Thu, Jun 25, 2026 at 11:15:54PM +0800, Qi Zheng wrote:
>>>>>>>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>>>>>>>> index 35c3bb15ae96..1ec8c23c72b9 100644
>>>>>>>>> --- a/mm/vmscan.c
>>>>>>>>> +++ b/mm/vmscan.c
>>>>>>>>> @@ -3262,10 +3262,44 @@ static void update_batch_size(struct
>>>>>>>>> lru_gen_mm_walk *walk, struct folio *folio,
>>>>>>>>>          walk->nr_pages[new_gen][type][zone] += delta;
>>>>>>>>>      }
>>>>>>>>>      +#ifdef CONFIG_MEMCG
>>>>>>>>> +static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec)
>>>>>>>>> +{
>>>>>>>>> +    struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>>>>>>>>> +    struct mem_cgroup *memcg = lruvec_memcg(lruvec);
>>>>>>>>> +
>>>>>>>>> +    rcu_read_lock();
>>>>>>>>
>>>>>>>> Where is this unlocked?
>>>>>>>
>>>>>>> The lruvec_unlock_irq() in reset_batch_size() will handle the
>>>>>>> unlocking.
>>>>>>>
>>>>>>>>> +    /*
>>>>>>>>> +     * The memcg can be NULL when the memory controller is
>>>>>>>>> disabled.
>>>>>>>>> +     * Otherwise, the caller keeps the memcg owning @lruvec
>>>>>>>>> alive.
>>>>>>>>> +     */
>>>>>>>>> +    if (!memcg || !css_is_dying(&memcg->css))
>>>>>>>>> +        goto lock;
>>>>>>>>> +
>>>>>>>>> +    do {
>>>>>>>>> +        memcg = parent_mem_cgroup(memcg);
>>>>>>>>> +    } while (memcg && css_is_dying(&memcg->css));
>>>>>>>>> +    lruvec = mem_cgroup_lruvec(memcg, pgdat);
>>>>>>>>
>>>>>>>>        while (unlikely(memcg && css_is_dying(&memcg->css))) {
>>>>>>>>            memcg = parent_mem_cgroup(memcg);
>>>>>>>>            lruvec = mem_cgroup_lruvec(memcg, pgdat);
>>>>>>>
>>>>>>> There is no need to acquire the lruvec before finding the first
>>>>>>> non-dying memcg.
>>>>>>
>>>>>> struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>>>>>> struct mem_cgroup *memcg = lruvec_memcg(lruvec);
>>>>>>
>>>>>> rcu_read_lock()
>>>>>>
>>>>>> while (unlikely(memcg_is_dying(memcg)))
>>>>>>            memcg = parent_mem_cgroup(memcg);
>>>>>>
>>>>>> lruvec = mem_cgroup_lruvec(memcg, pgdat);
>>>>>
>>>>> If the first memcg is already non-dying, there's no need to re-acquire
>>>>> the lruvec. ;)
>>>>
>>>> Oh, right :)
>>>>
>>>> Hmm but I still think Johannes' suggestion makes the code cleaner.
>>>
>>> I don't have a strong preference on which of the two coding styles is
>>> more readable. BTW, is there any kernel documentation I could refer to
>>> for this?
>>
>> I don't think there's a coding style guide that specifically
>> mentions this. Just thought it's cleaner because it merges if (...) goto
>> lock; and do-while into a single while loop.
>>
>>>> Observing a dying cgroup should be rare anyway, it's worth focusing
>>>> more on readability?
>>>
>>> While it's rare to encounter consecutive dying memcgs, it can still
>>> happen, right?
>>
>> But is worth saving a few instruction in a basic block that is
>> unlikely() to be executed?
> 
> I don't have a strong opinion here. Hi Johannes, I'll leave the decision
> up to you. If necessary, I can send out the v4.
> 
>>
>> I'm not a memcg maintainer myself, though. just my 2 cents.
> 
> I'd like to express my gratitude for your reviews, and especially
> for your invaluable help with the earlier dying memcg work!

No problem, likewise to you and Muchun for invaluable work ;)

-- 
Cheers,
Harry / Hyeonggon

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]