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

[markj]

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.

[alc]

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.

[kib]

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

[kib]

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]

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]

sys/vm/vm_page.c

3556 ↗

(On Diff #62509)

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

[alc]

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]

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]

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]

In D21783#475367, @markj 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.

[markj]

In D21783#478981, @alc wrote:

In D21783#475367, @markj 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.