Page MenuHomeFreeBSD

uipc_shm: Implements fspacectl(2) support
ClosedPublic

Authored by khng on Aug 10 2021, 12:33 PM.
Tags
None
Referenced Files
F105582393: D31490.diff
Tue, Dec 17, 10:41 PM
Unknown Object (File)
Thu, Dec 12, 2:39 AM
Unknown Object (File)
Wed, Dec 11, 1:40 AM
Unknown Object (File)
Thu, Nov 28, 8:06 AM
Unknown Object (File)
Sun, Nov 24, 2:40 PM
Unknown Object (File)
Wed, Nov 20, 9:11 AM
Unknown Object (File)
Nov 17 2024, 2:41 PM
Unknown Object (File)
Nov 17 2024, 1:47 PM
Subscribers

Details

Summary

This implements fspacectl(2) support on shared memory objects. The
semantic of SPACECTL_DEALLOC is equivalent to clearing the backing
store and free the pages within the affected range. If the call
succeeds, subsequent reads on the affected range return all zero.

tests/sys/posixshm/posixshm_tests.c is expanded to include a
fspacectl(2) functional test.

Sponsored by: The FreeBSD Foundation

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 40965
Build 37854: arc lint + arc unit

Event Timeline

khng requested review of this revision.Aug 10 2021, 12:33 PM

Return when shm_partial_page_invalidate fails.

Returns the offset shm_deallocate was failing at.

sys/kern/uipc_shm.c
634

The note about vm object lock is really redundant. The assert at the start of the static function already tells the story.

646

I suspect this is not a good assert. You want to assert that base + end is on the same page as base.

Comment and assert suggestions from kib@

khng marked 2 inline comments as done.Aug 10 2021, 7:59 PM

I think that DEALLOC is same as write, so you should honor F_SEAL_WRITE.
I am not sure should DEALLOC be disabled if there are kernel mappings. You may end up removing kernel-wired pages from the object' queue.

In D31490#709942, @kib wrote:

I am not sure should DEALLOC be disabled if there are kernel mappings. You may end up removing kernel-wired pages from the object' queue.

vm_object_page_remove() will not free wired pages from the object, it only removes managed mappings and marks the page invalid.

sys/kern/uipc_shm.c
1901

rl_cookie and td are unused.

1938

Suppose off == OFF_MAX - PAGE_MASK and len == PAGE_MASK. Then startofs == 0 && pi == piend && pistart == piend, so this function will do nothing, I believe.

tests/sys/posixshm/posixshm_test.c
1154

Extra newline.

sys/kern/uipc_shm.c
1987

Do we want this to be an __assert_unreachable() instead (with error previously initialized to EINVAL)? This is generally a nop if we were passed a cmd that we didn't handle, so I don't know that it's worth taking down the machine when we could toss back an error instead on non-debug kernels.

tests/sys/posixshm/posixshm_test.c
3

Kind of nitpicky, but to curtail someone pointing it out after the fact: the 'All rights reserved.' should be moved to the line above it with rwatson unless you've obtained his permission to drop it.

tests/sys/posixshm/posixshm_test.c
3

I'm pretty sure rwatson said it was OK to drop all rights reserved on his code.

khng marked 5 inline comments as done.
  • rl_cookie and td are unused, remove them
  • off == OFF_MAX - PAGE_MASK and len == PAGE_MASK can be fixed by simply using vm_ooffset_t.
  • Remove extra newline.
  • panic -> __assert_unreachable for unreachable path in shm_fspacectl
  • Restore copyright notice.
tests/sys/posixshm/posixshm_test.c
3

I will play safe for now.

sys/kern/uipc_shm.c
1962

I think this should be EINVAL in case we hit the unreachable branch on a non-debug kernel. All of the success paths will set it so.

initialize error to EINVAL in shm_fspacectl

khng marked an inline comment as done.Aug 11 2021, 6:10 PM

Something similar should work and should be done to tmpfs.

This revision is now accepted and ready to land.Aug 12 2021, 12:18 AM
  • Fix shm_partial_page_invalidation assert condition
This revision now requires review to proceed.Aug 12 2021, 2:53 PM
This revision is now accepted and ready to land.Aug 12 2021, 2:56 PM
This revision was automatically updated to reflect the committed changes.