Page MenuHomeFreeBSD

swapoff to release multiple swap blocks at a time
Needs ReviewPublic

Authored by ota_j.email.ne.jp on Dec 14 2017, 6:16 AM.

Details

Reviewers
kib
alc
markj
Summary

Ref: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=223831

Improve swp_pager_force_pagein() such that "swapoff" reads multiple blocks at a time.

I used vm_thread_swapin() as a reference on how to grab pages.

Call swap_pager_haspages() to find out extra and continuous VM pages and then call vm_page_grab() to prepare multiple vm_pages_t.
swap_pager_getpages() calls vm_page_alloc() for rahead but it looks we need to call vm_page_grab(s) instead for page-in purpose.
We find up to SWB_NPAGES (32 for i386) by swap_pager_haspages().
While SWAP_META_PAGES (8 for i386) is less than SWAP_META_PAGES, we skip "sb" blocks accordingly.

It looks VM_PAGE_BITS_ALL is set for pages that have been put back from swap devices. I find regions of non-VM_PAGE_BITS_ALL for npages to getpages back.

Test Plan

case 1:
% make buildworld -j 300
While above using swap space and have sufficient space to take one off,
% swapoff <swap-device>
case 2:
% mount -t tmpfs tmpfs /mnt/tmp
% dd if=/dev/zero of=/mnt/tmp/bigfile bs=1M count=10000
While above using swap space and have sufficient space to take one off,
% swapoff <swap-device>

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

kib added inline comments.Dec 17 2017, 2:14 PM
sys/vm/swap_pager.c
1620

There must be an empty line after the '{' if function does not declare locals. Think of it as a delimiter between empty locals block and body.

1621

object is actually m->object, there is no need to pass it as the argument.

1636

Same two comments.

1667–1686

It makes sense to add assert that the object is locked.

1677

Continue is not needed there. I would write this as

for (i = 0; i < npages; i++) {
   if (m[i]->valid != VM_PAGE_BITS_ALL)
      break;
  swp_pager_activate_vmpage(m[i]);
}
1680

Better add {} around loop body, because it is multiline.

1686

Perhaps XXX comment is useless.

1687

No need for {}, loop body is single-line.

1688

return (npages);

1743

Why do you need offset variable at all ? Isn't freedpages alone contains the necessary information ?

1755

() are not needed. The '-1' seems to undo the loop step, then change the loop body to

if (sb->d[i] != SWAPBLK_NONE && swp_pager_isondev(...)) {
...
} else
   i++;

and remove -1.

kib added reviewers: alc, markj.Dec 17 2017, 2:15 PM
ota_j.email.ne.jp planned changes to this revision.Dec 30 2017, 10:28 PM
ota_j.email.ne.jp marked 5 inline comments as done.
ota_j.email.ne.jp added inline comments.
sys/vm/swap_pager.c
1667–1686

Is that one of VM_OBJECT_ASSERT_WLOCKED(object) or VM_OBJECT_ASSERT_LOCKED(object)

1677

swp_pager_force_pagein() used to only act on a single page before this change.

Now, it does for multiple npages. i < npages loop is to mark already in memory pages to "active" one by one. We don't need to page-in for these. Once find a page that we need to page-in, we need to find a continuous region of not-on-memory pages to page-in from backing store as we want to pass as big "count" as possible. There may be more than on regions within npages.

I'm changing to if-else statement and see if that makes easier to read.

1743

This was a case when we already paged-in all of the array elements of a d[] and I wanted to skip the below loop.

Combined with my comment and intention below, I can simplify. I will make change, test and then upload.

1755

I wanted to get out of the inner loop. I didn't need the exact number of pages...

Adjusted styles.
Removed object argument from 2 swp_pager_*_vmpage functions

I will update another diff after simplifying "offset" in swap_pager_swapoff().

Simplify "offset" adjustments.

ota_j.email.ne.jp marked 5 inline comments as done.Jan 3 2018, 4:26 AM
ota_j.email.ne.jp added inline comments.
sys/vm/swap_pager.c
1743

I didn't need to use offset at this location.

1755

swp_pager_isondev() never returns true for SWAPBLK_NONE case.
I think I can drop the comparison and depend on swap_pager_isondev() return value alone.

Removed #if 0 and added a blank line in swp_pager_launder_vmpage().

ota_j.email.ne.jp marked an inline comment as done.Jan 3 2018, 4:36 AM
alc added a comment.Aug 15 2018, 4:54 AM

Overall, I think that this change is okay.

sys/vm/swap_pager.c
1619

I would suggest the name swp_pager_force_dirty. In particular, the page may not be in the active queue.

1635

I would suggest the name swp_pager_force_launder.

1664

Variables of this type would conventionally be named "ma".

1672

style(9) doesn't require a blank line here. It can be deleted.

dougm_rice.edu added inline comments.Aug 15 2018, 7:46 AM
sys/vm/swap_pager.c
1661

Update the comment to state what the return value signified.

1668

I don't understand what 'm' has to do with the unexpected result from swap_pager_haspage, so I don't know why it is written out here. haspage returns TRUE or FALSE, so don't compare it to 1.

1673

I find the nested loops a bit confusing, and this a bit simpler:
for (i = j = 0;; i++) {

if (i < npages && m[i]->valid != VM_PAGE_BITS_ALL)
  continue;
if (j < i &&
    swap_pager_getpages(object, &m[j], i - j, NULL, NULL) != VM_PAGER_OK)
  panic("swp_pager_force_pagein: read from swap failed");
while (j < i)
  swp_pager_launder_vmpage(m[j++]);
if (i == npages)
  break;
swp_pager_activate_vmpage(m[j++]);

}

1681

The variable 'count' is not necessary.

1683

swap->swp in panic message

1753

It's not as if I understand this code, but still...

Suppose that on the i=SWAP_META_PAGES-1 iteration of this loop, isondev is TRUE and freedpages is set to one million. Then offset will be set to SWAP_META_PAGES=1, and on the next loop test i will be 2*SWAP_META_PAGES-2, not SWAP_META_PAGES. So I know that we're leaving the loop and i won't be examined again, but is it bad that i is so big? Shouldn't offset be limited so that i can't go past SWAP_META_PAGES, and freedpages can't be reduced so much?