On Thu, Feb 20, 2025 at 12:07:35PM +0530, Anshuman Khandual wrote:
> On 2/19/25 14:28, Ryan Roberts wrote:
> > On 19/02/2025 08:45, Anshuman Khandual wrote:
> >> On 2/17/25 19:34, Ryan Roberts wrote:
> >>> + 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?
--
Catalin
