linux-kernel - Re: [PATCH v2 2/3] khugepaged: Optimize collapse_pte_mapped

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <2d0c061c-df01-42dc-ba04-69d47d7516ce@arm.com>
Date: Tue, 15 Jul 2025 16:10:11 +0530
From: Dev Jain <dev.jain@....com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
 David Hildenbrand <david@...hat.com>
Cc: akpm@...ux-foundation.org, ziy@...dia.com, baolin.wang@...ux.alibaba.com,
 Liam.Howlett@...cle.com, npache@...hat.com, ryan.roberts@....com,
 baohua@...nel.org, linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] khugepaged: Optimize collapse_pte_mapped_thp() for
 large folios by PTE batching


On 15/07/25 3:32 pm, Lorenzo Stoakes wrote:
> On Tue, Jul 15, 2025 at 11:56:26AM +0200, David Hildenbrand wrote:
>>>>>>>> -	if (nr_ptes) {
>>>>>>>> +	if (nr_mapped_ptes) {
>>>>>>> I know it's ironic coming from me :P but I'm not sure why we need to churn this
>>>>>>> up by renaming?
>>>>>> Because nr_ptes is an existing variable and I need a new variable to make
>>>>>> the jump at the end of the PTE batch.
>>>>> I thought you eliminated nr_ptes as a variable here? Where else is it used?
>>>>>
>>>>> Oh how this code needs refactoring...
>>>> If we retain nr_ptes, then the two variables will be nr_ptes and nr_mapped_ptes,
>>>> which is confusing since the former is plain and the latter has a _mapped_ thingy
>>>> in it, so instead now we call them nr_batch_ptes and nr_mapped_ptes.
>>>>
>>> Sigh, this is still awful. But probably just existing awfulness. This whole
>>> thing needs a tent thrown over it and fumigation... but again not your fault :)
>>>
>>> I mean fine, this is fine then.
>> Probably best to be pragmatic here: as long as the educated reader
>> understands the code, all good. I hope I'll never have to explain it to my
>> daughters.
> :)
>
> To be crystal clear here - Dev - I am not asking you to refactor the existing
> mess, and obviously I concede your point on the issue raised here.
>
> Did you need to respin other things here? If not I can relook and give tags.


Yes, I will have to respin because of folio_pte_batch() changes from David and
have to add a simplification patch to this series authored by David, I was just
doing that and was reading the email thread to see if I didn't miss any review
comment and then I noticed I forgot to reply to the above point.


>
>> I know, I'm a perfectionist myself ;)
> I mean if I wanted perfection I'd have run screaming from the kernel right at
> the start :P
>
> But in general on the refactoring point - I think this code is hard to
> understand even for the informed, the endless nesting and nesting and levels of
> 'you just have to know what's happening' make it really easy to miss details
> here.
>
> What I'm saying is - we can improve on this, and really all I'm doing is
> todo++. Possibly even dave_todo++?? ;) Or could even be $<name here>_todo++ :P
>
> I find it impossible to not mention this when it seems apparent to me... force
> of habit :)

I personally suck at refactoring code - it is something I have been trying to get
better at. Now that I understand khugepaged a code a lot better, I have been
thinking for a long time to clean this file up, maybe someday I will gather
the courage to do that : )
  

>
> Cheers, Lorenzo