Page MenuHomeFreeBSD

Defer and aggregate swap_pager_meta_build frees
ClosedPublic

Authored by dougm_rice.edu on Dec 31 2017, 5:03 AM.

Details

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.

Diff Detail

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

dougm_rice.edu created this revision.

Add missing declaration.

kib accepted this revision.Jan 13 2018, 12:46 PM
kib added inline comments.
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.

This revision is now accepted and ready to land.Jan 13 2018, 12:46 PM
dougm_rice.edu marked 3 inline comments as done.

Apply reviewer suggestions.

This revision now requires review to proceed.Jan 13 2018, 5:08 PM
markj accepted this revision.Jan 14 2018, 5:22 PM
markj added inline comments.
sys/vm/swap_pager.c
991 ↗(On Diff #37918)

"source block"?

This revision is now accepted and ready to land.Jan 14 2018, 5:22 PM

Add recommended space to one comment.

This revision now requires review to proceed.Jan 14 2018, 5:37 PM
alc added inline comments.Aug 5 2018, 6:09 PM
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_rice.edu added inline comments.Aug 5 2018, 6:42 PM
sys/vm/swap_pager.c
986–987 ↗(On Diff #37939)

We do. I'll take the latter option.

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

alc added inline comments.Aug 5 2018, 7:17 PM
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.

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

alc added a comment.Aug 5 2018, 8:07 PM

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 added a comment.Aug 5 2018, 9:54 PM
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 added inline comments.Aug 5 2018, 10:21 PM
sys/vm/swap_pager.c
1350–1351 ↗(On Diff #46318)

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

kib added a comment.Aug 5 2018, 10:35 PM
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.

Replace another conditional freerange with a KASSERT, as instructed.

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

alc accepted this revision.Aug 5 2018, 11:47 PM
This revision is now accepted and ready to land.Aug 5 2018, 11:47 PM
alc added a subscriber: pho.Aug 5 2018, 11:48 PM

Peter, could you please test this patch.

pho added a comment.Aug 7 2018, 6:47 AM
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.

This revision was automatically updated to reflect the committed changes.