Re: [PATCH 06/36] HMM: add HMM page table v2.

[Mark Hairgrove <mhairgrove@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 2869 bytes --]



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