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: <7ce82695-f2f6-4a9b-a512-422e9efa902c@lucifer.local>
Date: Tue, 15 Jul 2025 11:02:30 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: Dev Jain <dev.jain@....com>, 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 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.

>
> 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 :)

Cheers, Lorenzo