On Mon, 29 Jun 2015, Jerome Glisse wrote:
> [...]
>
> Iterator is what protect against concurrent freeing of the directory so it
> has to return to caller on directory boundary (for 64bits arch with 64bits
> pte it has return every 512 entries). Otherwise pt_iter_fini() would have
> to walk over the whole directory range again just to drop reference and this
> doesn't sound like a good idea.
I don't understand why it would have to return to the caller to unprotect
the directory. The iterator would simply drop the reference to the
previous directory, take a reference on the next one, and keep searching
for a valid entry.
Why would pt_iter_fini have to walk over the entire range? The iterator
would keep at most one directory per level referenced. _fini would walk
the per-level ptd array and unprotect each level, the same way it does
now.
>
> So really with what you are asking it whould be:
>
> hmm_pt_iter_init(&iter, start, end);
> for(next=pt_iter_next(&iter,&ptep); next<end; next=pt_iter_next(&iter,&ptep))
> {
> // Here ptep is valid until next address. Above you have to call
> // pt_iter_next() to switch to next directory.
> addr = max(start, next - (~HMM_PMD_MASK + 1));
> for (; addr < next; addr += PAGE_SIZE, ptep++) {
> // access ptep
> }
> }
>
> My point is that internally pt_iter_next() will do the exact same test it is
> doing now btw cur and addr. Just that the addr is no longer explicit but iter
> infer it.
But this way, the iteration across directories is more efficient because
the iterator can simply walk the directory array. Take a directory that
has one valid entry at the very end. The existing iteration will do this:
hmm_pt_iter_next(dir_addr[0], end)
Walk up the ptd array
Compute level start and end and compare them to dir_addr[0]
Compute dir_addr[1] using addr and pt->mask
Return dir_addr[1]
hmm_pt_iter_update(dir_addr[1])
Walk up the ptd array, compute level start and end
Compute level index of dir_addr[1]
Read entry for dir_addr[1]
Return NULL
hmm_pt_iter_next(dir_addr[1], end)
...
And so on 511 times until the last entry is read.
This is really more suited to a for loop iteration, which it could be if
this were fully contained within the _next call.
>
> > If _next only returned to the caller when it hit a valid hmm_pte (or end),
> > then only one function would be needed (_next) instead of two
> > (_update/_walk and _next).
>
> On the valid entry side, this is because when you are walking the page table
> you have no garanty that the entry will not be clear below you (in case of
> concurrent invalidation). The only garanty you have is that if you are able
> to read a valid entry from the update() callback then this entry is valid
> until you get a new update() callback telling you otherwise.
>
> Cheers,
> Jérôme
>