On Thu, Apr 11, 2024 at 8:14 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > On Thu, Apr 11, 2024 at 11:02:32PM +0100, Matthew Wilcox wrote: > > > How many instructions it takes for a late RETRY for WRITEs to private file > > > mappings, fallback to mmap_sem? > > > > Doesn't matter. That happens _once_ per VMA, and it's dwarfed by the > > cost of allocating and initialising the COWed page. You're adding > > instructions to every single page fault. I'm not happy that we had to > > add extra instructions to the fault path for single-threaded programs, > > but we at least had the justification that we were improving scalability > > on large systems. Your excuse is "it makes the code cleaner". And > > honestly, I don't think it even does that. > > Suren, what would you think to this? > > diff --git a/mm/memory.c b/mm/memory.c > index 6e2fe960473d..e495adcbe968 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -5821,15 +5821,6 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm, > if (!vma_start_read(vma)) > goto inval; > > - /* > - * find_mergeable_anon_vma uses adjacent vmas which are not locked. > - * This check must happen after vma_start_read(); otherwise, a > - * concurrent mremap() with MREMAP_DONTUNMAP could dissociate the VMA > - * from its anon_vma. > - */ > - if (unlikely(vma_is_anonymous(vma) && !vma->anon_vma)) > - goto inval_end_read; > - > /* Check since vm_start/vm_end might change before we lock the VMA */ > if (unlikely(address < vma->vm_start || address >= vma->vm_end)) > goto inval_end_read; > > That takes a few insns out of the page fault path (good!) at the cost > of one extra trip around the fault handler for the first fault on an > anon vma. It makes the file & anon paths more similar to each other > (good!) I see what you mean. The impact would depend on the workload but in my earlier tests when developing per-VMA locks there were on average less than 1% faults which were for anonymous pages and had vma->anon_vma==NULL. I recorded that after using my desktop for a day or so and running a series of benchmark tests. Again, that number might be drastically different on some other workloads. About the code, I'll take a closer look once I'm back from vacation this weekend but I think you will also have to modify do_anonymous_page() to use vmf_anon_prepare() instead of anon_vma_prepare(). > > We'd need some data to be sure it's really a win, but less code is > always good. > > We could even eagerly initialise vma->anon_vma for anon vmas. I don't > know why we don't do that. You found the answer to that question a long time ago and IIRC it was because in many cases we end up not needing to set vma->anon_vma at all. So, this is an optimization to try avoiding extra operations whenever we can. I'll try to find your comment on this.
