Bug 221356 - [patch] Improve swapon_check_swzone() function in swap_pager.c
Summary: [patch] Improve swapon_check_swzone() function in swap_pager.c
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 11.1-RELEASE
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-bugs (Nobody)
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2017-08-09 05:22 UTC by ota
Modified: 2017-10-17 02:47 UTC (History)
4 users (show)

See Also:


Attachments
Improve swapon_check_swzone() function (3.81 KB, patch)
2017-08-09 05:22 UTC, ota
no flags Details | Diff
NO_SWAPPING in vm_pageout() (418 bytes, patch)
2017-10-11 05:08 UTC, ota
no flags Details | Diff
set vm_swap_enabled=1 after swap zones are initialized (3.09 KB, patch)
2017-10-11 05:22 UTC, ota
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ota 2017-08-09 05:22:20 UTC
Created attachment 185184 [details]
Improve swapon_check_swzone() function

Improve swapon_check_swzone() function.
 
Change to more descriptive swapon_check_swzone() warnings.
#1 Indicate half the size is recommended size.
(I couldn't figure out and don't know why half is recommended, though.)
#2 Advice when the swap device size exceeds the max size.

Convert swap_pager_full to static variable as it is not used in global space.
Add a unit to swap_total description.
Call uma_zone_get_max() to find proper swap_maxpages.
Use swap_maxpages and other global variable in swapon_check_swzone().
Convert swapon_check_swzone() to a void function.


Patch tested on 11.1-RELEASE and compiled on CURRENT of i386.

How to test
$ swapon <extra devices>
until you exceed these 2 thresholds - half size and the max size.
Easier to test on systems with less memory.
Comment 1 Konstantin Belousov freebsd_committer freebsd_triage 2017-08-12 21:00:59 UTC
There is WIP https://reviews.freebsd.org/D11435 which affects significant part of the patch.  In particular, the zone limiting bits might go out altogether.  I suggest to wait for D11435 to land, hopefully some time soon, and then rebase the patch.
Comment 2 ota 2017-08-13 16:22:16 UTC
Thank you for your feedback.

I looked at https://reviews.freebsd.org/D11435 and my patch doesn't interfere much yet.

Your side of changes looks quite involved and changes how swap information is managed.  Do you plan to MFC to 11/stable?
Comment 3 Ed Maste freebsd_committer freebsd_triage 2017-08-30 02:57:12 UTC
D11435 is now committed with a planned MFC near end of September.
Comment 4 commit-hook freebsd_committer freebsd_triage 2017-08-30 09:44:24 UTC
A commit references this bug:

Author: kib
Date: Wed Aug 30 09:44:05 UTC 2017
New revision: 323017
URL: https://svnweb.freebsd.org/changeset/base/323017

Log:
  Make the swap_pager_full variable static.

  r290920 removed the use of the variable from vm/vm_pageout.c.

  Submitted by:	ota@j.email.ne.jp
  PR:	221356
  MFC after:	1 week

Changes:
  head/sys/vm/swap_pager.c
  head/sys/vm/swap_pager.h
Comment 5 Konstantin Belousov freebsd_committer freebsd_triage 2017-08-30 09:49:42 UTC
(In reply to commit-hook from comment #4)
As you see, I committed one trivial bit of your patch.  Please re-merge with the HEAD if needed and put the rest of it on reviews.freebsd.org.
Comment 6 commit-hook freebsd_committer freebsd_triage 2017-08-30 10:17:52 UTC
A commit references this bug:

Author: kib
Date: Wed Aug 30 10:17:00 UTC 2017
New revision: 323018
URL: https://svnweb.freebsd.org/changeset/base/323018

Log:
  Adjust interface of swapon_check_swzone() to its actual usage.

  The function return value is not used.  Its argument is always
  swap_total/PAGE_SIZE, so make it not take any arguments.

  Submitted by:	ota@j.email.ne.jp
  PR:	221356
  MFC after:	1 week

Changes:
  head/sys/vm/swap_pager.c
Comment 7 commit-hook freebsd_committer freebsd_triage 2017-09-06 07:05:06 UTC
A commit references this bug:

Author: kib
Date: Wed Sep  6 07:04:29 UTC 2017
New revision: 323209
URL: https://svnweb.freebsd.org/changeset/base/323209

Log:
  MFC r323017:
  Make the swap_pager_full variable static.

  PR:	221356

Changes:
_U  stable/11/
  stable/11/sys/vm/swap_pager.c
  stable/11/sys/vm/swap_pager.h
Comment 8 commit-hook freebsd_committer freebsd_triage 2017-09-06 07:07:11 UTC
A commit references this bug:

Author: kib
Date: Wed Sep  6 07:06:59 UTC 2017
New revision: 323210
URL: https://svnweb.freebsd.org/changeset/base/323210

Log:
  MFC r323018:
  Adjust interface of swapon_check_swzone() to its actual usage.

  PR:	221356

Changes:
_U  stable/11/
  stable/11/sys/vm/swap_pager.c
Comment 9 ota 2017-10-11 05:08:44 UTC
Created attachment 187070 [details]
NO_SWAPPING in vm_pageout()

After some experiments and reading swap_pager.c, we can put NO_SWAPPING macro to disable swapping.
Comment 10 ota 2017-10-11 05:12:33 UTC
Thank you for taking few pieces of the 1st patch.
After your announced changes and these patches got into 11-stable, I've been experimenting with swap.

I prepared 2 additional patch sets; one is above and the other is below.
Comment 11 ota 2017-10-11 05:22:34 UTC
Created attachment 187071 [details]
set vm_swap_enabled=1 after swap zones are initialized

This is a proposal and not an complete change.

One purpose is set vm_swap_enabled to 1 after swap initialization is completed.

Another purpose is to avoid/reduce panic().  Given the change tested with "NO_SWAPPING in vm_pageout()", if we set zones to NULL, we can effectively disable swapping.

The catch with this change set is 1st uma_zone_reserve_kva() allocated in KVA for swpctrie_zone won't be freed if 2nd uma_zone_reserve_kva() for swblk_zone fails and tries to recover.  I couldn't find proper UMA API to release reserved space.
Also, we may not need to check swblk_zone here.
Comment 12 Konstantin Belousov freebsd_committer freebsd_triage 2017-10-13 15:28:23 UTC
(In reply to ota from comment #11)
NO_SWAPPING and vm_swap_enabled control the swap-out activity, this is the action almost unrelated to the page swapping.  If the swap pager zones are left NULL, then pageout (not swapout) would trap when pagedaemon tries to page out anonymous memory.  For the same reason the vm_swap_enabled variable cannot be used to avoid the issue, the variable control the swapping.

In the modern FreeBSD, the process is considered swapped out when all its threads kernel stacks are marked as pageable.  In this state, the process' thread is never allowed to be made runnable.  Idea is that non-running process does not reference its private pages and they are eventually get paged out.

So the two later patches need serious rework.  The first patch is still not re-merged with the latest HEAD code.  I only found one bit which might be taken as is from it, see https://reviews.freebsd.org/D12660.
Comment 13 commit-hook freebsd_committer freebsd_triage 2017-10-13 16:23:34 UTC
A commit references this bug:

Author: kib
Date: Fri Oct 13 16:23:05 UTC 2017
New revision: 324600
URL: https://svnweb.freebsd.org/changeset/base/324600

Log:
  Evaluate the real size of the sblk_zone.

  Submitted by:	ota@j.email.ne.jp
  PR:	221356
  Reviewed by:	alc, markj
  MFC after:	1 week
  Differential revision:	https://reviews.freebsd.org/D12660

Changes:
  head/sys/vm/swap_pager.c
Comment 14 ota 2017-10-14 06:05:42 UTC
(In reply to Konstantin Belousov from comment #12)

Thank you for your feedback and comments for the 2nd and 3rd ones.
I wasn't much confident about the change myself.  It is good to hear from a person who is familiar.
I haven't been paying enough attention to page out and swap out while I read the code; I will pay more attention to these going forward.


I didn't know you were still working on the 1st one.
Among 3 machines I have and tried, I had one machine that reserved more UMA zone than requested.
I think "changed" is better than "reduced" in the wording for this case.

i.e. copied from such machine:
Swap blk zone entries reduced from 58663 to 58688.

Or change the wording completely like
Swap blk zone reserved for 58688 entries.
Comment 15 Konstantin Belousov freebsd_committer freebsd_triage 2017-10-14 08:48:03 UTC
(In reply to ota from comment #14)
I definitely did not worked on, and do not plan to do a work on the patch1.  I am only committing bits which are ready and easy to understand and extract.  I think that there are some useful ideas left uncommitted in the patch1, which is why I am asking you to rebase the patch on the current HEAD code.
Comment 16 ota 2017-10-17 02:47:12 UTC
(In reply to Konstantin Belousov from comment #15)

I think you already took good pieces out of the patch1 and will be closing this one.

I'm still testing and check few pieces in patch1 but I am not sure about these yet.
If they are also useful, I will enter another report and subscribe people who are in this PR.
I am indeed entering a new one related to sw_nblks, shortly.