vm pager: writemapping accounting for OBJT_SWAP

[kevans]

Summary

Currently writemapping accounting is only done for vnode_pager which does some accounting on the underlying vnode.

Extend this to allow accounting to be possible for any of the pager types. New pageops are added to update/release writecount that need to be implemented for any pager wishing to do said accounting, and we implement these methods now for both vnode_pager (unchanged) and swap_pager.

The primary motivation for this is to allow other systems with OBJT_SWAP objects to check if their objects have any write mappings and reject operations with EBUSY if so. posixshm will be the first to do so in order to reject adding write seals to the shmfd if any writable mappings exist.

[kevans]

sys/kern/uipc_shm.c

935 ↗

(On Diff #61406)

This shouldn't happen for private mappings.

[markj]

sys/kern/uipc_shm.c

935 ↗

(On Diff #61406)

This updates the writecount for read-only mappings. Is that intentional?

sys/vm/vm_map.h

147 ↗

(On Diff #61406)

The comment is still specific to vnodes.

[kevans]

• shm: only do accounting on writable shared mappings
• adjust comment

[kib]

sys/vm/vm_object.h

171 ↗

(On Diff #61411)

This increases each vm_object by 8 bytes. Objects are typically allocated for almost any cached vnodes, and modern systems cache millions of vnodes.

With some efforts, you can put writemappings under swp structure, I believe. Then it should not increase the vm_object' size.

Did you coded it that way because shmfd starts as OBJT_DEFAULT ? You can safely change that to OBJT_SWAP.

sys/vm/vm_pager.h

207 ↗

(On Diff #61411)

I dislike this. Why not provide the 'else' part as method ?

I suggest evaluating use of special pagertab for swap objects with counted writeable mappings.

[kevans]

Let's give this a shot:

Move writemappings back into vnp and add another one for swp; size of vm_object should remain the same. shm is now OBJT_SWAP by default so it can take advantage of writemapping.

Approach is still mostly the same- generalize the flags for writemapping, add pageops to do the accounting. Without the pageops, {update,release}_writecount are nop. Non-OBJT_VNODE and OBJT_SWAP types can specify writecounting and nothing useful happens.

I'll rewrite title/summary when we come to a consensus.

[kib]

Overall this looks good.

I would want to extend/adopt this to writecounting writeable mappings of tmpfs vnodes, which use swap objects for v_object as well. You could look at it now, or postpone for a followup.

sys/kern/uipc_shm.c

553 ↗

(On Diff #61433)

I suggest to commit this change on its own, when it is time to commit all the stuff.

902 ↗

(On Diff #61433)

Remove this.

912 ↗

(On Diff #61433)

writecnt = (flags & MAP_SHARED) != 0 && (prot & VM_PROT_WRITE) != 0;

sys/vm/swap_pager.c

3004 ↗

(On Diff #61433)

In both functions, assert that OBJ_NOSPLIT flag is set.

[kevans]

In D21456#467490, @kib wrote:

Overall this looks good.

I would want to extend/adopt this to writecounting writeable mappings of tmpfs vnodes, which use swap objects for v_object as well. You could look at it now, or postpone for a followup.

Hmm... I think this just kind of inherently works out for the most part, unless I'm looking at a different path than you're meaning:

vn_mmap calls vm_mmap_vnode; obj->type != OBJT_VNODE and writex = true. Previously it would have called vnode_pager_update_writecount, which would be a NOP because obj->type != OBJT_VNODE, but now it delegates to the swap_pager's update_writecount implementation -- so before we thought we were writecounting but we weren't, and it didn't really matter because that only invokes methods that didn't do anything with the OBJT_SWAP they were getting passed. Now we think we're writecounting and we are, because we're looking at OBJT_SWAP.

So for tmpfs, we really just need to use the v_object's writemappings now, no?

[kib]

In D21456#467500, @kevans wrote:

In D21456#467490, @kib wrote:

Overall this looks good.

I would want to extend/adopt this to writecounting writeable mappings of tmpfs vnodes, which use swap objects for v_object as well. You could look at it now, or postpone for a followup.

Hmm... I think this just kind of inherently works out for the most part, unless I'm looking at a different path than you're meaning:

vn_mmap calls vm_mmap_vnode; obj->type != OBJT_VNODE and writex = true. Previously it would have called vnode_pager_update_writecount, which would be a NOP because obj->type != OBJT_VNODE, but now it delegates to the swap_pager's update_writecount implementation -- so before we thought we were writecounting but we weren't, and it didn't really matter because that only invokes methods that didn't do anything with the OBJT_SWAP they were getting passed. Now we think we're writecounting and we are, because we're looking at OBJT_SWAP.

So for tmpfs, we really just need to use the v_object's writemappings now, no?

I do not know, great if it just works. Take a look at tmpfs_vfsops.c:tmpfs_rw_to_ro().

[kevans]

Address remaining comments from @kib, some light wordsmithing on the title/summary.

[kevans]

In D21456#467501, @kib wrote:

In D21456#467500, @kevans wrote:

In D21456#467490, @kib wrote:

Overall this looks good.

I would want to extend/adopt this to writecounting writeable mappings of tmpfs vnodes, which use swap objects for v_object as well. You could look at it now, or postpone for a followup.

Hmm... I think this just kind of inherently works out for the most part, unless I'm looking at a different path than you're meaning:

vn_mmap calls vm_mmap_vnode; obj->type != OBJT_VNODE and writex = true. Previously it would have called vnode_pager_update_writecount, which would be a NOP because obj->type != OBJT_VNODE, but now it delegates to the swap_pager's update_writecount implementation -- so before we thought we were writecounting but we weren't, and it didn't really matter because that only invokes methods that didn't do anything with the OBJT_SWAP they were getting passed. Now we think we're writecounting and we are, because we're looking at OBJT_SWAP.

So for tmpfs, we really just need to use the v_object's writemappings now, no?

I do not know, great if it just works. Take a look at tmpfs_vfsops.c:tmpfs_rw_to_ro().

I'll defer this for a little later after wrapping up some of the other bits I'm working on

sys/kern/uipc_shm.c

553 ↗

(On Diff #61433)

Alright, will do three (3) commits total: DEFAULT -> SWAP, vm/* changes, then shm_mmap changes

[markj]

sys/kern/uipc_shm.c

937 ↗

(On Diff #61487)

shm_map(2) can also be used to map the object - don't we need to update the writecnt there too?

sys/vm/vm_map.h

147 ↗

(On Diff #61406)

Maybe, "tracked writeable mapping"? Not all writeable mappings will have this flag set.

[kevans]

sys/kern/uipc_shm.c

937 ↗

(On Diff #61487)

This is unclear to me- I was planning to check object's writemappings == 0 and shmfd->shm_kmappings == 0 since all kernel mappings are writable and we can avoid the extra accounting overhead since we already track that, so it's at least not necessarily important for what I need to observe at least.

[markj]

sys/kern/uipc_shm.c

937 ↗

(On Diff #61487)

I see, I misunderstood what shm_map() is for.

[kib]

sys/kern/uipc_shm.c

937 ↗

(On Diff #61487)

My opinion is that we can certainly ignore kernel mappings for the purpose of memfd sealing.

[kevans]

sys/vm/vm_map.h

147 ↗

(On Diff #61406)

Sounds good to me- I've changed this locally to "tracked writeable mapping" -- wrapping remains wrapped as it pre-diff.

[kevans]

reupload/reopen... I was trying to avoid this getting closed by converting uipc_shm to OBJT_SWAP in case @alc had any comments/objections. I failed to mask it properly, apparently.