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

[Zi Yan <ziy@nvidia.com>]

On 30 Dec 2025, at 14:18, Chris Mason wrote:

> On 12/30/25 1:13 PM, Shakeel Butt wrote:
>> On Tue, Dec 30, 2025 at 11:46:02AM -0500, Zi Yan wrote:
>>> On 29 Dec 2025, at 23:48, Shakeel Butt wrote:
>>>
>>>> On Tue, Dec 30, 2025 at 12:25:31PM +0800, Qi Zheng wrote:
>>>>>
>>>>>
>>>> [...]
>>>>>>>
>>>>>>> Thank you for running the AI review for this patchset, but please do not
>>>>>>> directly send the raw data from the AI review to the community, as this
>>>>>>> is no different from automated review by a robot.
>>>>>>
>>>>>> Hi Qi,
>>>>>>
>>>>>> I don't know why you're so negative towards it. It's been great at
>>>>>
>>>>> No, I don't object to having a dedicated robot to do this.
>>>>>
>>>>>> finding pretty tricky bugs often missed by human reviewers. In no way
>>>>>> it's a replacement for human reviews, but if a robot can find real
>>>>>> issues and make the kernel more reliable and safe, I'm in.
>>>>>
>>>>> I just think you should do a preliminary review of the AI ​​review results
>>>>> instead of sending them out directly. Otherwise, if everyone does this,
>>>>> the community will be full of bots.
>
> I do think it's awkward to dump the whole review output for the patch
> series in a single message.  It looks like there's a sudden jump to XML?
> It's better to reply to the individual patches with the comments
> inline, which I think is where Roman is trying to go long term.
>
> With BPF, it looks more like this:
> https://lore.kernel.org/bpf/?q=AI+reviewed+your+patch

These look really good. At least the patch author can easily see the
feedback.

>
>>>>>
>>>>> No?
>>>>>
>>>>
>>>> We don't want too many bots but we definitely want at least one AI
>>>> review bot. Now we have precedence of BPF and networking subsystem and
>>>> the results I have seen are really good. I think the MM community needs
>>>> to come together and decide on the formalities of AI review process and
>>>> I see Roman is doing some early experimentation and result looks great.
>>>
>>> Do you mind explaining why the result looks great? Does it mean you agree
>>> the regressions pointed out by the AI review?
>>
>> The result looks great because the points raised are really thought
>> provoking and things I have not thought about when I reviewed the
>> series. The lru lock without irq or the possible infinite retry loop in
>> get_mem_cgroup_from_folio() are two such examples. Are these real
>> regressions? I am not sure.
>>
>>>
>>> If we want to do AI reviews, the process should be improved instead of
>>> just pasting the output from AI. In the initial stage, I think some human
>>> intervention is needed, at least adding some comment on AI reviews would
>>> be helpful.
>>
>> Yes I agree and therefore I mentioned we should discuss how should we
>> (MM community) should adopt the AI reviews.
>
> What tends to happen with BPF is the patch author or bpf maintainers
> point out problems with the reviews and I fix up the prompts over time.
> The false positive rate is ~20% today (measured since late October), and
> it's generally declining.

Yeah, I can see bpf.md contains more detailed rules compared to mm.md.

>
>>
>>> Otherwise, it looks like you agree completely with AI reviews.
>>> In addition, “50% of the reported issues are real”, is the AI tossing
>>> a coin when reporting issues?
>>>
>>> When I am looking into the prompt part, I have the following questions:
>>>
>>> 1. What is “Prompts SHA: 192922ae6bf4 ("bpf.md: adjust the documentation
>>> about bpf kfunc parameter validation”)”? I got the actual prompts
>>> from irc: https://github.com/masoncl/review-prompts/tree/main , but it
>>> should be provided along with the review for others to reproduce.
>>
>> I agree and I didn't know that Chris's review prompts are used here.
>>
>> Ccing Chris for your following questions.
>>
>>>>> 2. Looking at the mm prompt: https://github.com/masoncl/review-prompts/blob/main/mm.md , are you sure the patterns are all right?
>>> 	a. Page/Folio States, Large folios require per-page state tracking for
>>> 		Reference counts. I thought we want to get rid of per page refcount.
>
> Early in prompt development I hand picked a few hundred patches from
> 6.16 fixing bugs, and I iterated on these adding subsystem knowledge to
> catch the known bugs.  That's where that rule came from, but as you say
> there's a risk this information gets old.  Do we want to get rid of per
> page refcounts or have we done it?  (more on that at the bottom of the
> email).

willy has covered this part in another email.

>
>>>     b. Migration Invariants, NUMA balancing expects valid PTE combinations.
>>> 		PROTNONE PTEs are hardware invalid to trigger fault.
>>> 	c. TLB flushes required after PTE modifications. How about spurious fault
>>> 		handling?
>>>
>
> AI generally uses them as a starting point and fills in details, but
> I agree the MM bits are pretty minimal.
>
>>> 3. For a cgroup patchset, I was expecting some cgroup specific prompt rules,
>>> 	but could not find any. What am I missing?
>
> I think the only cgroup specific information I've needed so far is
> explaining css_get() and the section on __GFP_ACCOUNT.  I actively try
> to avoid adding details unless we're missing bugs or generating false
> positives.

I assume your review prompts are mainly used for BPF code, so it is understandable
that there are not many MM rules. My above concerns are mainly on the prompts
are directly used on MM patches without adding more MM specific rules.

Ideally, each subsystem could maintain its own rules in the corresponding
file to get a better outcome. :)

Best Regards,
Yan, Zi