swap_pager: handle swblk being freed while lock dropped in meta_transfer

[dougm]

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.

[dougm]

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

[markj]

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

sys/vm/swap_pager.c

2221–2222

There should be an extra newline after variable declarations.

2259

Can we get away without having both pindex and p?

[dougm]

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.

[markj]

sys/vm/swap_pager.c

2228

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

2248

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.

[dougm]

sys/vm/swap_pager.c

2228

Yes. Careless haste on my part.

2248

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

[dougm]

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

[markj]

sys/vm/swap_pager.c

2252

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.

[dougm]

sys/vm/swap_pager.c

2252

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.

[dougm]

A version without bitmasking. Perhaps this is acceptable?

[markj]

sys/vm/swap_pager.c

2252

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.

[dougm]

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

[alc]

sys/vm/swap_pager.c

2204

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

2221

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

2226

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]

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?

[dougm]

Address @alc feedback.

[alc]

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.

2236–2237

... and a comment here too.