Le 22/04/2019 à 23:29, Michel Lespinasse a écrit :
Hi Laurent, Thanks a lot for copying me on this patchset. It took me a few days to go through it - I had not been following the previous iterations of this series so I had to catch up. I will be sending comments for individual commits, but before tat I would like to discuss the series as a whole.
Hi Michel, Thanks for reviewing this series.
I think these changes are a big step in the right direction. My main reservation about them is that they are additive - adding some complexity for speculative page faults - and I wonder if it'd be possible, over the long term, to replace the existing complexity we have in mmap_sem retry mechanisms instead of adding to it. This is not something that should block your progress, but I think it would be good, as we introduce spf, to evaluate whether we could eventually get all the way to removing the mmap_sem retry mechanism, or if we will actually have to keep both.
Until we get rid of the mmap_sem which seems to be a very long story, I can't see how we could get rid of the retry mechanism.
The proposed spf mechanism only handles anon vmas. Is there a fundamental reason why it couldn't handle mapped files too ? My understanding is that the mechanism of verifying the vma after taking back the ptl at the end of the fault would work there too ? The file has to stay referenced during the fault, but holding the vma's refcount could be made to cover that ? the vm_file refcount would have to be released in __free_vma() instead of remove_vma; I'm not quite sure if that has more implications than I realize ?
The only concern is the flow of operation done in the vm_ops->fault() processing. Most of the file system relie on the generic filemap_fault() which should be safe to use. But we need a clever way to identify fault processing which are compatible with the SPF handler. This could be done using a tag/flag in the vm_ops structure or in the vma's flags.
This would be the next step.
The proposed spf mechanism only works at the pte level after the page tables have already been created. The non-spf page fault path takes the mm->page_table_lock to protect against concurrent page table allocation by multiple page faults; I think unmapping/freeing page tables could be done under mm->page_table_lock too so that spf could implement allocating new page tables by verifying the vma after taking the mm->page_table_lock ?
I've to admit that I didn't dig further here. Do you have a patch? ;)
The proposed spf mechanism depends on ARCH_HAS_PTE_SPECIAL. I am not sure what is the issue there - is this due to the vma->vm_start and vma->vm_pgoff reads in *__vm_normal_page() ?
Yes that's the reason, no way to guarantee the value of these fields in the SPF path.
My last potential concern is about performance. The numbers you have look great, but I worry about potential regressions in PF performance for threaded processes that don't currently encounter contention (i.e. there may be just one thread actually doing all the work while the others are blocked). I think one good proxy for measuring that would be to measure a single threaded workload - kernbench would be fine - without the special-case optimization in patch 22 where handle_speculative_fault() immediately aborts in the single-threaded case.
I'll have to give it a try.
Reviewed-by: Michel Lespinasse <walken@xxxxxxxxxx> This is for the series as a whole; I expect to do another review pass on individual commits in the series when we have agreement on the toplevel stuff (I noticed a few things like out-of-date commit messages but that's really minor stuff).
Thanks a lot for reviewing this long series.
I want to add a note about mmap_sem. In the past there has been discussions about replacing it with an interval lock, but these never went anywhere because, mostly, of the fact that such mechanisms were too expensive to use in the page fault path. I think adding the spf mechanism would invite us to revisit this issue - interval locks may be a great way to avoid blocking between unrelated mmap_sem writers (for example, do not delay stack creation for new threads while a large mmap or munmap may be going on), and probably also to handle mmap_sem readers that can't easily use the spf mechanism (for example, gup callers which make use of the returned vmas). But again that is a separate topic to explore which doesn't have to get resolved before spf goes in.
