Re: VM idle page zeroing

[Matthew Dillon <dillon@xxxxxxxxxxxxxxxxxxxx>]

:Hi,
:
:Here is a second pass at this; combine the last three patches in git to get
:a total picture:
:
:http://gitweb.dragonflybsd.org/~vsrinivas/dragonfly_zerothread.git
:
:Any comments would be nice; in particular, the vm_pagezero() function turned
:out more complex than I'd hoped, any help restructuring it would rock
:
:Thanks, world!

    I added the export file to the repo so it can be exported.  People should
    use the git:// path instead:

	git://leaf.dragonflybsd.org/~vsrinivas/dragonfly_zerothread.git

    Also when someone gets the chance on IRC give Venkatesh a run-down
    on how to use branch names.

    --

    Ok, here are my comments:

    bzeront addition:

	Could probably be moved to sys/cpu to reduce code duplication (just
	have one for x86_64 and one for i386 instead of one for each
	platform). 

	This was my bad, on IRC I suggested platform/ but now that I see
	the code it is clear it should be in cpu/

    cnt_prezero sysctl

	should probably be moved to vm_zeroidle.c

    TDPRI_IDLE_WORK

	good.

    vm_page_free_fromq_fast() and vm_page_free_toq_fast()

	Hmm.  I would say maybe have vm_page_free_fromq_fast() busy the
	VM page, leave it busy through the entire zeroing process, and
	then get rid of vm_page_free_toq_fast() and use the normal
	page freeing function.  Set PG_ZERO in vm_zeroidle.c after
	successfully zeroing the page.

    vm_zeroidle.c

	Adjust syntax for comments a bit to match the rest of the codebase.
	e.g.:

		/* comment
		 * comment
		 */

		to

		/*
		 * comment
		 * comment
		 */

	NTSTORE_AVAIL conditionalization needs to be cleaned up.  Make a
	cpu_*() call in the platform code to return whether the feature is
	available or not and set a local variable outside the zeroing loop.

	The main loop is a bit confusing with all the gotos.  I recommend
	a state machine switch.

	* Change SUSPENDED_* to an enumeration.

	* switch() on the state.

	* fall through to the tsleep or place the tsleep before the break in
	  each case statement.

	* Your tsleep() is set to sleep for 5 minutes?  I think you might
	  want to change that to, say, once a second (hz instead of hz * 300).
	  In fact, you don't really need to have a call-in for the wakeup.
	  The loop can just run off an internal sleep/timer and calculate
	  a burst of work to do.

	* Add an additional state for the normal 'waiting for some work to
	  do' case which can then set the state to the page acquisition state
	  and fall through.  Also make the state names more informative.
	  like this:

	  (Ignore my indentations here, I'm just throwing this down quickly)

	  for (;;) {
	      switch(state) {
	      case STATE_IDLE:
		    tsleep(&zero_state, 0, "idle", hz);
		    npages = (calculate number of pages to try to zero)
		    if (npages)
			    state = STATE_GET_PAGE;
		    break;
	      case STATE_GET_PAGE:
		    if (try_mplock() == 0) {
			    tsleep(&zero_state, 0, "idle", 1);
			    /* try again soon */
			    break;
		    }
		    if (--npages == 0) {
			    state = STATE_IDLE;
		    } else {
			    m = vm_page_free_fromq_fast();
			    if (m == NULL) {
				    state = STATE_IDLE;
			    } else {
				    state = STATE_ZERO_PAGE;
				    buf = .... get the lwbuf
			    }
		    }
		    rel_mplock();
		    break;
	      case STATE_ZERO_PAGE:
		    ... loop to zero page ....
		    ... just restart state if we have to break out due
			to a resched ...
		    break;
	      case STATE_RELEASE_PAGE:
		    if (try_mplock() == 0)
			break;

		    ... release the page, release the lwbuf, etc ...
		    state = STATE_GET_PAGE;
		    rel_mplock();
		    break;
	      }
	      lwkt_switch();
	 }

    Something like that.

						-Matt