Re: cvs commit: src/sys/sys mbuf.h objcache.h src/sys/kern kern_objcache.c uipc_mbuf.c

["Simon 'corecode' Schubert" <corecode@xxxxxxxxxxxx>]

On 09.06.2005, at 00:22, Matthew Dillon wrote:

> dillon      2005/06/08 15:22:59 PDT
>
> DragonFly src repository
>
>   Modified files:
>     sys/sys              mbuf.h objcache.h
>     sys/kern             kern_objcache.c uipc_mbuf.c
>   Log:
>   Rollup mbuf/objcache fixes.

 	/*
-	 * Both magazines empty.  Get a full magazine from the depot.
+	 * Both magazines empty.  Get a full magazine from the depot and
+	 * move one of the empty ones to the depot.  Do this even if we
+	 * block on the token to avoid a non-optimal corner case.
+	 *
+	 * Obtain the depot token.
 	 */
-
-	/* Obtain the depot token. */
 	depot = &oc->depot[myclusterid];
 	if (!lwkt_trytoken(&ilock, &depot->token)) {
 		lwkt_gettoken(&ilock, &depot->token);
 		++depot->contested;
-		if (!MAGAZINE_EMPTY(cpucache->loaded_magazine) ||
-		    !MAGAZINE_EMPTY(cpucache->previous_magazine)) {
-			lwkt_reltoken(&ilock);
-			goto retry;
-		}
 	}

what exactly would this corner case be? the previous version looked 
pretty sane to me, i.e. why don't we take an obj that was returned by 
an interrupt (on our cpu) when we were blocking for the depot lock?

+
+		/*
+		 * Return emptymag to the depot.  Due to blocking it may
+		 * not be entirely empty.
+		 */
+		if (MAGAZINE_EMPTY(emptymag)) {
+			SLIST_INSERT_HEAD(&depot->emptymagazines,
+					  emptymag, nextmagazine);
+		} else {
+			/*
+			 * NOTE: magazine is not necessarily entirely full
+			 */
+			SLIST_INSERT_HEAD(&depot->fullmagazines,
+					  emptymag, nextmagazine);
+			if (depot->waiting)
+				wakeup(depot);
+		}

isn't this a non-optimal case: returning nearly-empty magazines to the 
full depot and thus producing more traffic (the magazine empties 
faster) on the depot?


 	/*
-	 * Depot layer empty.
+	 * The depot does not have any non-empty magazines.  If we have
+	 * not hit our object limit we can allocate a new object using
+	 * the back-end allocator.
+	 *
+	 * note: unallocated_objects can be initialized to -1, which has
+	 * the effect of removing any allocation limits.
 	 */
+	if (depot->unallocated_objects) {
+		--depot->unallocated_objects;
+		lwkt_reltoken(&ilock);
+		crit_exit();

this should read if (depot->unallocated_objects > 0 || 
depot->unallocated_objects == -1), or are you taking a short cut by 
using the "free" negative space of the int for objects? that seems 
nasty (as we had to get the depot token anyways).

 static int
 mag_purge(struct objcache *oc, struct magazine *mag)
 {
+	int ndeleted;
 	void *obj;
-	int i;

-	for (i = 0; i < mag->rounds; i++) {
-		obj = mag->objects[i];
+	ndeleted = 0;
+	crit_enter();
+	while (mag->rounds) {
+		obj = mag->objects[--mag->rounds];
+		crit_exit();
 		oc->dtor(obj, oc->private);
 		oc->free(obj, oc->allocator_args);
+		++ndeleted;
+		crit_enter();
 	}
-
-	return (mag->rounds);
+	crit_exit();
+	return(ndeleted);
 }

I'm not really sure what the critical sections are good for. Aren't we 
always protected by a critical section anyways? And still if not, can't 
the magazine as well be moved into the depot while we were blocking in 
oc->dtor() or oc->free()? then we shouldn't operate on it anymore (of 
course only holds for magazines that were in the cpu layer before).

static void
 depot_purge(struct magazinedepot *depot, struct objcache *oc)
 {
-	depot->cluster_balance -= maglist_purge(oc, &depot->fullmagazines,
-						TRUE);
-	maglist_purge(oc, &depot->emptymagazines, TRUE);
+	depot->unallocated_objects +=
+		maglist_purge(oc, &depot->fullmagazines, TRUE);
+	depot->unallocated_objects +=
+		maglist_purge(oc, &depot->emptymagazines, TRUE);
+	if (depot->unallocated_objects && depot->waiting)
+		wakeup(depot);
 }

. .. like here.


cheers
  simon

--
Serve - BSD     +++  RENT this banner advert  +++    ASCII Ribbon   /"\
Work - Mac      +++  space for low $$$ NOW!1  +++      Campaign     \ /
Party Enjoy Relax   |   http://dragonflybsd.org      Against  HTML   \
Dude 2c 2 the max   !   http://golden-apple.biz       Mail + News   / \

Attachment: PGP.sig
Description: This is a digitally signed message part