Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 21 Jun 2009 20:30:41 GMT
From:      Edward Tomasz Napierala <trasz@FreeBSD.org>
To:        Perforce Change Reviews <perforce@freebsd.org>
Subject:   PERFORCE change 164828 for review
Message-ID:  <200906212030.n5LKUfjf064888@repoman.freebsd.org>

next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=164828

Change 164828 by trasz@trasz_victim on 2009/06/21 20:29:52

	Trying to get hierarchical resource accounting to work.  This still
	trips on KASSERTs horribly, for some reason.  I'm running out of
	ideas.  ;-/

Affected files ...

.. //depot/projects/soc2009/trasz_limits/sys/kern/kern_exec.c#4 edit
.. //depot/projects/soc2009/trasz_limits/sys/kern/kern_exit.c#7 edit
.. //depot/projects/soc2009/trasz_limits/sys/kern/kern_fork.c#5 edit
.. //depot/projects/soc2009/trasz_limits/sys/kern/kern_hrl.c#14 edit
.. //depot/projects/soc2009/trasz_limits/sys/sys/hrl.h#10 edit
.. //depot/projects/soc2009/trasz_limits/sys/sys/ucred.h#5 edit

Differences ...

==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_exec.c#4 (text+ko) ====

@@ -705,7 +705,7 @@
 		 */
 		change_svuid(newcred, newcred->cr_uid);
 		change_svgid(newcred, newcred->cr_gid);
-		p->p_ucred = newcred;
+		change_cred(p, newcred);
 		newcred = NULL;
 	} else {
 		if (oldcred->cr_uid == oldcred->cr_ruid &&
@@ -728,7 +728,7 @@
 			crcopy(newcred, oldcred);
 			change_svuid(newcred, newcred->cr_uid);
 			change_svgid(newcred, newcred->cr_gid);
-			p->p_ucred = newcred;
+			change_cred(p, newcred);
 			newcred = NULL;
 		}
 	}

==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_exit.c#7 (text+ko) ====

@@ -128,7 +128,7 @@
 	struct ucred *tracecred;
 #endif
 	struct plimit *plim;
-	int locked, i;
+	int locked;
 
 	mtx_assert(&Giant, MA_NOTOWNED);
 
@@ -398,16 +398,6 @@
 	PROC_UNLOCK(p);
 	lim_free(plim);
 
-	for (i = 0; i < HRL_RESOURCE_MAX; i++) {
-		if (p->p_accounting.ha_resources[i] != 0)
-#ifdef notyet
-			printf("exit1: exiting process: resource %d = %lld\n",
-			    i, p->p_accounting.ha_resources[i]);
-#else
-			;
-#endif
-	}
-
 	/*
 	 * Remove proc from allproc queue and pidhash chain.
 	 * Place onto zombproc.  Unlink from parent's child list.
@@ -772,6 +762,8 @@
 	 * Decrement the count of procs running with this uid.
 	 */
 	(void)chgproccnt(p->p_ucred->cr_ruidinfo, -1, 0);
+	hrl_free_proc(p->p_pptr, HRL_RESOURCE_MAXPROCESSES, 1);
+	hrl_proc_exiting(p);
 
 	/*
 	 * Free credentials, arguments, and sigacts.
@@ -934,6 +926,10 @@
 	if (child->p_pptr == parent)
 		return;
 
+	hrl_free_proc(child->p_pptr, HRL_RESOURCE_MAXPROCESSES, 1);
+	/* XXX: What about return value? */
+	hrl_alloc_proc(parent, HRL_RESOURCE_MAXPROCESSES, 1);
+
 	PROC_LOCK(child->p_pptr);
 	sigqueue_take(child->p_ksi);
 	PROC_UNLOCK(child->p_pptr);

==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_fork.c#5 (text+ko) ====

@@ -570,6 +570,12 @@
 	PROC_UNLOCK(p1);
 	PROC_UNLOCK(p2);
 
+	/*
+	 * Initialize HRL accounting information.
+	 * XXX: Handle failure?
+	 */
+	hrl_proc_fork(p1, p2);
+
 	/* Bump references to the text vnode (for procfs) */
 	if (p2->p_textvp)
 		vref(p2->p_textvp);

==== //depot/projects/soc2009/trasz_limits/sys/kern/kern_hrl.c#14 (text+ko) ====

@@ -47,6 +47,11 @@
 #include <sys/tree.h>
 #include <vm/uma.h>
 
+#define	HRF_DEFAULT		0
+#define	HRF_DONT_INHERIT	1
+#define	HRF_DONT_ACCUMULATE	2
+
+
 struct hrl_node {
 	struct hrl_rule		hn_rule;
 	RB_ENTRY(hrl_node)	hn_next;
@@ -93,14 +98,24 @@
 RB_HEAD(hrl_tree, hrl_node) hrls = RB_INITIALIZER(hrls);
 RB_GENERATE_STATIC(hrl_tree, hrl_node, hn_next, hn_compare);
 
+static const char * hrl_resource_name(int resource);
 static void hrl_init(void);
-SYSINIT(hrl, SI_SUB_RUN_SCHEDULER, SI_ORDER_SECOND, hrl_init, NULL);
+SYSINIT(hrl, SI_SUB_CPU, SI_ORDER_FIRST, hrl_init, NULL);
 
 static uma_zone_t hrl_zone;
 static struct mtx hrl_lock;
 
 MALLOC_DEFINE(M_HRL, "hrl", "Hierarchical Resource Limits");
 
+#define notyet
+#if 0
+#undef KASSERT
+#define	KASSERT(exp,msg) do {                                           \
+        if (__predict_false(!(exp)))                                    \
+                printf msg;                                             \
+} while (0)
+#endif
+
 #ifdef INVARIANTS
 /*
  * Go through the accounting info and verify that it makes sense.
@@ -114,7 +129,7 @@
 	struct prison *pr;
 	
 	cred = p->p_ucred;
-
+	mtx_assert(&hrl_lock, MA_OWNED);
 	for (resource = 0; resource < HRL_RESOURCE_MAX; resource++)
 		KASSERT(p->p_accounting.ha_resources[resource] >= 0, ("resource usage propagation meltdown"));
 	KASSERT(cred->cr_ruidinfo->ui_accounting.ha_resources[resource] >= 0, ("resource usage propagation meltdown"));
@@ -128,6 +143,88 @@
 }
 #endif /* INVARIANTS */
 
+void
+hrl_proc_exiting(struct proc *p)
+{
+	int i;
+
+	mtx_lock(&hrl_lock);
+	for (i = 0; i < HRL_RESOURCE_MAX; i++) {
+		if (p->p_accounting.ha_resources[i] != 0)
+#if 0
+			KASSERT(p->p_accounting.ha_resources == 0,
+			    ("dead process still holding resources"));
+#else
+			printf("hrl_proc_exiting: %s = %lld\n",
+			    hrl_resource_name(i),
+			    p->p_accounting.ha_resources[i]);
+			if (p->p_accounting.ha_resources[i] > 0)
+				hrl_free_proc(p, i, p->p_accounting.ha_resources[i]);
+			else
+			    p->p_accounting.ha_resources[i] = 0;
+#endif
+	}
+	mtx_unlock(&hrl_lock);
+}
+
+static int
+hrl_resource_inheritable(int resource)
+{
+
+	switch (resource) {
+	case HRL_RESOURCE_MAXPROCESSES:
+		return (0);
+	default:
+		return (1);
+	}
+}
+
+static const char *
+hrl_resource_name(int resource)
+{
+	switch (resource) {
+	case HRL_RESOURCE_CPUTIME:
+		return ("cputime");
+	case HRL_RESOURCE_FILESIZE:
+		return ("filesize");
+	case HRL_RESOURCE_DATASIZE:
+		return ("datasize");
+	case HRL_RESOURCE_STACKSIZE:
+		return ("stacksize");
+	case HRL_RESOURCE_COREDUMPSIZE:
+		return ("coredumpsize");
+	case HRL_RESOURCE_MEMORYUSE:
+		return ("memoryuse");
+	case HRL_RESOURCE_MEMORYLOCKED:
+		return ("memorylocked");
+	case HRL_RESOURCE_MAXPROCESSES:
+		return ("maxprocesses");
+	case HRL_RESOURCE_OPENFILES:
+		return ("openfiles");
+	case HRL_RESOURCE_SBSIZE:
+		return ("sbsize");
+	case HRL_RESOURCE_VMEMORYUSE:
+		return ("vmemoryuse");
+	case HRL_RESOURCE_PTY:
+		return ("vmemoryuse");
+	default:
+		panic("hrl_resource_name: unknown resource");
+	}
+}
+
+void
+hrl_proc_fork(struct proc *parent, struct proc *child)
+{
+	int i;
+
+	mtx_lock(&hrl_lock);
+	for (i = 0; i < HRL_RESOURCE_MAX; i++) {
+		if (parent->p_accounting.ha_resources[i] != 0 && hrl_resource_inheritable(i))
+			hrl_allocated_proc(child, i, parent->p_accounting.ha_resources[i]);
+	}
+	mtx_unlock(&hrl_lock);
+}
+
 /*
  * Increase allocation of 'resource' by 'amount' for process 'p'.
  * Return 0 if it's below limits, or errno, if it's not.
@@ -135,27 +232,43 @@
 int
 hrl_alloc_proc(struct proc *p, int resource, uint64_t amount)
 {
-	int i;
+	int i, j;
 	struct ucred *cred;
 	struct prison *pr;
 
-	KASSERT(amount > 0, ("invalid amount"));
+	KASSERT(amount > 0, ("hrl_alloc_proc: invalid amount for %s: %lld",
+	    hrl_resource_name(resource), amount));
+	if (amount <= 0)
+		panic("bleh.");
 
-	/*
-	 * XXX: Obviously wrong, fix later.
-	 */
+	mtx_lock(&hrl_lock);
 	p->p_accounting.ha_resources[resource] += amount;
 	cred = p->p_ucred;
 	cred->cr_ruidinfo->ui_accounting.ha_resources[resource] += amount;
-	cred->cr_uidinfo->ui_accounting.ha_resources[resource] += amount;
+	if (cred->cr_ruidinfo != cred->cr_uidinfo)
+		cred->cr_uidinfo->ui_accounting.ha_resources[resource] += amount;
 	for (pr = cred->cr_prison; pr != NULL; pr = pr->pr_parent)
 		pr->pr_accounting.ha_resources[resource] += amount;
-	for (i = 0; i < cred->cr_ngroups; i++)
+	/*
+	 * XXX: Slow.
+	 */
+	for (i = 0; i < cred->cr_ngroups; i++) {
+		/*
+		 * Make sure we don't account a group more than once if it appears
+		 * in cr_groups[] more than once.
+		 */
+		for (j = 0; j < i; j++) {
+			if (cred->cr_groups[i] == cred->cr_groups[j])
+				goto skip_group;
+		}
 		cred->cr_gidinfos[i]->gi_accounting.ha_resources[resource] += amount;
-
+skip_group:
+		continue;
+	}
 #ifdef INVARIANTS
 	hrl_assert_proc(p);
 #endif
+	mtx_unlock(&hrl_lock);
 
 	/*
 	 * XXX: When denying, return proper errno - EFSIZ, ENOMEM etc.
@@ -174,24 +287,45 @@
 int
 hrl_allocated_proc(struct proc *p, int resource, uint64_t amount)
 {
-	int i;
+	int i, j;
 	int64_t diff;
 	struct ucred *cred;
 	struct prison *pr;
 
+	KASSERT(amount > 0, ("hrl_allocated_proc: invalid amount for %s: %lld",
+	    hrl_resource_name(resource), amount));
+	if (amount <= 0)
+		panic("bleh.");
+
+	mtx_lock(&hrl_lock);
 	diff = amount - p->p_accounting.ha_resources[resource];
-	p->p_accounting.ha_resources[resource] += diff;
+	p->p_accounting.ha_resources[resource] = amount;
 	cred = p->p_ucred;
 	cred->cr_ruidinfo->ui_accounting.ha_resources[resource] += diff;
-	cred->cr_uidinfo->ui_accounting.ha_resources[resource] += diff;
+	if (cred->cr_ruidinfo != cred->cr_uidinfo)
+		cred->cr_uidinfo->ui_accounting.ha_resources[resource] += diff;
 	for (pr = cred->cr_prison; pr != NULL; pr = pr->pr_parent)
 		pr->pr_accounting.ha_resources[resource] += diff;
-	for (i = 0; i < cred->cr_ngroups; i++)
+	/*
+	 * XXX: Slow.
+	 */
+	for (i = 0; i < cred->cr_ngroups; i++) {
+		/*
+		 * Make sure we don't account a group more than once if it appears
+		 * in cr_groups[] more than once.
+		 */
+		for (j = 0; j < i; j++) {
+			if (cred->cr_groups[i] == cred->cr_groups[j])
+				goto skip_group;
+		}
 		cred->cr_gidinfos[i]->gi_accounting.ha_resources[resource] += diff;
-
+skip_group:
+		continue;
+	}
 #ifdef INVARIANTS
 	hrl_assert_proc(p);
 #endif
+	mtx_unlock(&hrl_lock);
 
 	return (0);
 }
@@ -202,27 +336,49 @@
 void
 hrl_free_proc(struct proc *p, int resource, uint64_t amount)
 {
-	int i;
+	int i, j;
 	struct ucred *cred;
 	struct prison *pr;
 
-	KASSERT(amount > 0, ("invalid amount"));
+	KASSERT(amount > 0, ("hrl_free_proc: invalid amount for %s: %lld",
+	    hrl_resource_name(resource), amount));
+	if (amount <= 0)
+		panic("bleh.");
 
+	mtx_lock(&hrl_lock);
 	p->p_accounting.ha_resources[resource] -= amount;
 #ifdef notyet
-	KASSERT(amount >= p->p_accounting.ha_resources[resource], ("freeing more than allocated"));
+	KASSERT(amount <= p->p_accounting.ha_resources[resource],
+	    ("hrl_free_proc: freeing %lld, which is more than allocated %lld "
+	    "for %s", amount, p->p_accounting.ha_resources[resource],
+	    hrl_resource_name(resource)));
 #endif
 	cred = p->p_ucred;
 	cred->cr_ruidinfo->ui_accounting.ha_resources[resource] -= amount;
-	cred->cr_uidinfo->ui_accounting.ha_resources[resource] -= amount;
+	if (cred->cr_ruidinfo != cred->cr_uidinfo)
+		cred->cr_uidinfo->ui_accounting.ha_resources[resource] -= amount;
 	for (pr = cred->cr_prison; pr != NULL; pr = pr->pr_parent)
 		pr->pr_accounting.ha_resources[resource] -= amount;
-	for (i = 0; i < cred->cr_ngroups; i++)
+	/*
+	 * XXX: Slow.
+	 */
+	for (i = 0; i < cred->cr_ngroups; i++) {
+		/*
+		 * Make sure we don't account a group more than once if it appears
+		 * in cr_groups[] more than once.
+		 */
+		for (j = 0; j < i; j++) {
+			if (cred->cr_groups[i] == cred->cr_groups[j])
+				goto skip_group;
+		}
 		cred->cr_gidinfos[i]->gi_accounting.ha_resources[resource] -= amount;
-
+skip_group:
+		continue;
+	}
 #ifdef INVARIANTS
 	hrl_assert_proc(p);
 #endif
+	mtx_unlock(&hrl_lock);
 }
 
 /*
@@ -263,7 +419,7 @@
 		node = RB_FIND(hrl_tree, &hrls, &searched);
 		if (node != NULL) {
 			node = RB_REMOVE(hrl_tree, &hrls, node);
-			KASSERT(node != NULL, ("node removal failed"));
+			KASSERT(node != NULL, ("hrl_adjust: node removal failed"));
 		}
 		mtx_unlock(&hrl_lock);
 		if (node != NULL)
@@ -292,16 +448,18 @@
 {
 	int i;
 
+	mtx_lock(&hrl_lock);
 	for (i = 0; i < HRL_RESOURCE_MAX; i++) {
 #ifdef notyet
 		KASSERT(dest->ha_resources[i] >= 0, ("resource usage propagation meltdown"));
+		KASSERT(src->ha_resources[i] >= 0, ("resource usage propagation meltdown"));
 #endif
 		dest->ha_resources[i] += src->ha_resources[i];
 #ifdef notyet
-		KASSERT(src->ha_resources[i] >= 0, ("resource usage propagation meltdown"));
 		KASSERT(dest->ha_resources[i] >= 0, ("resource usage propagation meltdown"));
 #endif
 	}
+	mtx_unlock(&hrl_lock);
 }
 
 void
@@ -309,16 +467,19 @@
 {
 	int i;
 
+	mtx_lock(&hrl_lock);
 	for (i = 0; i < HRL_RESOURCE_MAX; i++) {
 #ifdef notyet
 		KASSERT(dest->ha_resources[i] >= 0, ("resource usage propagation meltdown"));
+		KASSERT(src->ha_resources[i] >= 0, ("resource usage propagation meltdown"));
+		KASSERT(src->ha_resources[i] <= dest->ha_resources[i], ("resource usage propagation meltdown"));
 #endif
 		dest->ha_resources[i] -= src->ha_resources[i];
 #ifdef notyet
-		KASSERT(src->ha_resources[i] >= 0, ("resource usage propagation meltdown"));
 		KASSERT(dest->ha_resources[i] >= 0, ("resource usage propagation meltdown"));
 #endif
 	}
+	mtx_unlock(&hrl_lock);
 }
 
 /*
@@ -526,7 +687,7 @@
 			continue;
 
 		node = RB_REMOVE(hrl_tree, &hrls, node);
-		KASSERT(node != NULL, ("node removal failed"));
+		KASSERT(node != NULL, ("hrl_proc_exit: node removal failed"));
 
 		mtx_unlock(&hrl_lock);
 		uma_zfree(hrl_zone, node);
@@ -541,6 +702,6 @@
 
 	hrl_zone = uma_zcreate("hrl", sizeof(struct hrl_node), NULL, NULL,
 	    NULL, NULL, UMA_ALIGN_PTR, UMA_ZONE_NOFREE);
-	mtx_init(&hrl_lock, "hrl lock", NULL, MTX_DEF);
+	mtx_init(&hrl_lock, "hrl lock", NULL, MTX_RECURSE); /* XXX: Make it non-recurseable later. */
 	EVENTHANDLER_REGISTER(process_exit, hrl_proc_exit, NULL, EVENTHANDLER_PRI_ANY);
 }

==== //depot/projects/soc2009/trasz_limits/sys/sys/hrl.h#10 (text+ko) ====

@@ -120,6 +120,8 @@
 void	hrl_adjust(int subject, id_t subject_id, int per, int resource, int action, int64_t amount);
 void	hrl_acc_add(struct hrl_acc *dest, const struct hrl_acc *src);
 void	hrl_acc_subtract(struct hrl_acc *dest, const struct hrl_acc *src);
+void	hrl_proc_exiting(struct proc *p);
+void	hrl_proc_fork(struct proc *parent, struct proc *child);
 
 #else /* !_KERNEL */
 

==== //depot/projects/soc2009/trasz_limits/sys/sys/ucred.h#5 (text+ko) ====

@@ -84,7 +84,9 @@
 
 #ifdef _KERNEL
 struct thread;
+struct proc;
 
+void	change_cred(struct proc *p, struct ucred *newcred);
 void	change_egid(struct ucred *newcred, struct gidinfo *egip);
 void	change_euid(struct ucred *newcred, struct uidinfo *euip);
 void	change_rgid(struct ucred *newcred, gid_t rgid);



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200906212030.n5LKUfjf064888>