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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Tue, Jul 15, 2025 at 04:10:11PM +0530, Dev Jain wrote:
>
> 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.

Cool thanks :)

Will have a look on respin!

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