Subject: Re: pmap_is_referenced() -- not used?
To: Chuck Cranor <chuck@xxx.research.att.com>
From: Jason R Thorpe <thorpej@zembu.com>
List: tech-kern
Date: 01/24/2001 18:30:12
On Wed, Jan 24, 2001 at 08:04:56PM -0500, Chuck Cranor wrote:

 > i think you should be careful about not protecting it.   by placing
 > a page that has possible active mappings on the inactive queue you are
 > violating one of the assumptions that i wrote the rest of the code with
 > and it could cause trouble later....

I'm now running with the following patch.  Things seem to be a fair
bit better.  I haven't had a chance to really stress it out yet, but
I'm doing a few parallel compiles, and things seem okay so far.

-- 
        -- Jason R. Thorpe <thorpej@zembu.com>

Index: uvm_anon.c
===================================================================
RCS file: /cvsroot/syssrc/sys/uvm/uvm_anon.c,v
retrieving revision 1.13
diff -c -r1.13 uvm_anon.c
*** uvm_anon.c	2001/01/23 02:27:39	1.13
--- uvm_anon.c	2001/01/25 02:24:30
***************
*** 526,532 ****
  	 */
  
  	pmap_clear_reference(pg);
! 	pmap_page_protect(pg, VM_PROT_NONE);
  	uvm_lock_pageq();
  	uvm_pagedeactivate(pg);
  	uvm_unlock_pageq();
--- 526,532 ----
  	 */
  
  	pmap_clear_reference(pg);
! 	pmap_page_protect(pg, VM_PROT_NONE);	/* XXX unnecessary? */
  	uvm_lock_pageq();
  	uvm_pagedeactivate(pg);
  	uvm_unlock_pageq();
Index: uvm_aobj.c
===================================================================
RCS file: /cvsroot/syssrc/sys/uvm/uvm_aobj.c,v
retrieving revision 1.37
diff -c -r1.37 uvm_aobj.c
*** uvm_aobj.c	2000/11/25 06:27:59	1.37
--- uvm_aobj.c	2001/01/25 02:24:31
***************
*** 882,890 ****
  			    pp->wire_count != 0)
  				continue;
  
- 			/* zap all mappings for the page. */
- 			pmap_page_protect(pp, VM_PROT_NONE);
- 
  			/* ...and deactivate the page. */
  			uvm_pagedeactivate(pp);
  
--- 882,887 ----
***************
*** 1543,1549 ****
  	 * deactivate the page (to put it on a page queue).
  	 */
  	pmap_clear_reference(pg);
! 	pmap_page_protect(pg, VM_PROT_NONE);
  	uvm_lock_pageq();
  	uvm_pagedeactivate(pg);
  	uvm_unlock_pageq();
--- 1540,1546 ----
  	 * deactivate the page (to put it on a page queue).
  	 */
  	pmap_clear_reference(pg);
! 	pmap_page_protect(pg, VM_PROT_NONE);	/* XXX unnecessary? */
  	uvm_lock_pageq();
  	uvm_pagedeactivate(pg);
  	uvm_unlock_pageq();
Index: uvm_fault.c
===================================================================
RCS file: /cvsroot/syssrc/sys/uvm/uvm_fault.c,v
retrieving revision 1.54
diff -c -r1.54 uvm_fault.c
*** uvm_fault.c	2001/01/23 02:27:39	1.54
--- uvm_fault.c	2001/01/25 02:24:32
***************
*** 205,211 ****
  		if (pg && (pg->flags & PG_BUSY) == 0 && pg->loan_count == 0) {
  			uvm_lock_pageq();
  			if (pg->wire_count == 0) {
- 				pmap_page_protect(pg, VM_PROT_NONE);
  				uvm_pagedeactivate(pg);
  			}
  			uvm_unlock_pageq();
--- 205,210 ----
Index: uvm_map.c
===================================================================
RCS file: /cvsroot/syssrc/sys/uvm/uvm_map.c,v
retrieving revision 1.88
diff -c -r1.88 uvm_map.c
*** uvm_map.c	2001/01/14 02:10:01	1.88
--- uvm_map.c	2001/01/25 02:24:35
***************
*** 2566,2574 ****
  				}
  				KASSERT(pg->uanon == anon);
  
- 				/* zap all mappings for the page. */
- 				pmap_page_protect(pg, VM_PROT_NONE);
- 
  				/* ...and deactivate the page. */
  				uvm_pagedeactivate(pg);
  
--- 2566,2571 ----
Index: uvm_page_i.h
===================================================================
RCS file: /cvsroot/syssrc/sys/uvm/uvm_page_i.h,v
retrieving revision 1.15
diff -c -r1.15 uvm_page_i.h
*** uvm_page_i.h	2001/01/14 02:10:02	1.15
--- uvm_page_i.h	2001/01/25 02:24:35
***************
*** 193,199 ****
  }
  
  /*
!  * uvm_pagedeactivate: deactivate page -- no pmaps have access to page
   *
   * => caller must lock page queues
   * => caller must check to make sure page is not wired
--- 193,199 ----
  }
  
  /*
!  * uvm_pagedeactivate: deactivate page
   *
   * => caller must lock page queues
   * => caller must check to make sure page is not wired
***************
*** 218,225 ****
  		pg->pqflags |= PQ_INACTIVE;
  		uvmexp.inactive++;
  		pmap_clear_reference(pg);
- 		if (pmap_is_modified(pg))
- 			pg->flags &= ~PG_CLEAN;
  	}
  }
  
--- 218,223 ----
Index: uvm_pager.c
===================================================================
RCS file: /cvsroot/syssrc/sys/uvm/uvm_pager.c,v
retrieving revision 1.38
diff -c -r1.38 uvm_pager.c
*** uvm_pager.c	2000/12/09 23:26:27	1.38
--- uvm_pager.c	2001/01/25 02:24:35
***************
*** 315,325 ****
  	/*
  	 * attempt to cluster around the left [backward], and then 
  	 * the right side [forward].    
- 	 *
- 	 * note that for inactive pages (pages that have been deactivated)
- 	 * there are no valid mappings and PG_CLEAN should be up to date.
- 	 * [i.e. there is no need to query the pmap with pmap_is_modified
- 	 * since there are no mappings].
  	 */
  
  	for (forward  = 0 ; forward <= 1 ; forward++) {
--- 315,320 ----
***************
*** 333,356 ****
  			if (pclust == NULL) {
  				break;			/* no page */
  			}
- 			/* handle active pages */
- 			/* NOTE: inactive pages don't have pmap mappings */
- 			if ((pclust->pqflags & PQ_INACTIVE) == 0) {
- 				if ((flags & PGO_DOACTCLUST) == 0) {
- 					/* dont want mapped pages at all */
- 					break;
- 				}
  
! 				/* make sure "clean" bit is sync'd */
! 				if ((pclust->flags & PG_CLEANCHK) == 0) {
! 					if ((pclust->flags & (PG_CLEAN|PG_BUSY))
! 					   == PG_CLEAN &&
! 					   pmap_is_modified(pclust))
! 						pclust->flags &= ~PG_CLEAN;
  
! 					/* now checked */
! 					pclust->flags |= PG_CLEANCHK;
! 				}
  			}
  
  			/* is page available for cleaning and does it need it */
--- 328,355 ----
  			if (pclust == NULL) {
  				break;			/* no page */
  			}
  
! 			if ((flags & PGO_DOACTCLUST) == 0) {
! 				/* dont want mapped pages at all */
! 				break;
! 			}
! 
! 			/*
! 			 * get an up-to-date view of the "clean" bit.
! 			 * note this isn't 100% accurate, but it doesn't
! 			 * have to be.  if it's not quite right, the
! 			 * worst that happens is we don't cluster as
! 			 * aggressively.  we'll sync-it-for-sure before
! 			 * we free the page, and clean it if necessary.
! 			 */
! 			if ((pclust->flags & PG_CLEANCHK) == 0) {
! 				if ((pclust->flags & (PG_CLEAN|PG_BUSY))
! 				    == PG_CLEAN &&
! 				   pmap_is_modified(pclust))
! 					pclust->flags &= ~PG_CLEAN;
  
! 				/* now checked */
! 				pclust->flags |= PG_CLEANCHK;
  			}
  
  			/* is page available for cleaning and does it need it */
Index: uvm_pdaemon.c
===================================================================
RCS file: /cvsroot/syssrc/sys/uvm/uvm_pdaemon.c,v
retrieving revision 1.28
diff -c -r1.28 uvm_pdaemon.c
*** uvm_pdaemon.c	2001/01/25 00:24:48	1.28
--- uvm_pdaemon.c	2001/01/25 02:24:36
***************
*** 533,542 ****
  
  			/*
  			 * we now have the object and the page queues locked.
! 			 * the page is not busy.   if the page is clean we
! 			 * can free it now and continue.
  			 */
  
  			if (p->flags & PG_CLEAN) {
  				if (p->pqflags & PQ_SWAPBACKED) {
  					/* this page now lives only in swap */
--- 533,548 ----
  
  			/*
  			 * we now have the object and the page queues locked.
! 			 * the page is not busy.  remove all the permissions
! 			 * from the page so we can sync the modified info
! 			 * without any race conditions.  if the page is clean
! 			 * we can free it now and continue.
  			 */
  
+ 			pmap_page_protect(p, VM_PROT_NONE);
+ 			if ((p->flags & PG_CLEAN) != 0 && pmap_is_modified(p))
+ 				p->flags &= ~PG_CLEAN;
+ 
  			if (p->flags & PG_CLEAN) {
  				if (p->pqflags & PQ_SWAPBACKED) {
  					/* this page now lives only in swap */
***************
*** 1105,1111 ****
  
  		if (inactive_shortage > 0 &&
  		    pmap_clear_reference(p) == FALSE) {
- 			pmap_page_protect(p, VM_PROT_NONE);
  			/* no need to check wire_count as pg is "active" */
  			uvm_pagedeactivate(p);
  			uvmexp.pddeact++;
--- 1111,1116 ----
Index: uvm_vnode.c
===================================================================
RCS file: /cvsroot/syssrc/sys/uvm/uvm_vnode.c,v
retrieving revision 1.41
diff -c -r1.41 uvm_vnode.c
*** uvm_vnode.c	2001/01/08 06:21:13	1.41
--- uvm_vnode.c	2001/01/25 02:24:37
***************
*** 540,546 ****
  			 */
  			if ((pp->flags & PG_CLEAN) != 0 && 
  			    (flags & PGO_FREE) != 0 &&
! 			    (pp->pqflags & PQ_ACTIVE) != 0)
  				pmap_page_protect(pp, VM_PROT_NONE);
  			if ((pp->flags & PG_CLEAN) != 0 &&
  			    pmap_is_modified(pp))
--- 540,547 ----
  			 */
  			if ((pp->flags & PG_CLEAN) != 0 && 
  			    (flags & PGO_FREE) != 0 &&
! 			    /* XXX ACTIVE|INACTIVE test unnecessary? */
! 			    (pp->pqflags & (PQ_ACTIVE|PQ_INACTIVE)) != 0)
  				pmap_page_protect(pp, VM_PROT_NONE);
  			if ((pp->flags & PG_CLEAN) != 0 &&
  			    pmap_is_modified(pp))
***************
*** 564,570 ****
  				if ((pp->pqflags & PQ_INACTIVE) == 0 &&
  				    (pp->flags & PG_BUSY) == 0 &&
  				    pp->wire_count == 0) {
- 					pmap_page_protect(pp, VM_PROT_NONE);
  					uvm_pagedeactivate(pp);
  				}
  
--- 565,570 ----
***************
*** 756,762 ****
  				if ((pp->pqflags & PQ_INACTIVE) == 0 &&
  				    (pp->flags & PG_BUSY) == 0 &&
  				    pp->wire_count == 0) {
- 					pmap_page_protect(ptmp, VM_PROT_NONE);
  					uvm_pagedeactivate(ptmp);
  				}
  			} else if (flags & PGO_FREE) {
--- 756,761 ----