Page MenuHomeFreeBSD

amd64/arm64 pmap: consistently clear PGA_WRITEABLE on fictitious, managed pages
ClosedPublic

Authored by alc on Sun, Sep 6, 6:33 PM.
Tags
None
Referenced Files
F170928181: D59466.id186012.diff
Mon, Sep 7, 4:14 PM
F170928124: D59466.id186111.diff
Mon, Sep 7, 4:14 PM
F170928012: D59466.diff
Mon, Sep 7, 4:13 PM
F170913326: D59466.diff
Mon, Sep 7, 1:14 PM
Subscribers

Details

Summary

While optimizing icache management on arm64, I found that we don't consistently clear PGA_WRITEABLE on fictitious, managed pages. Some functions do, e.g., pmap_remove_all(), but several do not. At worst, this is just a pessimization, but there is no good reason to be inconsistent Clear PGA_WRITEABLE in those that previously did not.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

alc requested review of this revision.Sun, Sep 6, 6:33 PM
sys/amd64/amd64/pmap.c
5321

Is it worth making a static function like this

static void
pmap_might_be_clear_writeable(vm_page_t m)
{
    if (!TAILQ_EMPTY(&m->md.pv_list))
        return;
    if ((m->flags & PG_FICTITIOUS) == 0 &&
        !TAILQ_EMPTY(&pa_to_pvh(tpte & PG_FRAME)->pv_list)
           return;
      vm_page_aflag_clear(m, PGA_WRITEABLE);
}

Introduce pmap_page_is_mapped_locked().

markj added inline comments.
sys/amd64/amd64/pmap.c
7279

Some sites check whether PGA_WRITEABLE is set before checking whether or not to clear it. Others don't, but I can't see a good reason for the inconsistency.

This revision is now accepted and ready to land.Mon, Sep 7, 1:17 PM
sys/amd64/amd64/pmap.c
7279

This one is the easiest to explain. It is most likely replacing a mapping to a COW page, and the additional check avoids what would be a pointless atomic. The other is pmap_remove_pages(), where we are also destroying a lot of R/O and COW mappings. In the other cases, I think it is less clear what type of mapping it is.

sys/amd64/amd64/pmap.c
7279

Would you like me to add /* The old page is likely COW. */ here?

sys/amd64/amd64/pmap.c
7279

'... so check for writeable first' or like?

alc marked an inline comment as done.Mon, Sep 7, 4:35 PM