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.
Details
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Allow the possibility of freeing the block and retrying without releasing the lock before finally letting go of the lock and retrying.
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. |
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. |
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.
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 |
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? |