Message-ID: <85ec2a50-e094-cc39-c42b-b36ce0f53010@redhat.com>
Date:   Tue, 14 Jun 2022 09:50:58 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Peter Xu <peterx@...hat.com>
Cc:     Nadav Amit <nadav.amit@...il.com>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-mm <linux-mm@...ck.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dave Hansen <dave.hansen@...el.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Yang Shi <shy828301@...il.com>,
        Hugh Dickins <hughd@...gle.com>,
        Mel Gorman <mgorman@...hsingularity.net>,
        Peter Collingbourne <pcc@...gle.com>
Subject: Re: [PATCH v2] mm/mprotect: try avoiding write faults for exclusive
 anonymous pages when changing protection

>> Not really, but I assume performance gain will be minimal and might not
>> be worth the trouble.
>>
>> I'm fairly busy (and not aware of Andreas version), so I can look at
>> this, but it will be part of a separate patch because it will go on my
>> TODO list. Not mad if someone beats me to it ;)
> 
> Just for the reference:
> 
> https://github.com/aagit/aa/commit/34cd0d78db407af06d35a06b24be8e92593964be

Thanks for that reference!

> 
>>
>>>
>>>>>
>>>>> Results of a simple microbenchmark on my Ryzen 9 3900X, comparing the new
>>>>> optimization (avoiding write faults) during mprotect() with softdirty
>>>>> tracking, where we require a write fault.
>>>
>>> Are we comparing the mprotect() sequence operations against softdirty
>>> clearing operation?  Would it make more sense if we compare the same
>>> mprotect() sequence to kernels that are before/after this patch applied?
>>
>> For simplicity I compared on the same kernel, one time exploting the
>> optimization and one time disabling the optimization via softdirty.
>>
>> I can also simply measure without+with. Extra work for me to combine
>> outputs :P
> 
> Well, still that's normally how we work on these, don't we? :)
> 
> Still note that the SOFTDIRTY check (I think) was still reverted..  I meant
> I kept thinking below check "vma->vm_flags & VM_SOFTDIRTY" should be
> "!(vma->vm_flags & VM_SOFTDIRTY)", but again that's separate change so feel
> free to ignore as we've discussed, but please make sure even if you want to
> compare with softdirty that's taking into account.

I wrapped my head around that softdirty check *a lot* and ended up
figuring out that it unintuitively is correct. But maybe I ended up
confusing myself. Anyhow, this patch merely moves that check.

> 
>>
>>>
>>>>>
>>>>>  Running 1000 iterations each
>>>>>
>>>>>  ==========================================================
>>>>>  Measuring memset() of 4096 bytes
>>>>>   First write access:
>>>>>    Min: 169 ns, Max: 8997 ns, Avg: 830 ns
>>>>>   Second write access:
>>>>>    Min: 80 ns, Max: 251 ns, Avg: 168 ns
>>>>>   Write access after mprotect(PROT_READ)+mprotect(PROT_READ|PROT_WRITE):
>>>>>    Min: 180 ns, Max: 290 ns, Avg: 190 ns
>>>>>   Write access after clearing softdirty:
>>>>>    Min: 451 ns, Max: 1774 ns, Avg: 470 ns
>>>>>  -> mprotect = 1.131 * second [avg]
>>>>>  -> mprotect = 0.404 * softdirty [avg]
>>>
>>> (I don't understand these two lines.. but maybe I'm the only one?)
>>
>> Most probably not.
>>
>> "mprotect(PROT_READ)+mprotect(PROT_READ|PROT_WRITE)" needs 113,1% the
>> runtime compared with the "second write" access.
>>
>> "mprotect(PROT_READ)+mprotect(PROT_READ|PROT_WRITE)" needs 40% of the
>> runtime compared with disabling the optimization via softdirty tracking.
>>
>> I may find time to clean that up a bit more to make it easier to consume
>> for humans.
> 
> I see, thanks.  Appending the explanation after the test result will also
> work for me.
> 
> I'm curious is that 113.1% came from tlb miss?  If that's the case, I'd
> suggest drop those comparisons if there's a new version, since they're
> probably not helping to explain what this patch is changing (avoid page
> faluts), and IMHO it can slightly confuse reviewers, if you agree.

As we seem to have easier benchmarks from Andrea and Peter, I can just
reuse these and make my life easier :)

[...]

>>>> Looks good in general. Just wondering (out loud) whether it makes more sense
>>>> to do all the vm_flags and cp_flags related checks in one of the callers
>>>> (mprotect_fixup()?) and propagate whether to try to write-unprotect in
>>>> cp_flags (e.g., by introducing new MM_CP_TRY_WRITE_UNPROTECT).
>>>
>>> I can see why David put it like that, because most of the checks are on
>>> ptes not vm_flags.
>>>
>>> But I also agree on this point, especially if to put it in another way:
>>> IMHO it'll be confusing if we keey MM_CP_DIRTY_ACCT==false for all private
>>> pages even if we're going to take them into account and do smart unprotect
>>> operations.
>>>
>>> So I'm wondering whether we could still at least move vm_flags check into
>>> the mprotect_fixup() as suggested by Nadav, perhaps something like:
>>>
>>> diff --git a/mm/mprotect.c b/mm/mprotect.c
>>> index ba5592655ee3..aefd5fe982af 100644
>>> --- a/mm/mprotect.c
>>> +++ b/mm/mprotect.c
>>> @@ -583,7 +583,11 @@ mprotect_fixup(struct mmu_gather *tlb, struct vm_area_struct *vma,
>>>          * held in write mode.
>>>          */
>>>         vma->vm_flags = newflags;
>>> -       dirty_accountable = vma_wants_writenotify(vma, vma->vm_page_prot);
>>> +       if (vma->vm_flags & VM_SHARED)
>>> +               dirty_accountable = vma_wants_writenotify(vma, vma->vm_page_prot);
>>> +       else
>>> +               /* For private mappings, only if it's writable */
>>> +               dirty_accountable = vma->vm_flags & VM_WRITE;
>>>         vma_set_page_prot(vma);
>>>  
>>>         change_protection(tlb, vma, start, end, vma->vm_page_prot,
>>>
>>> Then IIUC we could drop both the VM_WRITE check in change_pte_range(), and
>>> also the VM_SHARED check above in can_change_pte_writable().  Not sure
>>> whether that'll look slightly cleaner.
>>
>> I'll give it a shot and most probably rename dirty_accountable to
>> something more expressive -- like Nadav proposed, for example.
> 
> Sure.
> 
>>
>>>
>>> I'm also copying Peter Collingbourne <pcc@...gle.com> because afaict he
>>> proposed the initial idea (maybe worth some credit in the commit message?),
>>
>> Do you have a link to that conversation? Either my memory is messing
>> with me or I did this without reading that mail (which I think, because
>> it simply made sense with PageAnonExclusive at hand). Still, I can add a
>> reference to that mail and mention that this was suggested earlier by
>> Peter C..
> 
> I see, no worry then I thought it was coming from that.  In this case I'm
> not sure whether it's still needed.
> 
> PeterC's v1 was here:
> 
> https://lore.kernel.org/linux-mm/20201212053152.3783250-1-pcc@google.com/
> 
> But there're a bunch of versions:
> 
> https://lore.kernel.org/linux-mm/?q=mm%3A+improve+mprotect%28R%7CW%29+efficiency+on+pages+referenced+once

No idea why I missed that completely as I tend to read a lot linux-mm.
Thanks for the pointers!


-- 
Thanks,

David / dhildenb

Powered by blists - more mailing lists