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. 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. 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 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 ? 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() ? 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. 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). 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.
