Re: [PATCH v2 2/3] khugepaged: Optimize collapse_pte_mapped_thp() for large folios by PTE batching

[Dev Jain]

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