Re: [PATCH AUTOSEL 6.15 6/8] PM: Restrict swap use to later in the suspend sequence

[Mario Limonciello <mario.limonciello@amd.com>]

On 7/9/2025 12:23 PM, Eric W. Biederman wrote:
> Sasha Levin <sashal@kernel.org> writes:
> 
>> On Tue, Jul 08, 2025 at 04:46:19PM -0500, Eric W. Biederman wrote:
>>> Sasha Levin <sashal@kernel.org> writes:
>>>
>>>> On Tue, Jul 08, 2025 at 02:32:02PM -0500, Eric W. Biederman wrote:
>>>>>
>>>>> Wow!
>>>>>
>>>>> Sasha I think an impersonator has gotten into your account, and
>>>>> is just making nonsense up.
>>>>
>>>> https://lore.kernel.org/all/aDXQaq-bq5BMMlce@lappy/
>>>
>>> It is nice it is giving explanations for it's backporting decisions.
>>>
>>> It would be nicer if those explanations were clearly marked as
>>> coming from a non-human agent, and did not read like a human being
>>> impatient for a patch to be backported.
>>
>> Thats a fair point. I'll add "LLM Analysis:" before the explanation to
>> future patches.
>>
>>> Further the machine given explanations were clearly wrong.  Do you have
>>> plans to do anything about that?  Using very incorrect justifications
>>> for backporting patches is scary.
>>
>> Just like in the past 8 years where AUTOSEL ran without any explanation
>> whatsoever, the patches are manually reviewed and tested prior to being
>> included in the stable tree.
> 
> I believe there is some testing done.  However for a lot of what I see
> go by I would be strongly surprised if there is actually much manual
> review.
> 
> I expect there is a lot of the changes are simply ignored after a quick
> glance because people don't know what is going on, or they are of too
> little consequence to spend time on.
> 
>> I don't make a point to go back and correct the justification, it's
>> there more to give some idea as to why this patch was marked for
>> review and may be completely bogus (in which case I'll drop the patch).
>>
>> For that matter, I'd often look at the explanation only if I don't fully
>> understand why a certain patch was selected. Most often I just use it as
>> a "Yes/No" signal.
>>
>> In this instance I honestly haven't read the LLM explanation. I agree
>> with you that the explanation is flawed, but the patch clearly fixes a
>> problem:
>>
>> 	"On AMD dGPUs this can lead to failed suspends under memory
>> 	pressure situations as all VRAM must be evicted to system memory
>> 	or swap."
>>
>> So it was included in the AUTOSEL patchset.
> 
> 
>> Do you have an objection to this patch being included in -stable? So far
>> your concerns were about the LLM explanation rather than actual patch.
> 
> Several objections.
> - The explanation was clearly bogus.
> - The maintainer takes alarm.
> - The patch while small, is not simple and not obviously correct.
> - The patch has not been thoroughly tested.
> 
> I object because the code does not appear to have been well tested
> outside of the realm of fixing the issue.
> 
> There is no indication that the kexec code path has ever been exercised.
> 
> So this appears to be one of those changes that was merged under
> the banner of "Let's see if this causes a regression".>
> To the original authors.  I would have appreciated it being a little
> more clearly called out in the change description that this came in
> under "Let's see if this causes a regression".
> 

As the original author of this patch I don't feel this patch is any 
different than any other patch in that regard.
I don't write in a commit message the expected risk of a patch.

There are always people that find interesting ways to exercise it and 
they could find problems that I didn't envision.

> Such changes should not be backported automatically.  They should be
> backported with care after the have seen much more usage/testing of
> the kernel they were merged into.  Probably after a kernel release or
> so.  This is something that can take some actual judgment to decide,
> when a backport is reasonable.

TBH - I didn't include stable in the commit message with the intent that 
after this baked a cycle or so that we could bring it back later if 
AUTOSEL hadn't picked it up by then.

It's a real issue people have complained about for years that is 
non-obvious where the root cause is.

Once we're all confident on this I'd love to discuss bringing it back 
even further to LTS kernels if it's viable.

> 
>>> I still highly recommend that you get your tool to not randomly
>>> cut out bits from links it references, making them unfollowable.
>>
>> Good point. I'm not really sure what messes up the line wraps. I'll take
>> a look.
> 
> It was a bit more than line wraps.  At first glance I thought
> it was just removing a prefix from the links.  On second glance
> it appears it is completely making a hash of links:
> 
> The links in question:
> https://github.com/ROCm/ROCK-Kernel-Driver/issues/174
> https://gitlab.freedesktop.org/drm/amd/-/issues/2362
> 
> The unusable restatement of those links:
> ROCm/ROCK-Kernel-Driver#174
> freedesktop.org/drm/amd#2362
> 
> Short of knowing to look up into the patch to find the links,
> those references are completely junk.
> 
>>>>> At best all of this appears to be an effort to get someone else to
>>>>> do necessary thinking for you.  As my time for kernel work is very
>>>>> limited I expect I will auto-nack any such future attempts to outsource
>>>>> someone else's thinking on me.
>>>>
>>>> I've gone ahead and added you to the list of people who AUTOSEL will
>>>> skip, so no need to worry about wasting your time here.
>>>
>>> Thank you for that.
>>>
>>> I assume going forward that AUTOSEL will not consider any patches
>>> involving the core kernel and the user/kernel ABI going forward.  The
>>> areas I have been involved with over the years, and for which my review
>>> might be interesting.
>>
>> The filter is based on authorship and SoBs. Individual maintainers of a
>> subsystem can elect to have their entire subsystem added to the ignore
>> list.
> 
> As I said.  I expect that the process looking at the output of
> get_maintainers.pl and ignoring a change when my name is returned
> will result in effectively the entire core kernel and the user/kernel
> ABI not being eligible for backport.
> 
> I bring this up because I was not an author and I did not have any
> signed-off-by's on the change in question, and yet I was still selected
> for the review.
> 
> Eric
>