Page MenuHomeFreeBSD

vm: streamline uses of OBJT_SWAP and tmpfs swap objects
ClosedPublic

Authored by kib on May 1 2021, 6:05 PM.

Details

Summary

This patch extends vm_pager' table of virtual functions and adds new pager type OBJT_SWAP_TMPFS. In total it eliminates OBJ_TMPFS_NODE flag, replacing conditionals with the pager' method calls. It also eliminate several other checks for OBJT_SWAP and direct calls into swap_* functions.

I still needed some common attribute for OBJT_SWAP and OBJT_SWAP_TMPFS objects, so OBJ_TMPFS_NODE flag is repurposed as OBJ_SWAPPING, indicating any type of the swap object.

I booted the patched kernel multiuser with /tmp on tmpfs, but no other testing was done.

I did not looked at specific tests for other object types, there are definitely more opportunities to simplify the code with another set of vm_pager methods, but only OBJT_SWAP for now.
Interesting next step would be to allow dynamically loadable pagers, so OBJT_SWAP_TMPFS only appers after kldload tmpfs.ko.

Diff Detail

Repository
rG 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

kib requested review of this revision.May 1 2021, 6:05 PM
kib created this revision.
markj added inline comments.
sys/vm/vm_object.c
243 ↗(On Diff #88473)

(flags & OBJ_SWAPPING) != 0?

sys/vm/vm_object.h
204 ↗(On Diff #88473)

"swapping" sounds transient, like (flags & OBJ_SWAPPING) != 0 implies paging_in_progress > 0. I think OBJ_SWAP would be better.

sys/vm/vm_pager.h
240 ↗(On Diff #88473)

If INVARIANTS is off, this function will silently do nothing. I think callers handle it more or less sanely, but I wonder why we don't just assume method != NULL and call method unconditionally.

Ah I see it was written this way before too, ok.

This revision is now accepted and ready to land.May 3 2021, 2:15 PM
kib marked 3 inline comments as done.May 3 2021, 3:05 PM
kib added inline comments.
sys/vm/vm_object.c
243 ↗(On Diff #88473)

It is too early to check the flag, with the current code structure. I set OBJ_SWAPPING in swap_pager_alloc_init() right after vm_object_allocate() call.

I can move setting of the flag into vm_object_allocate().

kib marked an inline comment as done.

OBJ_SWAPPING->OBJ_SWAP
Set OBJ_SWAP in vm_object_allocate(), and test it in _vm_object_allocate() instead of checking types.

This revision now requires review to proceed.May 3 2021, 3:20 PM
sys/vm/swap_pager.c
767

Should we clear OBJ_SWAP as well? Otherwise the checks of object->type that were converted to checks of object->flags need to be audited to make sure they handle dead objects correctly.

kib marked an inline comment as done.May 3 2021, 5:02 PM
kib added inline comments.
sys/vm/swap_pager.c
767

Do you think that such places could exist?

swap_pager_dealloc() is called when object refcount == 0, which for non-vnode objects must imply that no one else except our caller has a pointer to the object.

Hmm, I think we can only reach destroyed swap object through the vm_object_list iteration, and then the only such function would be swap_pager_swapoff(). It rechecks OBJ_DEAD after obtaining object lock.

Ok, I will add the cleaning of the flag.

sys/vm/vm_pager.h
240 ↗(On Diff #88473)

In fact it triggers from e.g. vm_object_get_vnode(). I removed the assert.

kib marked an inline comment as done.

Clear OBJ_SWAP on dealloc
Tolerate NULL pgo_getvp()

sys/vm/swap_pager.c
437

I guess pagerops should be annotated __read_mostly.

767

It is not obvious to me that the object checks in vm_page_reclaim_run() are sufficient, for example. vm_object_terminate() does not necessarily remove all pages (see the OBJ_PG_DTOR flag). This is a contrived example though, OBJ_PG_DTOR has only a single user today. Really my point is that some state is now spread around multiple fields, so we should try to minimize the set of valid combinations.

To be honest, I somewhat dislike OBJ_SWAP. Instead of having a new object type, did you consider using a tuple <obj->type, obj->flags> to select pager methods? Or, is it really painful to check obj->type == OBJT_SWAP || obj->type == OBJT_SWAP_TMPFS inline?

kib marked 2 inline comments as done.May 3 2021, 9:58 PM
kib added inline comments.
sys/vm/swap_pager.c
437

I think they should be marked const and go into .rodata instead.

767

Plan is to (try to) make OBJT_SWAP_TMPFS pager kldloadable from tmpfs.ko. Hardcoding the type in sys/vm would be undesirable.

kib marked 2 inline comments as done.

Constify vm_pager ops, and ops for phys and managed cdevs.
Mark pageropstab[] array as read_mostly.

This revision is now accepted and ready to land.May 3 2021, 10:10 PM

Bug fixes:

  • typo in testing OBJ_SWAPPING
  • locking for vm_pager_getvp::swap_tmpfs_pager_getvp

Passed Peter' testing.

This revision now requires review to proceed.May 7 2021, 12:09 PM
This revision is now accepted and ready to land.May 7 2021, 1:38 PM