Subject: Re: amap_extend does unnecessary malloc/memcpy.
To: None <sommerfeld@orchard.arlington.ma.us>
From: enami tsugutomo <enami@sm.sony.co.jp>
List: tech-kern
Date: 11/20/2001 14:41:44
Bill Sommerfeld <sommerfeld@orchard.arlington.ma.us> writes:

> > +	if (size <= MAXALLOCSAVE)
> > +		slotalloc = (1 << BUCKETINDX(size)) / sizeof(int);
> > +	else
> > +		slotalloc = roundup(size, PAGE_SIZE) / sizeof(int);
> 
> Rather than spreading implementation details about our kernel malloc
> through the unrelated code elsewhere in the kernel, how about defining
> a malloc_roundup() function or MALLOC_ROUNDUP() macro in malloc.h?

Cool.  I prefer this idea.  The function may be useful in some other
place.

Here is a revised version.

- We can use malloc_roundup() on initial allocation.
- Since we don't zero clear am_bckptr on initial allocation, clearing
  rest of bckptr when extending amap shouldn't be necessary.

enami.

Index: uvm_amap.c
===================================================================
RCS file: /cvsroot/syssrc/sys/uvm/uvm_amap.c,v
retrieving revision 1.37
diff -u -r1.37 uvm_amap.c
--- uvm_amap.c	2001/11/10 07:36:59	1.37
+++ uvm_amap.c	2001/11/20 05:31:45
@@ -167,6 +167,22 @@
 }
 
 /*
+ * malloc_roundup: roundup size to the real allocation size.
+ * XXX this will goto malloc.[ch].
+ */
+size_t malloc_roundup __P((size_t));
+size_t
+malloc_roundup(size)
+	size_t size;
+{
+
+	if (size > MAXALLOCSAVE)
+		return (roundup(size, PAGE_SIZE));
+	else
+		return (1 << BUCKETINDX(size));
+}
+
+/*
  * amap_alloc1: internal function that allocates an amap, but does not
  *	init the overlay.
  *
@@ -177,12 +193,14 @@
 	int slots, padslots, waitf;
 {
 	struct vm_amap *amap;
-	int totalslots = slots + padslots;
+	int totalslots;
 
 	amap = pool_get(&uvm_amap_pool, (waitf == M_WAITOK) ? PR_WAITOK : 0);
 	if (amap == NULL)
 		return(NULL);
 
+	totalslots = malloc_roundup((slots + padslots) * sizeof(int)) /
+	    sizeof(int);
 	simple_lock_init(&amap->am_l);
 	amap->am_ref = 1;
 	amap->am_flags = 0;
@@ -241,7 +259,7 @@
 	amap = amap_alloc1(slots, padslots, waitf);
 	if (amap)
 		memset(amap->am_anon, 0,
-		    (slots + padslots) * sizeof(struct vm_anon *));
+		    amap->am_maxslot * sizeof(struct vm_anon *));
 
 	UVMHIST_LOG(maphist,"<- done, amap = 0x%x, sz=%d", amap, sz, 0, 0);
 	return(amap);
@@ -291,13 +309,12 @@
 {
 	struct vm_amap *amap = entry->aref.ar_amap;
 	int slotoff = entry->aref.ar_pageoff;
-	int slotmapped, slotadd, slotneed;
+	int slotmapped, slotadd, slotneed, slotadded, slotalloc;
 #ifdef UVM_AMAP_PPREF
 	int *newppref, *oldppref;
 #endif
 	int *newsl, *newbck, *oldsl, *oldbck;
 	struct vm_anon **newover, **oldover;
-	int slotadded;
 	UVMHIST_FUNC("amap_extend"); UVMHIST_CALLED(maphist);
 
 	UVMHIST_LOG(maphist, "  (entry=0x%x, addsize=0x%x)", entry,addsize,0,0);
@@ -366,10 +383,12 @@
 	 */
 
 	amap_unlock(amap);	/* unlock in case we sleep in malloc */
+	slotalloc = malloc_roundup(slotneed * sizeof(int)) / sizeof(int);
 #ifdef UVM_AMAP_PPREF
 	newppref = NULL;
 	if (amap->am_ppref && amap->am_ppref != PPREF_NONE) {
-		newppref = malloc(slotneed * sizeof(int), M_UVMAMAP, M_NOWAIT);
+		newppref = malloc(slotalloc * sizeof(int), M_UVMAMAP,
+		    M_NOWAIT);
 		if (newppref == NULL) {
 			/* give up if malloc fails */
 			free(amap->am_ppref, M_UVMAMAP);
@@ -377,9 +396,9 @@
 		}
 	}
 #endif
-	newsl = malloc(slotneed * sizeof(int), M_UVMAMAP, M_WAITOK);
-	newbck = malloc(slotneed * sizeof(int), M_UVMAMAP, M_WAITOK);
-	newover = malloc(slotneed * sizeof(struct vm_anon *),
+	newsl = malloc(slotalloc * sizeof(int), M_UVMAMAP, M_WAITOK);
+	newbck = malloc(slotalloc * sizeof(int), M_UVMAMAP, M_WAITOK);
+	newover = malloc(slotalloc * sizeof(struct vm_anon *),
 	    M_UVMAMAP, M_WAITOK);
 	amap_lock(amap);			/* re-lock! */
 	KASSERT(amap->am_maxslot < slotneed);
@@ -388,7 +407,7 @@
 	 * now copy everything over to new malloc'd areas...
 	 */
 
-	slotadded = slotneed - amap->am_nslot;
+	slotadded = slotalloc - amap->am_nslot;
 
 	/* do am_slots */
 	oldsl = amap->am_slots;
@@ -404,7 +423,6 @@
 	/* do am_bckptr */
 	oldbck = amap->am_bckptr;
 	memcpy(newbck, oldbck, sizeof(int) * amap->am_nslot);
-	memset(newbck + amap->am_nslot, 0, sizeof(int) * slotadded); /* XXX: needed? */
 	amap->am_bckptr = newbck;
 
 #ifdef UVM_AMAP_PPREF
@@ -417,13 +435,14 @@
 		if ((slotoff + slotmapped) < amap->am_nslot)
 			amap_pp_adjref(amap, slotoff + slotmapped,
 			    (amap->am_nslot - (slotoff + slotmapped)), 1);
-		pp_setreflen(newppref, amap->am_nslot, 1, slotadded);
+		pp_setreflen(newppref, amap->am_nslot, 1,
+		    slotneed - amap->am_nslot);
 	}
 #endif
 
 	/* update master values */
 	amap->am_nslot = slotneed;
-	amap->am_maxslot = slotneed;
+	amap->am_maxslot = slotalloc;
 
 	amap_unlock(amap);
 	free(oldsl, M_UVMAMAP);
@@ -683,6 +702,8 @@
 		amap->am_slots[amap->am_nused] = lcv;
 		amap->am_nused++;
 	}
+	memset(&amap->am_anon[lcv], 0,
+	    (amap->am_maxslot - lcv) * sizeof(struct vm_anon *));
 
 	/*
 	 * drop our reference to the old amap (srcamap) and unlock.