Page MenuHomeFreeBSD

swap_pager: handle swblk being freed while lock dropped in meta_transfer
ClosedPublic

Authored by dougm on Sep 10 2024, 9:46 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Oct 27, 4:24 AM
Unknown Object (File)
Mon, Oct 21, 1:54 PM
Unknown Object (File)
Mon, Oct 21, 1:54 PM
Unknown Object (File)
Mon, Oct 21, 1:54 PM
Unknown Object (File)
Mon, Oct 21, 1:54 PM
Unknown Object (File)
Mon, Oct 21, 1:54 PM
Unknown Object (File)
Mon, Oct 21, 1:54 PM
Unknown Object (File)
Mon, Oct 21, 1:54 PM
Subscribers

Details

Summary

If swp_pager_meta_transfer drops the write lock to call meta_build, the current swblk could conceivably be freed before the lock is reacquired, so, after regaining the lock, repeat the search for the swblk at pindex and finish processing it if it is found again.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

dougm requested review of this revision.Sep 10 2024, 9:46 PM
dougm created this revision.

Allow the possibility of freeing the block and retrying without releasing the lock before finally letting go of the lock and retrying.

I think this is broadly ok, just a couple of minor comments.

sys/vm/swap_pager.c
2222–2224

There should be an extra newline after variable declarations.

2261

Can we get away without having both pindex and p?

Drop p. Use pindex. Save all the untransferred addresses, and retry them all again with the lock held before giving up and dropping the lock.

sys/vm/swap_pager.c
2230

Doesn't the increment of pindex belong in the outer loop?

2250

This isn't really true in general. A hysteresis loop could prevent subsequent allocations until the amount of free memory has hit some threshold. I haven't spent much time in that part of UMA lately though and don't know what behaviour to expect offhand.

I'm also not really convinced that we should try to optimize this rarely executed path.

sys/vm/swap_pager.c
2230

Yes. Careless haste on my part.

2250

I'll take out the extra optimization loop. I would like to completely process sb in this iteration of the outer loop, though.

Correct and simplify from the previous offering. Ran the swap* stress2 tests once to test.

sys/vm/swap_pager.c
2254

Shouldn't this be d_mask &= ~(1 << i);?

I would probably just initialize all entries in d to SWAPBLK_NONE and avoid having d_mask entirely.

sys/vm/swap_pager.c
2254

As a matter of style, or of correctness? What's here is correct. Maybe if I flipped the bit with '^' to set it above, instead of using '|', then flipping it here would be stylistically okay?

If I drop d_mask, and initialize d[], then I have to walk the entire d array looking for values not equal to SWAPBLK_NONE, every time, where now I only have to set d_mask to zero and compare it to zero in all the normal, non-blocking iterations.

Perhaps I'm failing to understand what solution you are asking for.

A version without bitmasking. Perhaps this is acceptable?

sys/vm/swap_pager.c
2254

Sorry, I misunderstood what the old code was doing. I do the find the use of | and & in that context easier to read.

I don't object to the use of a bitmask in general, and compared to this version find the old one easier to understand.

Use a bitmask. Scope the bitmask outside of the loop to avoid reinitializing to zero.

This revision is now accepted and ready to land.Sep 14 2024, 3:30 PM
sys/vm/swap_pager.c
2204–2206

you can merge this w/ blk's def

2205

what happens if SWAP_META_PAGES increases? there should be an assertion to guard against this

2222–2224

for ease of understanding, I would assert that d_mask == 0

2228

keep in mind that this could hypothetically fail on iteration i and succeed on i + 1; however, I don't see this causing a problem

dougm added inline comments.
sys/vm/swap_pager.c
2205

Ideally, d_mask would be of type pn_popmap_t, or an alias for that, but that type is hidden in subr_pctrie.c. It could be exposed in the header file.

As it happens, a static assert in subr_pctrie.c guarantees that int will be big enough for int.

So, do you want a static assert about sizeof(d_mask) or to expose pn_popmap_t?

This revision now requires review to proceed.Sep 14 2024, 7:46 PM
This revision is now accepted and ready to land.Sep 15 2024, 5:53 AM
This revision was automatically updated to reflect the committed changes.
sys/vm/swap_pager.c
2190–2194

As a followup, I think that it would be good to document here how this code handles the case where the destination already has a blk at a page index.

2238–2239

... and a comment here too.