On Tue, Jun 29, 2021 at 6:39 PM Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> And since MM_CP_DIRTY_ACCT implies "VM_WRITE|VM_SHARED" all set, above should
> be a slightly faster version of below:
That's way too subtle, particularly since the MM_CP_DIRTY_ACCT logic
comes from another file entirely.
I don't think it's even faster, considering that presumably the
anonymous mapping case is the common one, and that's the one that
needs all the extra tests, it's likely better to _not_ test that very
subtle flag at all, and just doing the straightforward and obvious
tests that are understandable _locally_.
So I claim that it's
(a) not an optimization at all
(b) completely locally unintuitive and unreadable
> Again, I think in all cases some more comment should be good indeed..
I really want more than a comment. I want that MM_CP_DIRTY_ACCT bit
testing gone.
The only point where it makes sense to check MM_CP_DIRTY_ACCT is
within the context of "is the page already dirty".
So I think the logic should be something along the lines of
- first:
if (!(vma->vm_flags & VM_WRITE))
return false;
because that logic is set in stone, and true regardless of anything
else. If the vma isn't writable, we're not going to set the write bit.
End of story.
- then, check the vma_is_anonumous() case:
if (vma_is_anonymous(vma))
return page_count(pte_page(pte)) == 1;
because if it's a writable mapping, and anonymous, then we can
mark it writable if we're the exclusive owners of that page.
- and THEN we can handle the "ok, shared mapping, now let's start
thinking about dirty accounting" cases.
Make it obvious and correct. This is not a sequence where you should
try to (incorrectly) optimize away individual instructions.
Linus
