DragonFlyBSD

:Hi,
:
:
:Should restore hysteresis to the idle zero logic. The simplest way to do
:this was to add the zero_state set to vm_page_zero_check().
:
:-- vs

Hmm.  That still doesn't look quite right.  I think the LO and the HI
    are reversed.  How about this instead?

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

diff --git a/sys/vm/vm_zeroidle.c b/sys/vm/vm_zeroidle.c
index 276cc3a..02a7515 100644
--- a/sys/vm/vm_zeroidle.c
+++ b/sys/vm/vm_zeroidle.c
@ -35,8 +35,6 @ * from: @(#)vm_machdep.c 7.3 (Berkeley) 5/13/91 * Utah $Hdr: vm_machdep.c 1.16.1.1 89/06/23$ * from FreeBSD: .../i386/vm_machdep.c,v 1.165 2001/07/04 23:27:04 dillon
- *
- * $Id: vm_zeroidle.c,v 1.3 2010/05/12 04:50:45 sv5679 Exp $
*/

#include <sys/param.h>
@ -67,7 +65,6 @
 /* Maximum number of pages per second to zero */
 #define NPAGES_RUN    (20000)

-
static int idlezero_enable = 0;
TUNABLE_INT("vm.idlezero_enable", &idlezero_enable);
SYSCTL_INT(_vm, OID_AUTO, idlezero_enable, CTLFLAG_RW, &idlezero_enable, 0,
@ -99,17 +96,30 @ static int zero_state; * Otherwise we might get 'flutter' during disk I/O / IPC or * fast sleeps. We also do not want to be continuously zeroing * pages because doing so may flush our L1 and L2 caches too much.
+ *
+ * Returns non-zero if pages should be zerod.
*/
static int
vm_page_zero_check(void) {
if (idlezero_enable == 0)
return (0);
- if (zero_state && vm_page_zero_count >= ZIDLE_LO(vmstats.v_free_count))
- return (0);
- if (vm_page_zero_count >= ZIDLE_HI(vmstats.v_free_count))
- return (0);
- return (1);
+ if (zero_state == 0) {
+ /*
+ * Wait for the count to fall to LO before starting
+ * to zero pages.
+ /
+ if (vm_page_zero_count <= ZIDLE_LO(vmstats.v_free_count))
+ zero_state = 1;
+ } else {
+ /
+ * Once we are zeroing pages wait for the count to
+ * increase to HI before we stop zeroing pages.
+ */
+ if (vm_page_zero_count >= ZIDLE_HI(vmstats.v_free_count))
+ zero_state = 0;
+ }
+ return (zero_state);
}

static void

As written I think the patch behaves like this:

if (zero_state == 0) /* We've not hit the upper limit /
if (zeroed pages >= high count) / hit the upper limit */
zero_state = 1;
return STOP_ZEROING_PAGES;
return KEEP_ZEROING_PAGES;

if (zero_state == 1) /* We've hit the upper limit before /
if (zeroed_pages >= LOW MARK) / Still have free pages; also covers high /
return STOP_ZEROING_PAGES;
/ We've seen an upper limit; now we're below the lower limit. /
/ Restore to 0 (not seen high limit) */
zero_state = 0;
return KEEP_ZEROING_PAGES;

What was wrong with this?

-- vs

:As written I think the patch behaves like this:
:
:if (zero_state == 0) /* We've not hit the upper limit /
: if (zeroed pages >= high count) / hit the upper limit /
: zero_state = 1;
: return STOP_ZEROING_PAGES;
: return KEEP_ZEROING_PAGES;
:
:if (zero_state == 1) / We've hit the upper limit before /
: if (zeroed_pages >= LOW MARK) / Still have free pages; also covers high /
: return STOP_ZEROING_PAGES;
: / We've seen an upper limit; now we're below the lower limit. /
: / Restore to 0 (not seen high limit) */
: zero_state = 0;
: return KEEP_ZEROING_PAGES;
:
:
:What was wrong with this?
:
:-- vs

Well, which bit of code is easier to read?

-Matt

:Well, which bit of code is easier to read?

Hah, no question about that. I just didn't see what I'd done wrong.

-- vs

Resolved by commit a863c5ec14d2c6ac27c1d1a9d01f5cd1f6bf6d72.