On 15.06.22 17:25, Peter Xu wrote: > On Tue, Jun 14, 2022 at 11:36:29AM +0200, David Hildenbrand wrote: >> Similar to our MM_CP_DIRTY_ACCT handling for shared, writable mappings, we >> can try mapping anonymous pages in a private writable mapping writable if >> they are exclusive, the PTE is already dirty, and no special handling >> applies. Mapping the anonymous page writable is essentially the same thing >> the write fault handler would do in this case. >> >> Special handling is required for uffd-wp and softdirty tracking, so take >> care of that properly. Also, leave PROT_NONE handling alone for now; >> in the future, we could similarly extend the logic in do_numa_page() or >> use pte_mk_savedwrite() here. >> >> While this improves mprotect(PROT_READ)+mprotect(PROT_READ|PROT_WRITE) >> performance, it should also be a valuable optimization for uffd-wp, when >> un-protecting. >> >> This has been previously suggested by Peter Collingbourne in [1], >> relevant in the context of the Scudo memory allocator, before we had >> PageAnonExclusive. >> >> This commit doesn't add the same handling for PMDs (i.e., anonymous THP, >> anonymous hugetlb); benchmark results from Andrea indicate that there >> are minor performance gains, so it's might still be valuable to streamline >> that logic for all anonymous pages in the future. >> >> As we now also set MM_CP_DIRTY_ACCT for private mappings, let's rename >> it to MM_CP_TRY_CHANGE_WRITABLE, to make it clearer what's actually >> happening. > > I'm personally not sure why DIRTY_ACCT cannot be applied to private > mappings; it sounds not only for shared but a common thing. I also don't TBH, I think the name is just absolutely unclear in that context. > know whether "change writable" could be misread too anyway. Say, we're > never changing RO->RW mappings with this flag, but only try to unprotect > the page proactively when proper, from that POV Nadav's suggestion seems > slightly better on using "unprotect". write unprotection is a change from RO->RW, so I don't immediately see the difference. Anyhow, I don't like the sounding of TRY_WRITE_UNPROTECT. I made it match the function name that I had: MM_CP_TRY_CHANGE_WRITABLE -> !pte_write()? -> can_change_pte_writable() ? ->pte_mkwrite() Maybe MM_CP_TRY_MAKE_WRITABLE / MM_CP_TRY_MAKE_PTE_WRITABLE is clearer? Open for suggestions because I'm apparently not the bast at naming things either. > > No strong opinion, the patch looks correct to me, and thanks for providing > the new test results, > > Acked-by: Peter Xu <peterx@xxxxxxxxxx> > Thanks Peter! -- Thanks, David / dhildenb
