On 21/02/2025 09:55, Catalin Marinas wrote:

On Thu, Feb 20, 2025 at 12:07:35PM +0530, Anshuman Khandual wrote:

quoted

On 2/19/25 14:28, Ryan Roberts wrote:

quoted

On 19/02/2025 08:45, Anshuman Khandual wrote:

quoted

On 2/17/25 19:34, Ryan Roberts wrote:

quoted

+	while (--ncontig) {

Should this be converted into a for loop instead just to be in sync with other
similar iterators in this file.

for (i = 1; i < ncontig; i++, addr += pgsize, ptep++)
{
	tmp_pte = __ptep_get_and_clear(mm, addr, ptep);
	if (present) {
		if (pte_dirty(tmp_pte))
			pte = pte_mkdirty(pte);
		if (pte_young(tmp_pte))
			pte = pte_mkyoung(pte);
	}
}

I think the way you have written this it's incorrect. Let's say we have 16 ptes
in the block. We want to iterate over the last 15 of them (we have already read
pte 0). But you're iterating over the first 15 because you don't increment addr
and ptep until after you've been around the loop the first time. So we would
need to explicitly increment those 2 before entering the loop. But that is only
neccessary if ncontig > 1. Personally I think my approach is neater...

Thinking about this again. Just wondering should not a pte_present()
check on each entries being cleared along with (ncontig > 1) in this
existing loop before transferring over the dirty and accessed bits -
also work as intended with less code churn ?

Shouldn't all the ptes in a contig block be either all present or all
not-present? Is there any point in checking each individually?

I agree, that's why I was just testing it once outside the loop. I'm confident
my code and Anshuman's alternative are both correct. So it's just a stylistic
choice really. I'd prefer to leave it as is, but will change if there is
consensus. I'm not sure I'm hearing that though.

Catalin, do you want me to respin the series to fix up the typos that Anshuman
highlighted, or will you handle that?

Thanks,
Ryan