Defer and aggregate swap_pager_meta_build frees

[dougm]

Summary

Before swp_pager_meta_build replaces an old swapblk with an new one, it frees the old one. To allow such freeing of blocks to be aggregated, have swp_pager_meta_build return the old swap block, and make the caller responsible for freeing it.

Define a pair of short static functions, swp_pager_init_freerange and swp_pager_update_freerange, to do the initialization and updating of blk addresses and counters used in aggregating blocks to be freed.

[dougm]

Add missing declaration.

[kib]

sys/vm/swap_pager.c

414 ↗

(On Diff #37289)

Blank line is needed after '{'.

421 ↗

(On Diff #37289)

Same. Also please put {} around then case.

1416 ↗

(On Diff #37289)

De-split this line, please. There is a lot of free space left, the call does not need to take 5 lines.

[dougm]

Apply reviewer suggestions.

[markj]

sys/vm/swap_pager.c

991 ↗

(On Diff #37918)

"source block"?

[dougm]

Add recommended space to one comment.

[alc]

sys/vm/swap_pager.c

986–987 ↗

(On Diff #37939)

Don't we expect srcaddr to be SWAPBLK_NONE here, i.e., we already determined that there is no existing allocated block? So, this should actually be

KASSERT(srcaddr == SWAPBLK_NONE, ...
continue;

or drop the continue and restore the else.

[dougm]

sys/vm/swap_pager.c

986–987 ↗

(On Diff #37939)

We do. I'll take the latter option.

[dougm]

Accept a suggestion to remove an unnecessary conditional 'continue'.

[alc]

sys/vm/swap_pager.c

982 ↗

(On Diff #46317)

There is no point in this assignment, unless it is followed by a KASSERT(srcaddr == SWAPBLK_NONE, ...); That said, given that this is the destination object, it might be clearer if dstaddr were used here instead.

[dougm]

Add KASSERT on swap_pager_meta_build return value. Use a 'continue' to allow a big indented block to be outdented.

[alc]

Kostik, Mark,

I have a question about vm_object_split() and swap_pager_copy(). vm_object_split() is indirectly dirtying every transferred, because vm_page_rename() always performs vm_page_dirty(). But, that dirtying is not freeing the allocated swap. Instead, we are copying it. The obvious thing would be to release the swap when the page is dirtied, but does the vm_page_rename() by vm_object_split() actually need to dirty the page given that it is also performing swap_pager_copy()? If a transferred page is reclaimed, because it is currently clean, then we should be able to recover the data from swap.

[markj]

In D13707#352645, @alc wrote:

Kostik, Mark,

I have a question about vm_object_split() and swap_pager_copy(). vm_object_split() is indirectly dirtying every transferred, because vm_page_rename() always performs vm_page_dirty(). But, that dirtying is not freeing the allocated swap. Instead, we are copying it. The obvious thing would be to release the swap when the page is dirtied, but does the vm_page_rename() by vm_object_split() actually need to dirty the page given that it is also performing swap_pager_copy()? If a transferred page is reclaimed, because it is currently clean, then we should be able to recover the data from swap.

I can't see any problem with the reasoning the last sentence. The behaviour of vm_page_rename() marking the page dirty was introduced in r42957, but existed in vm_map_split() even before that: it was added in r35571 with a somewhat enigmatic commit log message. At that point, we weren't busying each page during the transfer - before that was added, I think the dirtying would have been necessary to prevent a race with reclamation by the page daemon?

[alc]

sys/vm/swap_pager.c

1350–1351 ↗

(On Diff #46318)

This should also be a KASSERT(addr == SWAPBLK_NONE, ...

[kib]

In D13707#352661, @markj wrote:

In D13707#352645, @alc wrote:

Kostik, Mark,

I have a question about vm_object_split() and swap_pager_copy(). vm_object_split() is indirectly dirtying every transferred, because vm_page_rename() always performs vm_page_dirty(). But, that dirtying is not freeing the allocated swap. Instead, we are copying it. The obvious thing would be to release the swap when the page is dirtied, but does the vm_page_rename() by vm_object_split() actually need to dirty the page given that it is also performing swap_pager_copy()? If a transferred page is reclaimed, because it is currently clean, then we should be able to recover the data from swap.

I can't see any problem with the reasoning the last sentence. The behaviour of vm_page_rename() marking the page dirty was introduced in r42957, but existed in vm_map_split() even before that: it was added in r35571 with a somewhat enigmatic commit log message. At that point, we weren't busying each page during the transfer - before that was added, I think the dirtying would have been necessary to prevent a race with reclamation by the page daemon?

As I see, vm_object_scan() does exactly this: vm_object_collapse_scan() renames the page and then frees the backing swap.

[dougm]

Replace another conditional freerange with a KASSERT, as instructed.

[dougm]

Alphabetically order variable definitions. Introduce standard whitespace before block comment.

[alc]

Peter, could you please test this patch.

[pho]

In D13707#352682, @alc wrote:

Peter, could you please test this patch.

I tested this with all of the stress2 tests on amd64. I also did a buildworld.
On i386 I ran about half of the tests, until I got an unrelated PMC panic.
LGTM.