Re: [patch 128/192] mm: improve mprotect(R|W) efficiency on pages referenced once

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux