Page MenuHomeFreeBSD

Assert that the PGA_{EXECUTABLE,WRITEABLE} flags do not leak.
ClosedPublic

Authored by markj on Sep 24 2019, 4:28 PM.

Details

Summary

These flags are maintained by the pmap layer and should be cleared when
all mappings are removed, so assert that. This assertion was useful in
tracking down a bug in one of my patches.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj created this revision.Sep 24 2019, 4:28 PM
markj added reviewers: alc, kib.Sep 24 2019, 4:33 PM
alc accepted this revision.Sep 24 2019, 5:03 PM
alc added inline comments.
sys/vm/vm_page.c
3556 ↗(On Diff #62509)

As an aside, I want to ask if we (collectively) think that there is still a point in including function names in the panic strings. The practice of including the functions originated before we started printing stack traces by default.

This revision is now accepted and ready to land.Sep 24 2019, 5:03 PM
kib accepted this revision.Sep 24 2019, 5:12 PM

Would pmap_page_is_mapped() also a good or even better place to put such asserts ?

kib added inline comments.Sep 24 2019, 5:14 PM
sys/vm/vm_page.c
3556 ↗(On Diff #62509)

I find it easier to search for the panic message this way. Also aggressive inlining and debuggers bugs sometimes make this invaluable.

alc added inline comments.Sep 24 2019, 5:19 PM
sys/vm/vm_page.c
3556 ↗(On Diff #62509)

Don't people's efforts at "improving/correcting" these strings by using %s and _ _func_ _ then undermine that ease of searching?

kib added inline comments.Sep 24 2019, 5:21 PM
sys/vm/vm_page.c
3556 ↗(On Diff #62509)

Yes, exactly. And I agree with bde on that.

alc added a comment.Sep 24 2019, 5:42 PM
In D21783#475321, @kib wrote:

Would pmap_page_is_mapped() also a good or even better place to put such asserts ?

On arm64, I have tinkered with using PGA_EXECUTABLE to mean that the i-cache is consistent with the physical page's contents, and so we don't need to perform an i-cache flush when creating an executable mapping, rather than meaning that there might exist an executable mapping. In other words, a page with no mappings could have PGA_EXECUTABLE set.

My recollection is that powerpc oea64 uses PGA_EXECUTABLE to avoid some i-cache flushes on creation of an executable when the flag is set, but like PGA_WRITEABLE clears it when the last mapping is destroyed. So, unless you're running the program concurrently two or more, I don't think unnecessary i-cache flushes are avoided.

We really need to create an machine-independent recipe/framework for dealing with i-cache consistency.

markj added a comment.Sep 24 2019, 7:06 PM
In D21783#475341, @alc wrote:
In D21783#475321, @kib wrote:

Would pmap_page_is_mapped() also a good or even better place to put such asserts ?

On arm64, I have tinkered with using PGA_EXECUTABLE to mean that the i-cache is consistent with the physical page's contents, and so we don't need to perform an i-cache flush when creating an executable mapping, rather than meaning that there might exist an executable mapping. In other words, a page with no mappings could have PGA_EXECUTABLE set.

The new assertion would fail in that case, right?

My recollection is that powerpc oea64 uses PGA_EXECUTABLE to avoid some i-cache flushes on creation of an executable when the flag is set, but like PGA_WRITEABLE clears it when the last mapping is destroyed. So, unless you're running the program concurrently two or more, I don't think unnecessary i-cache flushes are avoided.

That's my understanding as well.

sys/vm/vm_page.c
3556 ↗(On Diff #62509)

I agree that having the function name in the panic string is important, but I don't see why _ _func_ _ is problematic: if you already have the function name, your search is restricted to one function.

I dislike the micro-commits to fix kassert strings when some refactoring is done and the author inevitably forgets to update those strings. I would change the KASSERT macro do automatically prepend _ _func_ _ to the msg. This is doable, albeit quite ugly: https://reviews.freebsd.org/D21784

markj added a comment.Sep 27 2019, 3:00 PM
In D21783#475321, @kib wrote:

Would pmap_page_is_mapped() also a good or even better place to put such asserts ?

In principle I agree, but some implementations are not amenable to this. For example, in the arm64 pmap we have

#define pmap_page_is_mapped(m)  (!TAILQ_EMPTY(&(m)->md.pv_list))

so some locking is required to make the assertion non-racy.

Hmm. That implementation is incorrect since it does not look at 2MB mappings. I will fix this separately.

alc added a comment.Oct 7 2019, 9:21 PM
In D21783#475341, @alc wrote:
In D21783#475321, @kib wrote:

Would pmap_page_is_mapped() also a good or even better place to put such asserts ?

On arm64, I have tinkered with using PGA_EXECUTABLE to mean that the i-cache is consistent with the physical page's contents, and so we don't need to perform an i-cache flush when creating an executable mapping, rather than meaning that there might exist an executable mapping. In other words, a page with no mappings could have PGA_EXECUTABLE set.

The new assertion would fail in that case, right?

Yes.

My (ab)use of PGA_EXECUTABLE in that experimental patch shouldn't block you from committing this change. Ultimately, I think that we will want to have a distinct PGA_ flag for indicating whether the i-cache contents might be stale.

markj added a comment.Oct 7 2019, 11:31 PM
In D21783#478981, @alc wrote:
In D21783#475341, @alc wrote:
In D21783#475321, @kib wrote:

Would pmap_page_is_mapped() also a good or even better place to put such asserts ?

On arm64, I have tinkered with using PGA_EXECUTABLE to mean that the i-cache is consistent with the physical page's contents, and so we don't need to perform an i-cache flush when creating an executable mapping, rather than meaning that there might exist an executable mapping. In other words, a page with no mappings could have PGA_EXECUTABLE set.

The new assertion would fail in that case, right?

Yes.
My (ab)use of PGA_EXECUTABLE in that experimental patch shouldn't block you from committing this change. Ultimately, I think that we will want to have a distinct PGA_ flag for indicating whether the i-cache contents might be stale.

For what it's worth, we are about to run out of aflags bits (Jeff has a diff which adds another aflag). I plan to extend aflags to 16 bits, and narrow the "flags" field, which has plenty of spares.

This revision was automatically updated to reflect the committed changes.