lkml.org 
[lkml]   [2022]   [Jun]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@google.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

\
 
 \ /
  Last update: 2022-06-14 09:52    [W:0.053 / U:0.760 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site